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

[Artem Dergachev via cfe-commits]

https://github.com/haoNoQ created https://github.com/llvm/llvm-project/pull/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.

>From 6f7c0e33d10d6b3dec2a96a8cdc132eff71463fc Mon Sep 17 00:00:00 2001
From: Artem Dergachev <adergachev at apple.com>
Date: Thu, 2 May 2024 15:15:27 -0700
Subject: [PATCH] [analyzer] MallocChecker: Recognize std::atomics in smart
 pointer suppression.

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.
---
 .../StaticAnalyzer/Checkers/MallocChecker.cpp |  19 ++-
 .../Inputs/system-header-simulator-cxx.h      |   7 ++
 clang/test/Analysis/NewDelete-atomics.cpp     | 116 ++++++++++++++++--
 3 files changed, 130 insertions(+), 12 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 11651fd491f743..3d89d1e6343490 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3447,7 +3447,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;
       }
     }
@@ -3479,13 +3479,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 1c2be322f83c20..29326ec1f9280d 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 54fce17ea7bd22..1425acab7489ba 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.