[analyzer] MallocChecker: Suppress false positives in shared pointers.

Throw away MallocChecker warnings that occur after releasing a pointer within a
destructor (or its callees) after performing C11 atomic fetch_add or fetch_sub
within that destructor (or its callees).

This is an indication that the destructor's class is likely a
reference-counting pointer. The analyzer is not able to understand that the
original reference count is usually large enough to avoid most use-after-frees.

Even when the smart pointer is a local variable, we still have these false
positives that this patch suppresses, because the analyzer doesn't currently
support atomics well enough.

Differential Revision: https://reviews.llvm.org/D43791

llvm-svn: 326249

1 files changed, 56 insertions, 7 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 378728dc613..7b644d4dadf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -446,15 +446,24 @@ private:
     // 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);
     }
 
@@ -2822,6 +2831,32 @@ static SymbolRef findFailedReallocSymbol(ProgramStateRef currState,
 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();
 
@@ -2830,10 +2865,6 @@ std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
   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))).
 
@@ -2849,6 +2880,24 @@ std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
       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, "");