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

[Jordy Rose]

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.
-------------- next part --------------
Index: lib/Checker/MallocChecker.cpp
===================================================================
--- lib/Checker/MallocChecker.cpp	(revision 105311)
+++ lib/Checker/MallocChecker.cpp	(working copy)
@@ -59,11 +59,14 @@
   BuiltinBug *BT_DoubleFree;
   BuiltinBug *BT_Leak;
   BuiltinBug *BT_UseFree;
+  BuiltinBug *BT_FreeNonRegion;
+  BuiltinBug *BT_FreeNonMalloc;
   IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc;
 
 public:
   MallocChecker() 
-    : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), 
+    : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_FreeNonRegion(0),
+      BT_FreeNonMalloc(0),
       II_malloc(0), II_free(0), II_realloc(0), II_calloc(0) {}
   static void *getTag();
   bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
@@ -197,12 +200,52 @@
   if (ArgVal.isZeroConstant())
     return state;
 
-  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;
+  }
+  
+  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.
+    // 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;
+  }
+  
+  const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R);
   // Various cases could lead to non-symbol values here.
-  if (!Sym)
+  // For now, ignore them.
+  if (!SR)
     return state;
 
+  SymbolRef Sym = SR->getSymbol();
+  
   const RefState *RS = state->get<RegionState>(Sym);
 
   // If the symbol has not been tracked, return. This is possible when free() is
Index: test/Analysis/free.c
===================================================================
--- test/Analysis/free.c	(revision 0)
+++ test/Analysis/free.c	(revision 0)
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
+void free(void *);
+
+void t1 () {
+  int a[] = { 1 };
+  free(a); // expected-warning {{not allocated by malloc()}}
+}
+
+void t2 () {
+  int a = 1;
+  free(&a); // expected-warning {{not allocated by malloc()}}
+}
+
+void t3 () {
+  static int a[] = { 1 };
+  free(a); // expected-warning {{not allocated by malloc()}}
+}
+
+void t4 (char *x) {
+  free(x); // no-warning
+}
+
+void t5 () {
+  extern char *ptr();
+  free(ptr()); // no-warning
+}
+
+void t6 () {
+  free((void*)1000); // expected-warning {{not a valid location}}
+}
+
+void t7 (char **x) {
+  free(*x); // no-warning
+}
+
+void t8 (char **x) {
+  // ugh
+  free((*x)+8); // no-warning
+}
+
+void t9 () {
+label:
+  free(&&label); // expected-warning {{not a valid location}}
+}