[analyzer] MallocChecker: Add a visitor to leave a note on functions that could have, but did not change ownership on leaked memory

This is a rather common feedback we get from out leak checkers: bug reports are
really short, and are contain barely any usable information on what the analyzer
did to conclude that a leak actually happened.

This happens because of our bug report minimizing effort. We construct bug
reports by inspecting the ExplodedNodes that lead to the error from the bottom
up (from the error node all the way to the root of the exploded graph), and mark
entities that were the cause of a bug, or have interacted with it as
interesting. In order to make the bug report a bit less verbose, whenever we
find an entire function call (from CallEnter to CallExitEnd) that didn't talk
about any interesting entity, we prune it (click here for more info on bug
report generation). Even if the event to highlight is exactly this lack of
interaction with interesting entities.

D105553 generalized the visitor that creates notes for these cases. This patch
adds a new kind of NoStateChangeVisitor that leaves notes in functions that
took a piece of dynamically allocated memory that later leaked as parameter,
and didn't change its ownership status.

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

Author: Szelethus

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -485,7 +485,17 @@ def DynamicMemoryModeling: Checker<"DynamicMemoryModeling">,
                   "allocating and deallocating functions are annotated with "
                   "ownership_holds, ownership_takes and ownership_returns.",
                   "false",
-                  InAlpha>
+                  InAlpha>,
+    CmdLineOption<Boolean,
+                  "AddNoOwnershipChangeNotes",
+                  "Add an additional note to the bug report for leak-like "
+                  "bugs. Dynamically allocated objects passed to functions "
+                  "that neither deallocated it, or have taken responsibility "
+                  "of the ownership are noted, similarly to "
+                  "NoStoreFuncVisitor.",
+                  "false",
+                  InAlpha,
+                  Hide>
   ]>,
   Dependencies<[CStringModeling]>,
   Documentation<NotDocumented>,

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -48,6 +48,7 @@
 #include "InterCheckerAPI.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
@@ -64,12 +65,15 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Compiler.h"
@@ -298,6 +302,8 @@ class MallocChecker
   /// which might free a pointer are annotated.
   DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
 
+  DefaultBool ShouldRegisterNoOwnershipChangeVisitor;
+
   /// Many checkers are essentially built into this one, so enabling them will
   /// make MallocChecker perform additional modeling and reporting.
   enum CheckKind {
@@ -722,11 +728,146 @@ class MallocChecker
   bool isArgZERO_SIZE_PTR(ProgramStateRef State, CheckerContext &C,
                           SVal ArgVal) const;
 };
+} // end anonymous namespace
+
+//===----------------------------------------------------------------------===//
+// Definition of NoOwnershipChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+  SymbolRef Sym;
+  using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
+
+  // Collect which entities point to the allocated memory, and could be
+  // responsible for deallocating it.
+  class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
+    SymbolRef Sym;
+    OwnerSet &Owners;
+
+  public:
+    OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
+        : Sym(Sym), Owners(Owners) {}
+
+    bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
+                       SVal Val) override {
+      if (Val.getAsSymbol() == Sym)
+        Owners.insert(Region);
+      return true;
+    }
+  };
+
+protected:
+  OwnerSet getOwnersAtNode(const ExplodedNode *N) {
+    OwnerSet Ret;
+
+    ProgramStateRef State = N->getState();
+    OwnershipBindingsHandler Handler{Sym, Ret};
+    State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+                                                            Handler);
+    return Ret;
+  }
+
+  static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+    while (N && !N->getLocationAs<CallExitEnd>())
+      N = N->getFirstSucc();
+    return N;
+  }
+
+  virtual bool
+  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                            const ExplodedNode *CallExitN) override {
+    if (CurrN->getLocationAs<CallEnter>())
+      return true;
+
+    // Its the state right *after* the call that is interesting. Any pointers
+    // inside the call that pointed to the allocated memory are of little
+    // consequence if their lifetime ends within the function.
+    CallExitN = getCallExitEnd(CallExitN);
+    if (!CallExitN)
+      return true;
+
+    if (CurrN->getState()->get<RegionState>(Sym) !=
+        CallExitN->getState()->get<RegionState>(Sym))
+      return true;
+
+    OwnerSet CurrOwners = getOwnersAtNode(CurrN);
+    OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+
+    // Owners in the current set may be purged from the analyzer later on.
+    // If a variable is dead (is not referenced directly or indirectly after
+    // some point), it will be removed from the Store before the end of its
+    // actual lifetime.
+    // This means that that if the ownership status didn't change, CurrOwners
+    // must be a superset of, but not necessarily equal to ExitOwners.
+    return !llvm::set_is_subset(ExitOwners, CurrOwners);
+  }
+
+  static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) {
+    PathDiagnosticLocation L = PathDiagnosticLocation::create(
+        N->getLocation(),
+        N->getState()->getStateManager().getContext().getSourceManager());
+    return std::make_shared<PathDiagnosticEventPiece>(
+        L, "Returning without deallocating memory or storing the pointer for "
+           "later deallocation");
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) override {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) override {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) override {
+    // TODO: Factor the logic of "what constitutes as an entity being passed
+    // into a function call" out by reusing the code in
+    // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
+    // the printing technology in UninitializedObject's FieldChainInfo.
+    ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+    for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+      SVal V = Call.getArgSVal(I);
+      if (V.getAsSymbol() == Sym)
+        return emitNote(N);
+    }
+    return nullptr;
+  }
+
+public:
+  NoOwnershipChangeVisitor(SymbolRef Sym)
+      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough),
+        Sym(Sym) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+    ID.AddPointer(Sym);
+  }
+
+  void *getTag() const {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+};
+
+} // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Definition of MallocBugVisitor.
 //===----------------------------------------------------------------------===//
 
+namespace {
 /// The bug visitor which allows us to print extra diagnostics along the
 /// BugReport path. For example, showing the allocation site of the leaked
 /// region.
@@ -851,7 +992,6 @@ class MallocBugVisitor final : public BugReporterVisitor {
     }
   };
 };
-
 } // end anonymous namespace
 
 // A map from the freed symbol to the symbol representing the return value of
@@ -2579,6 +2719,8 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
       AllocNode->getLocationContext()->getDecl());
   R->markInteresting(Sym);
   R->addVisitor<MallocBugVisitor>(Sym, true);
+  if (ShouldRegisterNoOwnershipChangeVisitor)
+    R->addVisitor<NoOwnershipChangeVisitor>(Sym);
   C.emitReport(std::move(R));
 }
 
@@ -3395,6 +3537,9 @@ void ento::registerDynamicMemoryModeling(CheckerManager &mgr) {
   auto *checker = mgr.registerChecker<MallocChecker>();
   checker->ShouldIncludeOwnershipAnnotatedFunctions =
       mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "Optimistic");
+  checker->ShouldRegisterNoOwnershipChangeVisitor =
+      mgr.getAnalyzerOptions().getCheckerBooleanOption(
+          checker, "AddNoOwnershipChangeNotes");
 }
 
 bool ento::shouldRegisterDynamicMemoryModeling(const CheckerManager &mgr) {

clang/test/Analysis/NewDeleteLeaks.cpp

@@ -0,0 +1,142 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-output=text %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-config \
+// RUN:     unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=false
+
+// RUN: %clang_analyze_cc1 -verify=expected,ownership -analyzer-output=text %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-config \
+// RUN:     unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=true
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+//===----------------------------------------------------------------------===//
+// Report for which we expect NoOwnershipChangeVisitor to add a new note.
+//===----------------------------------------------------------------------===//
+
+bool coin();
+
+namespace memory_allocated_in_fn_call {
+
+void sink(int *P) {
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  sink(new int(5)); // expected-note {{Memory is allocated}}
+                    // ownership-note@-1 {{Calling 'sink'}}
+                    // ownership-note@-2 {{Returning from 'sink'}}
+} // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential memory leak}}
+
+} // namespace memory_allocated_in_fn_call
+
+namespace memory_passed_to_fn_call {
+
+void sink(int *P) {
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+              // ownership-note@-1 {{Taking false branch}}
+    delete P;
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // ownership-note {{Calling 'sink'}}
+                         // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_shorter_lifetime {
+
+void sink(int *P) {
+  int *Q = P;
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+              // ownership-note@-1 {{Taking false branch}}
+    delete P;
+  (void)Q;
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // ownership-note {{Calling 'sink'}}
+                         // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_shorter_lifetime
+
+//===----------------------------------------------------------------------===//
+// Report for which we *do not* expect NoOwnershipChangeVisitor add a new note,
+// nor do we want it to.
+//===----------------------------------------------------------------------===//
+
+namespace memory_not_passed_to_fn_call {
+
+void sink(int *P) {
+  if (coin())
+    delete P;
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  int *q = nullptr;
+  sink(q);
+  (void)ptr;
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_not_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_same_lifetime {
+
+void sink(int *P, int **Q) {
+  // NOTE: Not a job of NoOwnershipChangeVisitor, but maybe this could be
+  // highlighted still?
+  *Q = P;
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  int *q = nullptr;
+  sink(ptr, &q);
+} // expected-warning {{Potential leak of memory pointed to by 'q' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_same_lifetime
+
+// TODO: We don't want a note here. sink() doesn't seem like a function that
+// even attempts to take care of any memory ownership problems.
+namespace memory_passed_into_fn_that_doesnt_intend_to_free {
+
+void sink(int *P) {
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // ownership-note {{Calling 'sink'}}
+                         // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free
+
+namespace refkind_from_unoallocated_to_allocated {
+
+// RefKind of the symbol changed from nothing to Allocated. We don't want to
+// emit notes when the RefKind changes in the stack frame.
+static char *malloc_wrapper_ret() {
+  return (char *)malloc(12); // expected-note {{Memory is allocated}}
+}
+void use_ret() {
+  char *v;
+  v = malloc_wrapper_ret(); // expected-note {{Calling 'malloc_wrapper_ret'}}
+                            // expected-note@-1 {{Returned allocated memory}}
+} // expected-warning {{Potential leak of memory pointed to by 'v' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak of memory pointed to by 'v'}}
+
+} // namespace refkind_from_unoallocated_to_allocated

clang/test/Analysis/analyzer-config.c

@@ -116,6 +116,7 @@
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false