diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index efb980962e811..1c45ecaadcc56 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -231,12 +231,15 @@ class RefState { LLVM_DUMP_METHOD void dump(raw_ostream &OS) const { switch (K) { -#define CASE(ID) case ID: OS << #ID; break; - CASE(Allocated) - CASE(AllocatedOfSizeZero) - CASE(Released) - CASE(Relinquished) - CASE(Escaped) +#define CASE(ID) \ + case ID: \ + OS << #ID; \ + break; + CASE(Allocated) + CASE(AllocatedOfSizeZero) + CASE(Released) + CASE(Relinquished) + CASE(Escaped) } } @@ -414,7 +417,7 @@ class MallocChecker // This last frontend is associated with a single bug type which is not used // elsewhere and has a different bug category, so it's declared separately. CheckerFrontendWithBugType TaintedAllocChecker{"Tainted Memory Allocation", - categories::TaintedData}; + categories::TaintedData}; using LeakInfo = std::pair; @@ -430,7 +433,7 @@ class MallocChecker ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond, bool Assumption) const; void checkLocation(SVal l, bool isLoad, const Stmt *S, - CheckerContext &C) const; + CheckerContext &C) const; ProgramStateRef checkPointerEscape(ProgramStateRef State, const InvalidatedSymbols &Escaped, @@ -3012,7 +3015,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N, } void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper, - CheckerContext &C) const + CheckerContext &C) const { ProgramStateRef state = C.getState(); RegionStateTy OldRS = state->get(); @@ -3074,6 +3077,43 @@ void MallocChecker::checkPostCall(const CallEvent &Call, (*PostFN)(this, C.getState(), Call, C); return; } + + ProgramStateRef State = C.getState(); + + if (const auto *Ctor = dyn_cast(&Call)) { + // Ensure we are constructing a concrete object/subobject. + if (const MemRegion *ObjUnderConstr = Ctor->getCXXThisVal().getAsRegion()) { + ProgramStateRef NewState = State; + + for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) { + SVal ArgV = Call.getArgSVal(I); + + SymbolRef Sym = ArgV.getAsSymbol(); + if (!Sym) + continue; + + // Look up current ref-state for this symbol in the RegionState map. + if (const RefState *RS = State->get(Sym)) { + // Only re-label symbols that are still owned allocations from C++ + // new/new[]. + if (RS->isAllocated() && + (RS->getAllocationFamily().Kind == AF_CXXNew || + RS->getAllocationFamily().Kind == AF_CXXNewArray)) { + + // Mark as Relinquished at the constructor site: ownership moves + // into the constructed subobject. Pass the ctor's origin expr as + // the statement associated with this transition. + NewState = NewState->set( + Sym, RefState::getRelinquished(RS->getAllocationFamily(), + Ctor->getOriginExpr())); + } + } + } + + if (NewState != State) + C.addTransition(NewState); + } + } } void MallocChecker::checkPreCall(const CallEvent &Call, @@ -3358,9 +3398,8 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, } bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly( - const CallEvent *Call, - ProgramStateRef State, - SymbolRef &EscapingSymbol) const { + const CallEvent *Call, ProgramStateRef State, + SymbolRef &EscapingSymbol) const { assert(Call); EscapingSymbol = nullptr; @@ -3532,7 +3571,7 @@ ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State, ProgramStateRef MallocChecker::checkConstPointerEscape(ProgramStateRef State, const InvalidatedSymbols &Escaped, const CallEvent *Call, - PointerEscapeKind Kind) const { + PointerEscapeKind Kind) const { // If a const pointer escapes, it may not be freed(), but it could be deleted. return checkPointerEscapeAux(State, Escaped, Call, Kind, /*IsConstPointerEscape*/ true); @@ -3724,61 +3763,61 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N, } Msg = OS.str(); break; - } - case AF_None: - assert(false && "Unhandled allocation family!"); - return nullptr; - } + } + case AF_None: + assert(false && "Unhandled allocation family!"); + return nullptr; + } - // Record the stack frame that is _responsible_ for this memory release - // event. This will be used by the false positive suppression heuristics - // that recognize the release points of reference-counted objects. - // - // Usually (e.g. in C) we say that the _responsible_ stack frame is the - // current innermost stack frame: - ReleaseFunctionLC = CurrentLC->getStackFrame(); - // ...but if the stack contains a destructor call, then we say that the - // outermost destructor stack frame is the _responsible_ one: - for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) { - if (const auto *DD = dyn_cast(LC->getDecl())) { - if (isReferenceCountingPointerDestructor(DD)) { - // This immediately looks like a reference-counting destructor. - // We're bad at guessing the original reference count of the - // object, so suppress the report for now. - BR.markInvalid(getTag(), DD); - - // After report is considered invalid there is no need to proceed - // futher. - return nullptr; - } - - // Switch suspection to outer destructor to catch patterns like: - // (note that class name is distorted to bypass - // isReferenceCountingPointerDestructor() logic) - // - // SmartPointr::~SmartPointr() { - // if (refcount.fetch_sub(1) == 1) - // release_resources(); - // } - // void SmartPointr::release_resources() { - // free(buffer); - // } - // - // This way ReleaseFunctionLC will point to outermost destructor and - // it would be possible to catch wider range of FP. - // - // NOTE: it would be great to support smth like that in C, since - // currently patterns like following won't be supressed: - // - // void doFree(struct Data *data) { free(data); } - // void putData(struct Data *data) - // { - // if (refPut(data)) - // doFree(data); - // } - ReleaseFunctionLC = LC->getStackFrame(); + // Record the stack frame that is _responsible_ for this memory release + // event. This will be used by the false positive suppression heuristics + // that recognize the release points of reference-counted objects. + // + // Usually (e.g. in C) we say that the _responsible_ stack frame is the + // current innermost stack frame: + ReleaseFunctionLC = CurrentLC->getStackFrame(); + // ...but if the stack contains a destructor call, then we say that the + // outermost destructor stack frame is the _responsible_ one: + for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) { + if (const auto *DD = dyn_cast(LC->getDecl())) { + if (isReferenceCountingPointerDestructor(DD)) { + // This immediately looks like a reference-counting destructor. + // We're bad at guessing the original reference count of the + // object, so suppress the report for now. + BR.markInvalid(getTag(), DD); + + // After report is considered invalid there is no need to proceed + // futher. + return nullptr; } + + // Switch suspection to outer destructor to catch patterns like: + // (note that class name is distorted to bypass + // isReferenceCountingPointerDestructor() logic) + // + // SmartPointr::~SmartPointr() { + // if (refcount.fetch_sub(1) == 1) + // release_resources(); + // } + // void SmartPointr::release_resources() { + // free(buffer); + // } + // + // This way ReleaseFunctionLC will point to outermost destructor and + // it would be possible to catch wider range of FP. + // + // NOTE: it would be great to support smth like that in C, since + // currently patterns like following won't be supressed: + // + // void doFree(struct Data *data) { free(data); } + // void putData(struct Data *data) + // { + // if (refPut(data)) + // doFree(data); + // } + ReleaseFunctionLC = LC->getStackFrame(); } + } } else if (isRelinquished(RSCurr, RSPrev, S)) { Msg = "Memory ownership is transferred"; diff --git a/clang/test/Analysis/NewDeleteLeaks.cpp b/clang/test/Analysis/NewDeleteLeaks.cpp index b2bad7e76fad0..5d5f5a633c0dd 100644 --- a/clang/test/Analysis/NewDeleteLeaks.cpp +++ b/clang/test/Analysis/NewDeleteLeaks.cpp @@ -11,9 +11,30 @@ // RUN: -analyzer-checker=unix \ // RUN: -analyzer-config \ // RUN: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=true +// RUN: %clang_analyze_cc1 -std=c++20 -analyzer-checker=cplusplus.NewDeleteLeaks -verify %s #include "Inputs/system-header-simulator-for-malloc.h" +// Minimal move, no headers needed, C++11+ +namespace nstd { + +template +struct remove_reference { using type = T; }; +template +struct remove_reference { using type = T; }; +template +struct remove_reference { using type = T; }; + +template +constexpr typename remove_reference::type&& move(T&& t) noexcept { + using U = typename remove_reference::type; + return static_cast(t); +} + +} // namespace nstd + + + //===----------------------------------------------------------------------===// // Report for which we expect NoOwnershipChangeVisitor to add a new note. //===----------------------------------------------------------------------===// @@ -218,3 +239,138 @@ void caller() { (void)n; } // no-warning: No potential memory leak here, because that's been already reported. } // namespace symbol_reaper_lifetime + + +// Minimal RAII class that properly deletes its pointer. +class Bar { +public: + explicit Bar(int *ptr) : ptr_(ptr) {} + ~Bar() { + if (ptr_) { + delete ptr_; + ptr_ = nullptr; + } + } + + Bar(const Bar &) = delete; + Bar &operator=(const Bar &) = delete; + + Bar(Bar &&other) noexcept : ptr_(other.ptr_) { other.ptr_ = nullptr; } + Bar &operator=(Bar &&other) noexcept { + if (this != &other) { + delete ptr_; + ptr_ = other.ptr_; + other.ptr_ = nullptr; + } + return *this; + } + + int operator*() const { return *ptr_; } + +private: + int *ptr_; +}; + +// Factory returning a prvalue Bar that owns a freshly allocated int. +static Bar make_bar(int v) { return Bar(new int(v)); } + +struct Foo { + Bar a; + Bar b; +}; + +struct FooWithConstructor { + Bar a; + Bar b; + FooWithConstructor(Bar &&original_a, Bar &&original_b) + : a(nstd::move(original_a)), b(nstd::move(original_b)) {} +}; + +//===----------------------------------------------------------------------===// +// No-false-positive regression tests: these must be silent +//===----------------------------------------------------------------------===// + +namespace prvalue_aggregate_transfer { + +void ok_aggregate_from_factory() { + Foo foo = {make_bar(1), make_bar(2)}; // expected-no-diagnostics +} + +void ok_aggregate_from_temporary_exprs() { + Foo foo = {Bar(new int(1)), Bar(new int(2))}; // expected-no-diagnostics +} + +void ok_ctor_from_factory_rvalues() { + FooWithConstructor foo = {make_bar(1), make_bar(2)}; // expected-no-diagnostics +} + +} // namespace prvalue_aggregate_transfer + +//===----------------------------------------------------------------------===// +// True-positive regression tests: these should still warn +//===----------------------------------------------------------------------===// + +class BarNoDelete { +public: + explicit BarNoDelete(int *ptr) : ptr_(ptr) {} + ~BarNoDelete() {} // intentionally missing delete -> leak + + BarNoDelete(const BarNoDelete &) = delete; + BarNoDelete &operator=(const BarNoDelete &) = delete; + + BarNoDelete(BarNoDelete &&other) noexcept : ptr_(other.ptr_) { + other.ptr_ = nullptr; + } + BarNoDelete &operator=(BarNoDelete &&other) noexcept { + if (this != &other) { + // no delete of old ptr_ on purpose + ptr_ = other.ptr_; + other.ptr_ = nullptr; + } + return *this; + } + +private: + int *ptr_; +}; + +static BarNoDelete make_bar_nd(int v) { return BarNoDelete(new int(v)); } + +struct FooND { + BarNoDelete a; + BarNoDelete b; +}; + +namespace prvalue_aggregate_positive { + +void leak_aggregate_from_factory() { + FooND f = {make_bar_nd(1), make_bar_nd(2)}; + // expected-warning@-1 {{Potential memory leak}} +} + +void leak_direct_member() { + BarNoDelete b(new int(3)); + // expected-warning@-1 {{Potential memory leak}} +} + +} // namespace prvalue_aggregate_positive + +//===----------------------------------------------------------------------===// +// Guard tests: neighboring behaviors that must remain intact +// These ensure we didn't weaken unrelated diagnostics (mismatch/double-delete). +//===----------------------------------------------------------------------===// + +namespace guards { + +void mismatch_array_delete() { + int *p = new int[4]; + delete p; // expected-warning {{mismatched deallocation: 'delete' should be 'delete[]'}} +} + +void double_delete() { + int *p = new int(1); + delete p; + delete p; // expected-warning {{Attempt to free released memory}} +} + +} // namespace guards