[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)

[Donát Nagy via cfe-commits]

================
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
           return nullptr;
         }
 
-      // See if we're releasing memory while inlining a destructor
-      // (or one of its callees). This turns on various common
-      // false positive suppressions.
-      bool FoundAnyDestructor = false;
-      for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-        if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
-          if (isReferenceCountingPointerDestructor(DD)) {
-            // This immediately looks like a reference-counting destructor.
-            // We're bad at guessing the original reference count of the object,
-            // so suppress the report for now.
-            BR.markInvalid(getTag(), DD);
-          } else if (!FoundAnyDestructor) {
-            assert(!ReleaseDestructorLC &&
+        // See if we're releasing memory while inlining a destructor or
+        // functions that decrement reference counters (or one of its callees).
+        // This turns on various common false positive suppressions.
+        bool FoundAnyReleaseFunction = false;
+        for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+          if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
+            if (isReferenceCountingPointerDestructor(DD)) {
+              // This immediately looks like a reference-counting destructor.
+              // We're bad at guessing the original reference count of the
+              // object, so suppress the report for now.
+              BR.markInvalid(getTag(), DD);
+              continue;
+            }
+          }
+
+          if (!FoundAnyReleaseFunction) {
----------------
NagyDonat wrote:

The trickery with this boolean `FoundAny` is just useless cruft now, because `ReleaseFunctionLC` will always be equal to `CurrentLC` (unless we hit the `continue` after the `markInvalid()` call, but that case is irrelevant).

Previously the `for` loop was walking upwards on the call stack, looking for the innermost destructor call that contained the `free()` call -- now you immediately bind `ReleaseFunctionLC` in the first iteration of the loop.

This means that while your change can suppress more reports in situation where the call stack doesn't contain a destructor, it can also reduce the amount of suppression in cases where instead of a destructor stack frame it will look for the atomic ops only in the stack frame that directly contains the `free()`

This means that the suppression will no longer work for something like
```cpp
SmartPointr::~SmartPointr() {
  if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed) == 1)
    release_resources();
}
void SmartPointr::release_resources() {
  free(buffer);
  // probably some additional cleanup happens here
}
```
because the old code would've set `ReleaseDestructorLC` to the stack frame of the destructor `~SmartPointr()` (and finds that it contains an atomic subtraction call), while the new code sets  `ReleaseFunctionLC` to `release_resources()` which presumably doesn't use atomic add/sub.

(By the way, please add a testcase that tests this situation -- I'm a bit surprised that we don't already have a test like this. Note that I used the distorted name `SmartPointr` to avoid the other heuristic which checks the class name.)

I think this loss of suppression may be "acceptable collateral damage" for your change, but you should think about it and perhaps consider alternative solutions. (E.g. you could initialize `ReleaseFunctionLC` to `CurrentLC`, but then if you find a destructor on the stack, then you switch to using that (innermost) destructor as `ReleaseFunctionLC`.)

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