diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -398,6 +398,9 @@ }; 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 @@ 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 @@ 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 @@ 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 @@ } 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 @@ // 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 @@ 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 @@ 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 --- a/clang/test/Analysis/NewDeleteLeaks.cpp +++ b/clang/test/Analysis/NewDeleteLeaks.cpp @@ -35,7 +35,9 @@ } // 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 @@ } // 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 @@ } // 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