[clang] 51f178d - [analyzer] MallocChecker: Recognize std::atomics in smart pointer suppression. (#90918)

[via cfe-commits]

Author: Artem Dergachev
Date: 2024-05-08T18:00:59-07:00
New Revision: 51f178d909d477bd269e0b434af1a7f9373d4e61

URL: https://github.com/llvm/llvm-project/commit/51f178d909d477bd269e0b434af1a7f9373d4e61
DIFF: https://github.com/llvm/llvm-project/commit/51f178d909d477bd269e0b434af1a7f9373d4e61.diff

LOG: [analyzer] MallocChecker: Recognize std::atomics in smart pointer suppression. (#90918)

Fixes #90498.

Same as 5337efc69cdd5 for atomic builtins, but for `std::atomic` this
time. This is useful because even though the actual builtin atomic is
still there, it may be buried beyond the inlining depth limit.

Also add one popular custom smart pointer class name to the name-based
heuristics, which isn't necessary to fix the bug but arguably a good
idea regardless.

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    clang/test/Analysis/Inputs/system-header-simulator-cxx.h
    clang/test/Analysis/NewDelete-atomics.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index dd204b62dcc04..ab89fb14046be 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3451,7 +3451,7 @@ static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) {
     if (N.contains_insensitive("ptr") || N.contains_insensitive("pointer")) {
       if (N.contains_insensitive("ref") || N.contains_insensitive("cnt") ||
           N.contains_insensitive("intrusive") ||
-          N.contains_insensitive("shared")) {
+          N.contains_insensitive("shared") || N.ends_with_insensitive("rc")) {
         return true;
       }
     }
@@ -3483,13 +3483,24 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
   // original reference count is positive, we should not report use-after-frees
   // on objects deleted in such destructors. This can probably be improved
   // through better shared pointer modeling.
-  if (ReleaseDestructorLC) {
+  if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
+                              ReleaseDestructorLC->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) {
-        if (ReleaseDestructorLC == CurrentLC ||
-            ReleaseDestructorLC->isParentOf(CurrentLC)) {
+        BR.markInvalid(getTag(), S);
+      }
+    } else if (const auto *CE = dyn_cast<CallExpr>(S)) {
+      // Check for `std::atomic` and such. This covers both regular method calls
+      // and operator calls.
+      if (const auto *MD =
+              dyn_cast_or_null<CXXMethodDecl>(CE->getDirectCallee())) {
+        const CXXRecordDecl *RD = MD->getParent();
+        // A bit wobbly with ".contains()" because it may be like
+        // "__atomic_base" or something.
+        if (StringRef(RD->getNameAsString()).contains("atomic")) {
           BR.markInvalid(getTag(), S);
         }
       }

diff  --git a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
index 1c2be322f83c2..29326ec1f9280 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1260,6 +1260,13 @@ template<
     iterator end() const { return iterator(val + 1); }
 };
 
+template <typename T>
+class atomic {
+public:
+  T operator++();
+  T operator--();
+};
+
 namespace execution {
 class sequenced_policy {};
 }

diff  --git a/clang/test/Analysis/NewDelete-atomics.cpp b/clang/test/Analysis/NewDelete-atomics.cpp
index 54fce17ea7bd2..1425acab7489b 100644
--- a/clang/test/Analysis/NewDelete-atomics.cpp
+++ b/clang/test/Analysis/NewDelete-atomics.cpp
@@ -20,7 +20,7 @@ typedef enum memory_order {
   memory_order_seq_cst = __ATOMIC_SEQ_CST
 } memory_order;
 
-class Obj {
+class RawObj {
   int RefCnt;
 
 public:
@@ -37,11 +37,27 @@ class Obj {
   void foo();
 };
 
+class StdAtomicObj {
+  std::atomic<int> RefCnt;
+
+public:
+  int incRef() {
+    return ++RefCnt;
+  }
+
+  int decRef() {
+    return --RefCnt;
+  }
+
+  void foo();
+};
+
+template <typename T>
 class IntrusivePtr {
-  Obj *Ptr;
+  T *Ptr;
 
 public:
-  IntrusivePtr(Obj *Ptr) : Ptr(Ptr) {
+  IntrusivePtr(T *Ptr) : Ptr(Ptr) {
     Ptr->incRef();
   }
 
@@ -55,22 +71,106 @@ class IntrusivePtr {
       delete Ptr;
   }
 
-  Obj *getPtr() const { return Ptr; } // no-warning
+  T *getPtr() const { return Ptr; } // no-warning
+};
+
+// Also IntrusivePtr but let's dodge name-based heuristics.
+template <typename T>
+class DifferentlyNamed {
+  T *Ptr;
+
+public:
+  DifferentlyNamed(T *Ptr) : Ptr(Ptr) {
+    Ptr->incRef();
+  }
+
+  DifferentlyNamed(const DifferentlyNamed &Other) : Ptr(Other.Ptr) {
+    Ptr->incRef();
+  }
+
+  ~DifferentlyNamed() {
+  // We should not take the path on which the object is deleted.
+    if (Ptr->decRef() == 1)
+      delete Ptr;
+  }
+
+  T *getPtr() const { return Ptr; } // no-warning
 };
 
 void testDestroyLocalRefPtr() {
-  IntrusivePtr p1(new Obj());
+  IntrusivePtr<RawObj> p1(new RawObj());
+  {
+    IntrusivePtr<RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtr(const IntrusivePtr<RawObj> &p1) {
+  {
+    IntrusivePtr<RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroyLocalRefPtrWithAtomics() {
+  IntrusivePtr<StdAtomicObj> p1(new StdAtomicObj());
+  {
+    IntrusivePtr<StdAtomicObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+
+void testDestroyLocalRefPtrWithAtomics(const IntrusivePtr<StdAtomicObj> &p1) {
   {
-    IntrusivePtr p2(p1);
+    IntrusivePtr<StdAtomicObj> p2(p1);
   }
 
   // p1 still maintains ownership. The object is not deleted.
   p1.getPtr()->foo(); // no-warning
 }
 
-void testDestroySymbolicRefPtr(const IntrusivePtr &p1) {
+void testDestroyLocalRefPtrDifferentlyNamed() {
+  DifferentlyNamed<RawObj> p1(new RawObj());
+  {
+    DifferentlyNamed<RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtrDifferentlyNamed(
+    const DifferentlyNamed<RawObj> &p1) {
+  {
+    DifferentlyNamed<RawObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed() {
+  DifferentlyNamed<StdAtomicObj> p1(new StdAtomicObj());
+  {
+    DifferentlyNamed<StdAtomicObj> p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+
+void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed(
+    const DifferentlyNamed<StdAtomicObj> &p1) {
   {
-    IntrusivePtr p2(p1);
+    DifferentlyNamed<StdAtomicObj> p2(p1);
   }
 
   // p1 still maintains ownership. The object is not deleted.