Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ 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" @@ -722,11 +726,146 @@ 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; + + // 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()) + N = N->getFirstSucc(); + return N; + } + + virtual bool + wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitN) override { + if (CurrN->getLocationAs()) + 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(Sym) != + CallExitN->getState()->get(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( + 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 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(&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 +990,6 @@ } }; }; - } // end anonymous namespace // A map from the freed symbol to the symbol representing the return value of @@ -2579,6 +2717,7 @@ AllocNode->getLocationContext()->getDecl()); R->markInteresting(Sym); R->addVisitor(Sym, true); + R->addVisitor(Sym); C.emitReport(std::move(R)); } Index: clang/test/Analysis/NewDeleteLeaks.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/NewDeleteLeaks.cpp @@ -0,0 +1,130 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify -analyzer-output=text %s + +#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) { +} // expected-note {{Returning without deallocating memory or storing the pointer for later deallocation}} + +void foo() { + sink(new int(5)); // expected-note {{Memory is allocated}} + // expected-note@-1 {{Calling 'sink'}} + // expected-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()) // expected-note {{Assuming the condition is false}} + // expected-note@-1 {{Taking false branch}} + delete P; +} // expected-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); // expected-note {{Calling 'sink'}} + // expected-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()) // expected-note {{Assuming the condition is false}} + // expected-note@-1 {{Taking false branch}} + delete P; + (void)Q; +} // expected-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); // expected-note {{Calling 'sink'}} + // expected-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) { +} // expected-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); // expected-note {{Calling 'sink'}} + // expected-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 Index: clang/test/Analysis/self-assign.cpp =================================================================== --- clang/test/Analysis/self-assign.cpp +++ clang/test/Analysis/self-assign.cpp @@ -1,4 +1,9 @@ -// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection -analyzer-config eagerly-assume=false %s -verify -analyzer-output=text +// RUN: %clang_analyze_cc1 -std=c++11 %s -verify -analyzer-output=text \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=cplusplus \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false extern "C" char *strdup(const char* s); extern "C" void free(void* ptr); @@ -28,18 +33,31 @@ free(str); } -StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} - clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} +StringUsed &StringUsed::operator=(const StringUsed &rhs) { + // expected-note@-1{{Assuming rhs == *this}} + // expected-note@-2{{Assuming rhs == *this}} + // expected-note@-3{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} + // expected-warning@-1{{UNKNOWN}} + // expected-note@-2{{TRUE}} + // expected-note@-3{{UNKNOWN}} free(str); // expected-note{{Memory is released}} - str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}} -// expected-note@-1{{Memory is allocated}} + str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} + // expected-note@-1{{Use of memory after it is freed}} + // expected-note@-2{{Memory is allocated}} return *this; } -StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} - clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} +StringUsed &StringUsed::operator=(StringUsed &&rhs) { + // expected-note@-1{{Assuming rhs == *this}} + // expected-note@-2{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} + // expected-warning@-1{{UNKNOWN}} + // expected-note@-2{{TRUE}} + // expected-note@-3{{UNKNOWN}} str = rhs.str; - rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}} + rhs.str = nullptr; // expected-warning{{Potential memory leak}} + // expected-note@-1{{Potential memory leak}} return *this; } @@ -63,15 +81,27 @@ free(str); } -StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} - clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} +StringUnused &StringUnused::operator=(const StringUnused &rhs) { + // expected-note@-1{{Assuming rhs == *this}} + // expected-note@-2{{Assuming rhs == *this}} + // expected-note@-3{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} + // expected-warning@-1{{UNKNOWN}} + // expected-note@-2{{TRUE}} + // expected-note@-3{{UNKNOWN}} free(str); // expected-note{{Memory is released}} - str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}} + str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} + // expected-note@-1{{Use of memory after it is freed}} return *this; } -StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} - clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} +StringUnused &StringUnused::operator=(StringUnused &&rhs) { + // expected-note@-1{{Assuming rhs == *this}} + // expected-note@-2{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} + // expected-warning@-1{{UNKNOWN}} + // expected-note@-2{{TRUE}} + // expected-note@-3{{UNKNOWN}} str = rhs.str; rhs.str = nullptr; // FIXME: An improved leak checker should warn here return *this; @@ -84,7 +114,8 @@ int main() { StringUsed s1 ("test"), s2; - s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}} + s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} + // expected-note@-1{{Returned allocated memory}} s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}} return 0; }