[clang] [analyzer] Eliminate unique release point assertion (PR #150240)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

<details>
<summary>Changes</summary>

MallocChecker.cpp has a complex heuristic that supresses reports where the memory release happens during the release of a reference-counted object (to suppress a significant amount of false positives).

Previously this logic asserted that there is at most one release point corresponding to a symbol, but it turns out that there is a rare corner case where the symbol can be released, forgotten and then released again. This commit removes that assertion to avoid the crash. (As this issue just affects a bug suppression heuristic, I didn't want to dig deeper in the state changes.)

Fixes #<!-- -->149754

---
Full diff: https://github.com/llvm/llvm-project/pull/150240.diff


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+8-6) 
- (modified) clang/test/Analysis/malloc.c (+14) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 68efdbaec341b..a7704da82fcc2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3730,13 +3730,15 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
           return nullptr;
         }
 
-        // Save the first destructor/function as release point.
-        assert(!ReleaseFunctionLC && "There should be only one release point");
+        // Record the stack frame that is _responsible_ for this memory release
+        // event. This will be used by the false positive suppression heuristics
+        // that recognize the release points of reference-counted objects.
+        //
+        // Usually (e.g. in C) we say that the _responsible_ stack frame is the
+        // current innermost stack frame:
         ReleaseFunctionLC = CurrentLC->getStackFrame();
-
-        // See if we're releasing memory while inlining a destructor that
-        // decrement reference counters (or one of its callees).
-        // This turns on various common false positive suppressions.
+        // ...but if the stack contains a destructor call, then we say that the
+        // outermost destructor stack frame is the _responsible_ one:
         for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
           if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
             if (isReferenceCountingPointerDestructor(DD)) {
diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c
index 27a04ff873521..877b187e65997 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -1960,3 +1960,17 @@ void testExtent(void) {
   // expected-warning-re at -1 {{{{^conj_\$[[:digit:]]+{int, LC1, S[[:digit:]]+, #1}}}}}}
   free(p);
 }
+
+void gh149754(void *p) {
+  // This testcase demonstrates an unusual situation where a certain symbol
+  // (the value of `p`) is released (more precisely, transitions from
+  // untracked state to Released state) twice within the same bug path because
+  // the `EvalAssume` callback resets it to untracked state after the first
+  // time when it is released. This caused the failure of an assertion, which
+  // was since then removed for the codebase.
+  if (!realloc(p, 8)) {
+    realloc(p, 8);
+    free(p); // expected-warning {{Attempt to free released memory}}
+  }
+  // expected-warning at +1 {{Potential memory leak}}
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/150240