[cfe-dev] Ownership attribute for malloc etc. checking

[Jordy Rose]

Looking good! Some remaining comments below.

I'm also a reasonably new committer, so someone else should probably look
over this as well (particularly the Attr hunks, which I'm not very familiar
with).

Jordy


General:
- It would be nice to make sure one argument isn't both ownership_takes
and ownership_holds.
- LLVM's convention is to use spaces rather than tabs.
- There's a hunk where the only thing that changed is indentation; it'd be
nice to take that out.
- Two more useful test would be free(p); my_free(p) and free(p);
my_hold(p). Both of which should warn, of course.


(MallocChecker::EvalCallExpr)
+    for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext())
{
+      if (const OwnershipReturnsAttr* Att =
dyn_cast<OwnershipReturnsAttr>(attr)) {
+        MallocMemReturnsAttr(C, CE, Att);
+        rv = true;
+      }
+      if (const OwnershipTakesAttr* Att = dyn_cast<OwnershipTakesAttr>
(attr)) {
+        FreeMemTakesAttr(C, CE, Att);
+        rv = true;
+      }
+      if (const OwnershipHoldsAttr* Att = dyn_cast<OwnershipHoldsAttr>
(attr)) {
+        FreeMemHoldsAttr(C, CE, Att);
+        rv = true;
+      }
+    }

Here's the dispatch from EvalCallExpr(). I think even though it's
technically a little less efficient, you should just simplify this and use
FD->getAttr<OwnershipTakesAttr>(), etc. It's a little simpler, and it lets
you order the attributes.

Why does that matter? Because an ownership_takes() or ownership_holds()
function might have a return value! If it's also ownership_returns(), then
you're fine, but otherwise you need to set a symbolic return value. (You
can see how this is done in MallocMemAux(), or in StreamChecker.cpp with
fopen.) There should probably be a test for this as well.

Actually, because these attributes don't affect the return value, they
should probably go in a PreVisitCallExpr() callback, rather than
EvalCallExpr(). But for now it's probably okay, as long as you handle that
return value.


(MallocChecker::MallocMemReturnsAttr)
+  for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I,
++count) {
+    const GRState *state =
+        MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(),
C.getState());
+    C.addTransition(state);
+  }
+  if (count == 0) {
+    const GRState *state = MallocMemAux(C, CE, UndefinedVal(),
UndefinedVal(),
+                                        C.getState());
+    C.addTransition(state);
+  }

What does it mean to have multiple size fields? There can still only be
one return value. Maybe you should only allow zero or one positions for
ownership_returns (which would make this section simpler).


(MallocChecker::PreVisitReturnStmt)
+  if (RS->isReleased()) {
+    ExplodedNode *N = C.GenerateSink();
+    if (!BT_UseFree)
+      BT_UseFree = new BuiltinBug("Use dynamically allocated memory
after"
+                                  " it is freed.");
 
+    BugReport *R = new BugReport(*BT_UseFree,
BT_UseFree->getDescription(),
+                                 N);
+    C.EmitReport(R);
+  }

This section checks whether the return value is a released address.
Unfortunately this isn't a 100% unambiguous case; consider the following
(contrived) example:

struct Node *freeSmallest () {
  struct Node *min;
  // find the minimum node
  free(min);
  return min; // so the caller knows the ID of the deleted node
}

As long as the caller doesn't dereference the returned pointer, it could
still be a useful value. 95% of the time, this check would be useful, but
since a false positive is worse than a false negative for the analyzer, we
should probably leave it out.