[PATCH] D79415: [analyzer][MalloChecker] When modeling realloc-like functions, don't early return if the argument is symbolic

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, balazske, martong, baloghadamsoftware, vsavchenko, xazax.hun, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Szelethus edited the summary of this revision.
Szelethus edited the summary of this revision.

The very essence of MallocChecker lies in 2 overload sets: the `FreeMemAux` functions and the `MallocMemAux` functions. The former houses most of the error checking as well (aside from leaks), such as incorrect deallocation. There, we check whether the argument's `MemSpaceRegion` <https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemSpaceRegion.html> is the heap or unknown, and if it isn't, we know we encountered a bug (aside from a corner case patched by @balazske in D76830 <https://reviews.llvm.org/D76830>), as specified by MEM34-C <https://wiki.sei.cmu.edu/confluence/display/c/MEM34-C.+Only+free+memory+allocated+dynamically>.

In `ReallocMemAux`, which really is the combination of  `FreeMemAux` and `MallocMemAux`, we incorrectly early returned if the memory argument of realloc is non-symbolic. The problem is, one of the cases where this happens when we know precisely what the region is, like an array, as demonstrated in the test file. So, lets  get rid of this false negative :^)

Side note, I dislike the warning message and the associated checker name, but I'll address it in a later patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79415

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/malloc.c


Index: clang/test/Analysis/malloc.c
===================================================================
--- clang/test/Analysis/malloc.c
+++ clang/test/Analysis/malloc.c
@@ -1828,6 +1828,21 @@
   list_add(list, &x->li); // will free 'x'.
 }
 
+// MEM34-C. Only free memory allocated dynamically
+// Second non-compliant example.
+// https://wiki.sei.cmu.edu/confluence/display/c/MEM34-C.+Only+free+memory+allocated+dynamically
+enum { BUFSIZE = 256 };
+
+void MEM34_C(void) {
+  char buf[BUFSIZE];
+  char *p = (char *)realloc(buf, 2 * BUFSIZE);
+  // expected-warning at -1{{Argument to realloc() is the address of the local \
+variable 'buf', which is not memory allocated by malloc() [unix.Malloc]}}
+  if (p == NULL) {
+    /* Handle error */
+  }
+}
+
 // ----------------------------------------------------------------------------
 // False negatives.
 
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2391,13 +2391,7 @@
   if (PrtIsNull && SizeIsZero)
     return State;
 
-  // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size).
   assert(!PrtIsNull);
-  SymbolRef FromPtr = arg0Val.getAsSymbol();
-  SVal RetVal = C.getSVal(CE);
-  SymbolRef ToPtr = RetVal.getAsSymbol();
-  if (!FromPtr || !ToPtr)
-    return nullptr;
 
   bool IsKnownToBeAllocated = false;
 
@@ -2426,6 +2420,14 @@
     else if (!IsKnownToBeAllocated)
       Kind = OAR_DoNotTrackAfterFailure;
 
+    // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size).
+    SymbolRef FromPtr = arg0Val.getAsSymbol();
+    SVal RetVal = C.getSVal(CE);
+    SymbolRef ToPtr = RetVal.getAsSymbol();
+    assert(FromPtr && ToPtr &&
+           "By this point, FreeMemAux and MallocMemAux should have checked "
+           "whether the argument or the return value is symbolic!");
+
     // Record the info about the reallocated symbol so that we could properly
     // process failed reallocation.
     stateRealloc = stateRealloc->set<ReallocPairs>(ToPtr,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79415.262088.patch
Type: text/x-patch
Size: 2157 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200505/8ac874cd/attachment-0001.bin>