[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:

> And there is no warning. Did I get it wrong?

Your test code differs from the case that I tried to describe in two aspects:
1. The name of the class is recognized by `isReferenceCountingPointerDestructor()` (which does case-insensitive substring checks for several keywords, including "intrustive" and "ptr"), so the use-after-free report is suppressed by that name-based heuristic, which suppresses all use-after-free errors (directly or indirectly) within a destructor of a class whose name is recognized.
2. Your `DecRef()` call is not recognized by the heuristics defined in `MallocBugVisitor::VisitNode` [within the `if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC || ReleaseDestructorLC->isParentOf(CurrentLC)))` block] which searches for either C11 atomic add/sub instructions or methods called on an object whose class name contains "atomic".

If you rename the class (to avoid the other heuristic) and replace the abstract/opaque `DecRef` calls with visibly atomic operations then you will get a testcase that:
- is suppressed without your commit (because the visitor finds the destructor which contains both the `delete` operation and the atomic call)
- is not suppressed with your commit (because the name-based heuristic doesn't trigger AND the visitor only searches for the atomic call within the stack frame that directly contains the `delete` operation, and that doesn't contain an atomic call).

(Disclaimer: I didn't test this experimentally, I'm just reasoning based on the source code.)

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