[cfe-commits] [patch] Catch a few simple misuses of free()

[Ted Kremenek]

Hi Jordy,

This is a good start, but there are some issues.  Comments inline.

-  SymbolRef Sym = ArgVal.getAsLocSymbol();
-
+  const MemRegion *R = ArgVal.getAsRegion();
+  if (!R) {
+    
+    ExplodedNode *N = C.GenerateSink();
+    if (N) {
+      if (!BT_FreeNonRegion)
+        BT_FreeNonRegion = new BuiltinBug("Bad free",
+                         "Argument to free() is not a valid location");
+      BugReport *R = new BugReport(*BT_FreeNonRegion, 
+                                   BT_FreeNonRegion->getDescription(), N);
+      C.EmitReport(R);
+    }
+    return NULL;
+  }

Please add a brief comment here about what you are checking.  This also seems like a really terse and uninformative diagnostic, and the check seems overly promiscuous.  This could easily happen if ArgVal is 'UnknownVal' which just means we didn't have enough precision to track the value for the argument.  The notion of "location" is a detail of the static analyzer, and it isn't helpful to the user.

Please refine this check to special case the diagnostics and to make it less promiscuous.  Specifically, don't issue a warning for UnknownVal, and you don't need to handle UndefinedVal (since that is handled elsewhere).  Concerning diagnostics, consider the following from your test cases:

+  free((void*)1000); // expected-warning {{not a valid location}}

I'd expect to see something like:

   "Argument to free() is a constant address (0x1000) which is not memory returned by malloc()

Similarly:

  "Argument to free() is the address of a label ("label") which is not memory returned by malloc()

Isn't that a lot more helpful?

You don't need to add it for every case; even something generic like "Argument to free() refers to memory not returned by malloc()" and then special case the common cases.  Users really notice this polish.  For example:

   int p;
   free(&p)

Saying that the user is freeing 'p' helps convince them that the analyzer knows what it is talking about and also clearly identifies the problem.  The goal of static analyzer diagnostics is to not make them too verbose, but also really informative.  In cases like this, the "free" can happen long after the pointer value that is passed to "free" gets computed.  A lucid diagnostic makes it much clearer to the user what is going on.

+  
+  R = R->StripCasts();
+  
+  const MemSpaceRegion *MS = R->getMemorySpace();
+  if (!isa<UnknownSpaceRegion>(MS)) {
+    // Conjured symbols are all unknown-space right now,
+    // but malloc() regions should be in HeapSpaceRegion.

While HeapSpaceRegion isn't being used for malloc()'ed memory right now, shouldn't we include it in the condition anyway?  It seems like this is a really general check just to see if the memory is allocated from the stack or is a global variable.

+    // Of course, free() can work on memory allocated outside the current
+    // function, so UnknownSpaceRegion is always a possibility.
+    
+    ExplodedNode *N = C.GenerateSink();
+    if (N) {
+      if (!BT_FreeNonMalloc)
+        BT_FreeNonMalloc = new BuiltinBug("Bad free",
+                         "Try to free a memory block that was not allocated "
+                         "by malloc()");
+      // FIXME: should show where the memory came from
+      BugReport *R = new BugReport(*BT_FreeNonMalloc, 
+                                   BT_FreeNonMalloc->getDescription(), N);
+      C.EmitReport(R);
+    }
+    return NULL;
+  }

Another case you can easily warn about is freeing the address of a function or block.

On Jun 3, 2010, at 8:28 PM, Jordy Rose wrote:

> Catch free()s on non-regions and regions known to be not from malloc() (by
> checking the memory space).
> 
> (This actually arose because I was trying to test branches with bad
> free()s and was surprised they weren't warning.)
> 
> BTW: even though I was granted commit access, I assume "[patch]" is still
> a good way to introduce these small changes.<bad-free.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits