[analyzer] Suppress MallocChecker positives in destructors with atomics.

[NoQ]

Summary

This patch is a targeted suppression heuristic for false positives MallocChecker produces when a shared / reference-counting pointer is copied (including, but not limited to, llvm::IntrusiveRefCntPtr). The program increments the reference count through an atomic fetch_add (which is often the C++11 std::atomic<T>::fetch_add() that executes the respective C11 atomic when inlined), then decrements it via fetch_sub (or via fetch_add while adding -1), then sees if the reference count is zero by comparing the value returned by fetch_sub to 1, then deletes the object if the reference count is indeed zero.

These false positives get amplified by inlining temporary destructors, but even in code without any temporary destructors these positives are reproducible, as the tests suggest.

We cannot easily model the comparison to 1 correctly: even if we model all atomic expressions precisely, the original reference count may still have been symbolic to begin with. And if we tried to assume that no overflows occur on reference counts, this would still require pattern-matching heuristics to figure out if a certain variable is a reference counter.

The proposed fix is to suppress MallocChecker positives that are caused by releasing memory in a destructor stack frame (or its children stack frames) after performing any C11 atomic fetch_add or fetch_sub in that destructor's stack frame (or its children stack frames). This is done in a visitor suppression.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

527 ↗

(On Diff #135995)

Docstring.

2847 ↗

(On Diff #135995)

IMO would be slightly easier to read with logic reversed, e.g.

if (AE == dyn_cast<>(...))
   if (Op == add || Op == sub)
      for (...)
        if (...)
          return true
return false

But this is a preference and can be ignored.

2899 ↗

(On Diff #135995)

I'm not sure what is going on here.
We are just traversing the stack frame, finding the first destructor in the isReleased branch?

[NoQ]

Address comments.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2847 ↗

(On Diff #135995)

Ok. Just in case, this is usually regulated via https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code but here it seems fine.

2899 ↗

(On Diff #135995)

Do the updated comments make it more clear?

[george.karpenkov]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2899 ↗

(On Diff #135995)

I'm still confused :( you're going up the chain of frames until you see a destructor. How do you know that memory was released in a destructor at all, or that it was released in this particular destructor?

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2899 ↗

(On Diff #135995)

if (isReleased(RS, RSPrev, S)) says that the pointer against which the report is thrown was released in the current node.

[george.karpenkov]

OK I would still appreciate a comment on why you expect fetch_add and friends in the lowest destructor in the stack.

[NoQ]

Add one more comment.

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

protected:
     SymbolRef Sym;
 
     // The mode we are in, i.e. what kind of diagnostics will be emitted.
     NotificationMode Mode;
 
     // 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
+    // miscellaneous false positive suppression.
+    const StackFrameContext *ReleaseDestructorLC;
+
     bool IsLeak;
 
   public:
     MallocBugVisitor(SymbolRef S, bool isLeak = false)
-       : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr), IsLeak(isLeak) {}
+        : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
+          ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+
+    static void *getTag() {
+      static int Tag = 0;
+      return &Tag;
+    }
 
     void Profile(llvm::FoldingSetNodeID &ID) const override {
-      static int X = 0;
-      ID.AddPointer(&X);
+      ID.AddPointer(getTag());
       ID.AddPointer(Sym);
     }
 
     inline bool isAllocated(const RefState *S, const RefState *SPrev,
                             const Stmt *Stmt) {
       // Did not track -> allocated. Other state (released) -> allocated.
       return (Stmt && (isa<CallExpr>(Stmt) || isa<CXXNewExpr>(Stmt)) &&
               (S && (S->isAllocated() || S->isAllocatedOfSizeZero())) &&
static SymbolRef findFailedReallocSymbol(ProgramStateRef currState,
   }
 
   return nullptr;
 }
 
 std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
     const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
     BugReport &BR) {
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  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.
+  // 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
+  // through better shared pointer modeling.
+  if (ReleaseDestructorLC) {
+    if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
+      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);
+        }
+      }
+    }
+  }
+
   ProgramStateRef state = N->getState();
   ProgramStateRef statePrev = PrevN->getState();
 
   const RefState *RS = state->get<RegionState>(Sym);
   const RefState *RSPrev = statePrev->get<RegionState>(Sym);
   if (!RS)
     return nullptr;
 
-  const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
-    return nullptr;
-
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
 
   // Find out if this is an interesting point and what is the kind.
   const char *Msg = nullptr;
   StackHintGeneratorForSymbol *StackHint = nullptr;
   if (Mode == Normal) {
     if (isAllocated(RS, RSPrev, S)) {
       Msg = "Memory is allocated";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                                   "Returned allocated memory");
     } else if (isReleased(RS, RSPrev, S)) {
       Msg = "Memory is released";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                              "Returning; memory was released");
+
+      // See if we're releasing memory while inlining a destructor (or one of
+      // its callees). If so, enable the atomic-related suppression within that
+      // destructor (and all of its callees), which would kick in while visiting
+      // other nodes (the visit order is from the bug to the graph root).
+      for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+        if (isa<CXXDestructorDecl>(LC->getDecl())) {
+          assert(!ReleaseDestructorLC &&
+                 "There can be only one release point!");
+          ReleaseDestructorLC = LC->getCurrentStackFrame();
+          // 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.
+          break;
+        }
+      }
     } else if (isRelinquished(RS, RSPrev, S)) {
       Msg = "Memory ownership is transferred";
       StackHint = new StackHintGeneratorForSymbol(Sym, "");
     } else if (isReallocFailedCheck(RS, RSPrev, S)) {
       Mode = ReallocationFailed;
       Msg = "Reallocation failed";
       StackHint = new StackHintGeneratorForReallocationFailed(Sym,
                                                        "Reallocation failed");

cfe/trunk/test/Analysis/NewDelete-atomics.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef enum memory_order {
+  memory_order_relaxed = __ATOMIC_RELAXED,
+  memory_order_consume = __ATOMIC_CONSUME,
+  memory_order_acquire = __ATOMIC_ACQUIRE,
+  memory_order_release = __ATOMIC_RELEASE,
+  memory_order_acq_rel = __ATOMIC_ACQ_REL,
+  memory_order_seq_cst = __ATOMIC_SEQ_CST
+} memory_order;
+
+class Obj {
+  int RefCnt;
+
+public:
+  int incRef() {
+    return __c11_atomic_fetch_add((volatile _Atomic(int) *)&RefCnt, 1,
+                                  memory_order_relaxed);
+  }
+
+  int decRef() {
+    return __c11_atomic_fetch_sub((volatile _Atomic(int) *)&RefCnt, 1,
+                                  memory_order_relaxed);
+  }
+
+  void foo();
+};
+
+class IntrusivePtr {
+  Obj *Ptr;
+
+public:
+  IntrusivePtr(Obj *Ptr) : Ptr(Ptr) {
+    Ptr->incRef();
+  }
+
+  IntrusivePtr(const IntrusivePtr &Other) : Ptr(Other.Ptr) {
+    Ptr->incRef();
+  }
+
+  ~IntrusivePtr() {
+  // We should not take the path on which the object is deleted.
+    if (Ptr->decRef() == 1)
+      delete Ptr;
+  }
+
+  Obj *getPtr() const { return Ptr; }
+};
+
+void testDestroyLocalRefPtr() {
+  IntrusivePtr p1(new Obj());
+  {
+    IntrusivePtr p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtr(const IntrusivePtr &p1) {
+  {
+    IntrusivePtr p2(p1);
+  }
+
+  // p1 still maintains ownership. The object is not deleted.
+  p1.getPtr()->foo(); // no-warning
+}