From d832078904c6e1d26648236b9f724f699dafb201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= Date: Fri, 4 Feb 2022 15:40:26 +0100 Subject: [PATCH] [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators The problem with leak bug reports is that the most interesting event in the code is likely the one that did not happen -- lack of ownership change and lack of deallocation, which is often present within the same function that the analyzer inlined anyway, but not on the path of execution on which the bug occured. We struggle to understand that a function was responsible for freeing the memory, but failed. D105819 added a new visitor to improve memory leak bug reports. In addition to inspecting the ExplodedNodes of the bug pat, the visitor tries to guess whether the function was supposed to free memory, but failed to. Initially (in D108753), this was done by checking whether a CXXDeleteExpr is present in the function. If so, we assume that the function was at least party responsible, and prevent the analyzer from pruning bug report notes in it. This patch improves this heuristic by recognizing all deallocator functions that MallocChecker itself recognizes, by reusing MallocChecker::isFreeingCall. Differential Revision: https://reviews.llvm.org/D118880 --- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 76 ++++++++++++++----- clang/test/Analysis/NewDeleteLeaks.cpp | 58 +++++++++++++- 2 files changed, 115 insertions(+), 19 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 63194d69d636..238022d1253b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -398,6 +398,9 @@ private: }; bool isFreeingCall(const CallEvent &Call) const; + static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func); + + friend class NoOwnershipChangeVisitor; CallDescriptionMap AllocatingMemFnMap{ {{"alloca", 1}, &MallocChecker::checkAlloca}, @@ -742,7 +745,9 @@ private: namespace { class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { + // The symbol whose (lack of) ownership change we are interested in. SymbolRef Sym; + const MallocChecker &Checker; using OwnerSet = llvm::SmallPtrSet; // Collect which entities point to the allocated memory, and could be @@ -794,6 +799,29 @@ protected: return ""; } + /// Syntactically checks whether the callee is a deallocating function. Since + /// we have no path-sensitive information on this call (we would need a + /// CallEvent instead of a CallExpr for that), its possible that a + /// deallocation function was called indirectly through a function pointer, + /// but we are not able to tell, so this is a best effort analysis. + /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in + /// clang/test/Analysis/NewDeleteLeaks.cpp. + bool isFreeingCallAsWritten(const CallExpr &Call) const { + if (Checker.FreeingMemFnMap.lookupAsWritten(Call) || + Checker.ReallocatingMemFnMap.lookupAsWritten(Call)) + return true; + + if (const auto *Func = + llvm::dyn_cast_or_null(Call.getCalleeDecl())) + return MallocChecker::isFreeingOwnershipAttrCall(Func); + + return false; + } + + /// Heuristically guess whether the callee intended to free memory. This is + /// done syntactically, because we are trying to argue about alternative + /// paths of execution, and as a consequence we don't have path-sensitive + /// information. bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) { using namespace clang::ast_matchers; const FunctionDecl *FD = dyn_cast(Callee); @@ -807,13 +835,21 @@ protected: if (!FD || !FD->hasBody()) 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(); + auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"), + callExpr().bind("call")))), + *FD->getBody(), ACtx); + for (BoundNodes Match : Matches) { + if (Match.getNodeAs("delete")) + return true; + + if (const auto *Call = Match.getNodeAs("call")) + if (isFreeingCallAsWritten(*Call)) + return true; + } + // TODO: Ownership might change with an attempt to store the allocated + // memory, not only through deallocation. Check for attempted stores as + // well. + return false; } virtual bool @@ -882,9 +918,9 @@ protected: } public: - NoOwnershipChangeVisitor(SymbolRef Sym) - : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), - Sym(Sym) {} + NoOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker) + : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym), + Checker(*Checker) {} void Profile(llvm::FoldingSetNodeID &ID) const override { static int Tag = 0; @@ -1069,12 +1105,8 @@ static bool isStandardNewDelete(const FunctionDecl *FD) { // Methods of MallocChecker and MallocBugVisitor. //===----------------------------------------------------------------------===// -bool MallocChecker::isFreeingCall(const CallEvent &Call) const { - if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) - return true; - - const auto *Func = dyn_cast(Call.getDecl()); - if (Func && Func->hasAttrs()) { +bool MallocChecker::isFreeingOwnershipAttrCall(const FunctionDecl *Func) { + if (Func->hasAttrs()) { for (const auto *I : Func->specific_attrs()) { OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind(); if (OwnKind == OwnershipAttr::Takes || OwnKind == OwnershipAttr::Holds) @@ -1084,6 +1116,16 @@ bool MallocChecker::isFreeingCall(const CallEvent &Call) const { return false; } +bool MallocChecker::isFreeingCall(const CallEvent &Call) const { + if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) + return true; + + if (const auto *Func = dyn_cast_or_null(Call.getDecl())) + return isFreeingOwnershipAttrCall(Func); + + return false; +} + bool MallocChecker::isMemCall(const CallEvent &Call) const { if (FreeingMemFnMap.lookup(Call) || AllocatingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) @@ -2756,7 +2798,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N, R->markInteresting(Sym); R->addVisitor(Sym, true); if (ShouldRegisterNoOwnershipChangeVisitor) - R->addVisitor(Sym); + R->addVisitor(Sym, this); C.emitReport(std::move(R)); } diff --git a/clang/test/Analysis/NewDeleteLeaks.cpp b/clang/test/Analysis/NewDeleteLeaks.cpp index 57c7e57c98e3..19a6aff7833b 100644 --- a/clang/test/Analysis/NewDeleteLeaks.cpp +++ b/clang/test/Analysis/NewDeleteLeaks.cpp @@ -35,7 +35,9 @@ void foo() { } // namespace memory_allocated_in_fn_call -namespace memory_passed_to_fn_call { +// Realize that sink() intends to deallocate memory, assume that it should've +// taken care of the leaked object as well. +namespace memory_passed_to_fn_call_delete { void sink(int *P) { if (coin()) // ownership-note {{Assuming the condition is false}} @@ -50,7 +52,41 @@ void foo() { } // 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_passed_to_fn_call_delete + +namespace memory_passed_to_fn_call_free { + +void sink(int *P) { + if (coin()) // ownership-note {{Assuming the condition is false}} + // ownership-note@-1 {{Taking false branch}} + free(P); +} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}} + +void foo() { + int *ptr = (int *)malloc(sizeof(int)); // 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' [unix.Malloc]}} +// expected-note@-1 {{Potential leak}} + +} // namespace memory_passed_to_fn_call_free + +// Function pointers cannot be resolved syntactically. +namespace memory_passed_to_fn_call_free_through_fn_ptr { +void (*freeFn)(void *) = free; + +void sink(int *P) { + if (coin()) + freeFn(P); +} + +void foo() { + int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}} + sink(ptr); +} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}} +// expected-note@-1 {{Potential leak}} + +} // namespace memory_passed_to_fn_call_free_through_fn_ptr namespace memory_shared_with_ptr_of_shorter_lifetime { @@ -123,6 +159,24 @@ void foo() { } // namespace memory_passed_into_fn_that_doesnt_intend_to_free +namespace memory_passed_into_fn_that_doesnt_intend_to_free2 { + +void bar(); + +void sink(int *P) { + // Correctly realize that calling bar() doesn't mean that this function would + // like to deallocate anything. + bar(); +} + +void foo() { + int *ptr = new int(5); // expected-note {{Memory is allocated}} + 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_free2 + namespace refkind_from_unoallocated_to_allocated { // RefKind of the symbol changed from nothing to Allocated. We don't want to -- GitLab