[analyzer] [MallocChecker] suspect all release functions as candidate for suppression (#104599)

Current MalloChecker logic suppresses FP caused by refcounting only for
C++ destructors. The same pattern occurs a lot in C in objects with
intrusive refcounting. See #104229 for code example.

To extend current logic to C, suspect all release functions as candidate
for suppression.

Closes: #104229

Author: pskrgag

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -940,16 +940,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
   // A symbol from when the primary region should have been reallocated.
   SymbolRef FailedReallocSymbol;
 
-  // A C++ destructor stack frame in which memory was released. Used for
+  // A release function stack frame in which memory was released. Used for
   // miscellaneous false positive suppression.
-  const StackFrameContext *ReleaseDestructorLC;
+  const StackFrameContext *ReleaseFunctionLC;
 
   bool IsLeak;
 
 public:
   MallocBugVisitor(SymbolRef S, bool isLeak = false)
       : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-        ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+        ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
 
   static void *getTag() {
     static int Tag = 0;
@@ -3653,21 +3653,25 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
 
   const LocationContext *CurrentLC = N->getLocationContext();
 
-  // If we find an atomic fetch_add or fetch_sub within the destructor in which
-  // the pointer was released (before the release), this is likely a destructor
-  // of a shared pointer.
+  // If we find an atomic fetch_add or fetch_sub within the function in which
+  // the pointer was released (before the release), this is likely a release
+  // point of reference-counted object (like shared pointer).
+  //
   // Because we don't model atomics, and also because we don't know that the
   // original reference count is positive, we should not report use-after-frees
-  // on objects deleted in such destructors. This can probably be improved
+  // on objects deleted in such functions. This can probably be improved
   // through better shared pointer modeling.
-  if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
-                              ReleaseDestructorLC->isParentOf(CurrentLC))) {
+  if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+                            ReleaseFunctionLC->isParentOf(CurrentLC))) {
     if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
       // Check for manual use of atomic builtins.
       AtomicExpr::AtomicOp Op = AE->getOp();
       if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
           Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
         BR.markInvalid(getTag(), S);
+        // After report is considered invalid there is no need to proceed
+        // futher.
+        return nullptr;
       }
     } else if (const auto *CE = dyn_cast<CallExpr>(S)) {
       // Check for `std::atomic` and such. This covers both regular method calls
@@ -3679,6 +3683,9 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
         // "__atomic_base" or something.
         if (StringRef(RD->getNameAsString()).contains("atomic")) {
           BR.markInvalid(getTag(), S);
+          // After report is considered invalid there is no need to proceed
+          // futher.
+          return nullptr;
         }
       }
     }
@@ -3750,35 +3757,54 @@ 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 &&
-                   "There can be only one release point!");
-            // Suspect that it's a reference counting pointer destructor.
-            // On one of the next nodes might find out that it has atomic
-            // reference counting operations within it (see the code above),
-            // and if so, we'd conclude that it likely is a reference counting
-            // pointer destructor.
-            ReleaseDestructorLC = LC->getStackFrame();
-            // It is unlikely that releasing memory is delegated to a destructor
-            // inside a destructor of a shared pointer, because it's fairly hard
-            // to pass the information that the pointer indeed needs to be
-            // released into it. So we're only interested in the innermost
-            // destructor.
-            FoundAnyDestructor = true;
+        // Save the first destructor/function as release point.
+        assert(!ReleaseFunctionLC && "There should be only one release point");
+        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.
+        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);
+
+              // After report is considered invalid there is no need to proceed
+              // futher.
+              return nullptr;
+            }
+
+            // Switch suspection to outer destructor to catch patterns like:
+            // (note that class name is distorted to bypass
+            // isReferenceCountingPointerDestructor() logic)
+            //
+            // SmartPointr::~SmartPointr() {
+            //  if (refcount.fetch_sub(1) == 1)
+            //    release_resources();
+            // }
+            // void SmartPointr::release_resources() {
+            //   free(buffer);
+            // }
+            //
+            // This way ReleaseFunctionLC will point to outermost destructor and
+            // it would be possible to catch wider range of FP.
+            //
+            // NOTE: it would be great to support smth like that in C, since
+            // currently patterns like following won't be supressed:
+            //
+            // void doFree(struct Data *data) { free(data); }
+            // void putData(struct Data *data)
+            // {
+            //   if (refPut(data))
+            //     doFree(data);
+            // }
+            ReleaseFunctionLC = LC->getStackFrame();
           }
         }
-      }
+
     } else if (isRelinquished(RSCurr, RSPrev, S)) {
       Msg = "Memory ownership is transferred";
       StackHint = std::make_unique<StackHintGeneratorForSymbol>(Sym, "");

clang/test/Analysis/NewDelete-atomics.cpp

@@ -97,6 +97,32 @@ class DifferentlyNamed {
   T *getPtr() const { return Ptr; } // no-warning
 };
 
+// Also IntrusivePtr with different name and outline release in destructor
+template <typename T>
+class DifferentlyNamedOutlineRelease {
+  T *Ptr;
+
+public:
+  DifferentlyNamedOutlineRelease(T *Ptr) : Ptr(Ptr) {
+    Ptr->incRef();
+  }
+
+  DifferentlyNamedOutlineRelease(const DifferentlyNamedOutlineRelease &Other) : Ptr(Other.Ptr) {
+    Ptr->incRef();
+  }
+
+  void releasePtr(void) {
+      delete Ptr;
+  }
+
+  ~DifferentlyNamedOutlineRelease() {
+    if (Ptr->decRef() == 1)
+      releasePtr();
+  }
+
+  T *getPtr() const { return Ptr; } // no-warning
+};
+
 void testDestroyLocalRefPtr() {
   IntrusivePtr<RawObj> p1(new RawObj());
   {
@@ -176,3 +202,23 @@ void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed(
   // p1 still maintains ownership. The object is not deleted.
   p1.getPtr()->foo(); // no-warning
 }
+
+void testDestroyLocalRefPtrWithOutlineRelease() {
+  DifferentlyNamedOutlineRelease <RawObj> p1(new RawObj());
+  {
+    DifferentlyNamedOutlineRelease <RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtrWithOutlineRelease(
+    const DifferentlyNamedOutlineRelease<RawObj> &p1) {
+  {
+    DifferentlyNamedOutlineRelease <RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}

clang/test/Analysis/malloc-refcounted.c

@@ -0,0 +1,80 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -verify %s
+//
+
+typedef __SIZE_TYPE__ size_t;
+
+typedef enum memory_order {
+  memory_order_relaxed = __ATOMIC_RELAXED,
+} memory_order;
+
+void *calloc(size_t, size_t);
+void free(void *);
+
+struct SomeData {
+  int i;
+  _Atomic int ref;
+};
+
+static struct SomeData *alloc_data(void)
+{
+  struct SomeData *data = calloc(sizeof(*data), 1);
+
+  __c11_atomic_store(&data->ref, 2, memory_order_relaxed);
+  return data;
+}
+
+static void put_data(struct SomeData *data)
+{
+ if (__c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1)
+   free(data);
+}
+
+static int dec_refcounter(struct SomeData *data)
+{
+  return __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1;
+}
+
+static void put_data_nested(struct SomeData *data)
+{
+  if (dec_refcounter(data))
+    free(data);
+}
+
+static void put_data_uncond(struct SomeData *data)
+{
+  free(data);
+}
+
+static void put_data_unrelated_atomic(struct SomeData *data)
+{
+  free(data);
+  __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed);
+}
+
+void test_no_uaf(void)
+{
+  struct SomeData *data = alloc_data();
+  put_data(data);
+  data->i += 1; // no warning
+}
+
+void test_no_uaf_nested(void)
+{
+  struct SomeData *data = alloc_data();
+  put_data_nested(data);
+  data->i += 1; // no warning
+}
+
+void test_uaf(void)
+{
+  struct SomeData *data = alloc_data();
+  put_data_uncond(data);
+  data->i += 1; // expected-warning{{Use of memory after it is freed}}
+}
+
+void test_no_uaf_atomic_after(void)
+{
+  struct SomeData *data = alloc_data();
+  put_data_unrelated_atomic(data);
+  data->i += 1; // expected-warning{{Use of memory after it is freed}}
+}