[analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Szelethus]

Summary

D105819 Added NoOwnershipChangeVisitor, but it is only registered when an off-by-default, hidden checker option was enabled. The reason behind this was that it grossly overestimated the set of functions that really needed a note:

std::string getTrainName(const Train *T) {
  return T->name;
} // note: Retuning without changing the ownership of or deallocating memory
// Umm... I mean duh? Nor would I expect this function to do anything like that...

void foo() {
  Train *T = new Train("Land Plane");
  print(getTrainName(T)); // note: calling getTrainName / returning from getTrainName
} // warn: Memory leak

This patch adds a heuristic that guesses that any function that has an explicit operator delete call could have be responsible for deallocating the memory that ended up leaking. This is waaaay too conservative (see the TODOs in the new function), but it safer to err on the side of too little than too much, and would allow us to enable the option by default *now*, and add refinements one-by-one.

[martong]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

802

Sounds like a good idea to reuse isFreeingCall.

But interestingly delete is not listed there! Nor new in AllocatingMemFnMap. So, perhaps we need yet another abstraction. Maybe a function that iterates over FreeingMemFnMap collects the functions and then adds delete as well?

806

Good point!

clang/test/Analysis/NewDeleteLeaks.cpp

23

intent

BTW, why is this TODO here? It is obvious that sink does not have a delete, so we don't expect any warning. Or am I missing something?

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

282–289

These serve to identify new/delete operators. I've done a lot of work to make this better, but I don't remember why isFreeingCall doesn't call these.

clang/test/Analysis/NewDeleteLeaks.cpp

23

Well, its debatable, I suppose, but in this case, sink is noteworthy, as no other function had the ability to take care of this memory.

https://lists.llvm.org/pipermail/cfe-dev/2021-June/068469.html

Kristof's case is interesting in a different manner. If taken literally, it doesn't satisfy our criteria of "there exists another execution path on which it did actually do ${Something}". We probably shouldn't emit a note every time the function simply accepts the value of interest by pointer, because this doesn't necessarily prove the intent to delete the pointer; there are a million other reasons to accept a pointer. However, Kristof's case does still deserve a note because sink() is the *only* function that had a chance to delete the pointer.

[Szelethus]

indent->intent

[martong]

Okay, LGTM! Thanks!

[NoQ]

This looks good!

I guess one way to make this even more conservative would be to match the variable inside the delete-expression to the one we expect to get deallocated.

[Szelethus]

In D108753#2988632, @NoQ wrote:

This looks good!

I guess one way to make this even more conservative would be to match the variable inside the delete-expression to the one we expect to get deallocated.

Yeah, I thought about that, but one counter argument to that is that once an intent to do some deallocation is already there, its possible that the wrong variable was deleted, or the deallocation wasn't thorough enough.

Thanks for the reviews!

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

CmdLineOption<Boolean,
                   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,
+                  "true",
+                  Released,
                   Hide>
   ]>,
   Dependencies<[CStringModeling]>,
   Documentation<NotDocumented>,
   Hidden;
 
 def MallocChecker: Checker<"Malloc">,
   HelpText<"Check for memory leaks, double free, and use-after-free problems. "

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 #include "AllocationState.h"
 #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"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
return ReallocatedSym == X.ReallocatedSym &&
            Kind == X.Kind;
   }
 };
 
 } // end of anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
 static bool isStandardNewDelete(const CallEvent &Call) {
   if (!Call.getDecl() || !isa<FunctionDecl>(Call.getDecl()))
     return false;
   return isStandardNewDelete(cast<FunctionDecl>(Call.getDecl()));
 }
 
 //===----------------------------------------------------------------------===//
 // Definition of the MallocChecker class.
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 class MallocChecker
protected:
   getFunctionName(const ExplodedNode *CallEnterN) {
     if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
             CallEnterN->getLocationAs<CallEnter>()->getCallExpr()))
       if (const FunctionDecl *FD = CE->getDirectCallee())
         return FD->getQualifiedNameAsString();
     return "";
   }
 
+  bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
+    using namespace clang::ast_matchers;
+    const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+    if (!FD)
+      return false;
+    // TODO: Operator delete is hardly the only deallocator -- Can we reuse
+    // isFreeingCall() or something thats already here?
+    auto Deallocations = match(
+        stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
+             ), *FD->getBody(), ACtx);
+    // TODO: Ownership my change with an attempt to store the allocated memory.
+    return !Deallocations.empty();
+  }
+
   virtual bool
   wasModifiedInFunction(const ExplodedNode *CallEnterN,
                         const ExplodedNode *CallExitEndN) override {
+    if (!doesFnIntendToHandleOwnership(
+            CallExitEndN->getFirstPred()->getLocationContext()->getDecl(),
+            CallExitEndN->getState()->getAnalysisManager().getASTContext()))
+      return true;
+
     if (CallEnterN->getState()->get<RegionState>(Sym) !=
         CallExitEndN->getState()->get<RegionState>(Sym))
       return true;
 
     OwnerSet CurrOwners = getOwnersAtNode(CallEnterN);
     OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN);
 
     // Owners in the current set may be purged from the analyzer later on.

clang/test/Analysis/NewDeleteLeaks.cpp

 #include "Inputs/system-header-simulator-for-malloc.h"
 
 //===----------------------------------------------------------------------===//
 // Report for which we expect NoOwnershipChangeVisitor to add a new note.
 //===----------------------------------------------------------------------===//
 
 bool coin();
 
+// TODO: AST analysis of sink would reveal that it doesn't intent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
 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) {
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'}}
+  sink(ptr);
 } // 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

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: silence-checkers = ""
 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // 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:AddNoOwnershipChangeNotes = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: verbose-report-filename = false
 // CHECK-NEXT: widen-loops = false