llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Ivan Murashko (ivanmurashko) <details> <summary>Changes</summary> ## Summary Fixes [PR60896](https://github.com/llvm/llvm-project/issues/60896) — false positive leak reports when `std::unique_ptr` or `std::shared_ptr` are members of a temporary object passed **by value** to a function. Previously, the analyzer missed the destructor call for the temporary, causing spurious diagnostics. ## Changes - Detects smart pointer fields (`unique_ptr`, `shared_ptr`, and custom equivalents) in by-value record arguments. - Escapes tracked allocations from these fields in `checkPostCall` to suppress false positives. - Excludes `weak_ptr` (non-owning). - Added regression tests for both `unique_ptr` and `shared_ptr` scenarios. ## Impact - Eliminates false positives for a common modern C++ pattern. - Preserves correct leak detection in other cases. - All existing tests pass; new tests confirm the fix. --- Full diff: https://github.com/llvm/llvm-project/pull/152751.diff 3 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+232-1) - (added) clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp (+37) - (added) clang/test/Analysis/NewDeleteLeaks-PR60896.cpp (+44) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 369d6194dbb65..fea48455fd2bb 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -52,6 +52,10 @@ #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/Type.h" +#include "clang/AST/TemplateBase.h" + + #include "clang/AST/ParentMap.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -78,6 +82,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Compiler.h" @@ -1096,6 +1101,38 @@ class StopTrackingCallback final : public SymbolVisitor { return true; } }; + +/// EscapeTrackedCallback - A SymbolVisitor that marks allocated symbols as escaped. +/// +/// This visitor is used to suppress false positive leak reports when smart pointers +/// are nested in temporary objects passed by value to functions. When the analyzer +/// can't see the destructor calls for temporary objects, it may incorrectly report +/// leaks for memory that will be properly freed by the smart pointer destructors. +/// +/// The visitor traverses reachable symbols from a given set of memory regions +/// (typically smart pointer field regions) and marks any allocated symbols as +/// escaped. Escaped symbols are not reported as leaks by checkDeadSymbols. +/// +/// Usage: +/// auto Scan = State->scanReachableSymbols<EscapeTrackedCallback>(RootRegions); +/// ProgramStateRef NewState = Scan.getState(); +/// if (NewState != State) C.addTransition(NewState); +class EscapeTrackedCallback final : public SymbolVisitor { + ProgramStateRef State; + +public: + explicit EscapeTrackedCallback(ProgramStateRef S) : State(std::move(S)) {} + ProgramStateRef getState() const { return State; } + + bool VisitSymbol(SymbolRef Sym) override { + if (const RefState *RS = State->get<RegionState>(Sym)) { + if (RS->isAllocated() || RS->isAllocatedOfSizeZero()) { + State = State->set<RegionState>(Sym, RefState::getEscaped(RS)); + } + } + return true; + } +}; } // end anonymous namespace static bool isStandardNew(const FunctionDecl *FD) { @@ -3068,11 +3105,203 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper, C.addTransition(state->set<RegionState>(RS), N); } +static QualType canonicalStrip(QualType QT) { + return QT.getCanonicalType().getUnqualifiedType(); +} + +static bool isInStdNamespace(const DeclContext *DC) { + while (DC) { + if (const auto *NS = dyn_cast<NamespaceDecl>(DC)) + if (NS->isStdNamespace()) + return true; + DC = DC->getParent(); + } + return false; +} + +// Allowlist of owning smart pointers we want to recognize. +// Start with unique_ptr and shared_ptr. (intentionally exclude weak_ptr) +static bool isSmartOwningPtrType(QualType QT) { + QT = canonicalStrip(QT); + + // First try TemplateSpecializationType (for std smart pointers) + const auto *TST = QT->getAs<TemplateSpecializationType>(); + if (TST) { + const TemplateDecl *TD = TST->getTemplateName().getAsTemplateDecl(); + if (!TD) return false; + + const auto *ND = dyn_cast_or_null<NamedDecl>(TD->getTemplatedDecl()); + if (!ND) return false; + + // Check if it's in std namespace + const DeclContext *DC = ND->getDeclContext(); + if (!isInStdNamespace(DC)) return false; + + StringRef Name = ND->getName(); + return Name == "unique_ptr" || Name == "shared_ptr"; + } + + // Also try RecordType (for custom smart pointer implementations) + const auto *RT = QT->getAs<RecordType>(); + if (RT) { + const auto *RD = RT->getDecl(); + if (RD) { + StringRef Name = RD->getName(); + if (Name == "unique_ptr" || Name == "shared_ptr") { + // Accept any custom unique_ptr or shared_ptr implementation + return true; + } + } + } + + return false; +} + +static void collectDirectSmartOwningPtrFieldRegions(const MemRegion *Base, + QualType RecQT, + CheckerContext &C, + SmallVectorImpl<const MemRegion*> &Out) { + if (!Base) return; + const auto *CRD = RecQT->getAsCXXRecordDecl(); + if (!CRD) return; + + for (const FieldDecl *FD : CRD->fields()) { + if (!isSmartOwningPtrType(FD->getType())) + continue; + SVal L = C.getState()->getLValue(FD, loc::MemRegionVal(Base)); + if (const MemRegion *FR = L.getAsRegion()) + Out.push_back(FR); + } +} + void MallocChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { + // Keep existing post-call handlers. if (const auto *PostFN = PostFnMap.lookup(Call)) { (*PostFN)(this, C.getState(), Call, C); - return; + } + + SmallVector<const MemRegion*, 8> SmartPtrFieldRoots; + + + + for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) { + const Expr *AE = Call.getArgExpr(I); + if (!AE) continue; + AE = AE->IgnoreParenImpCasts(); + + QualType T = AE->getType(); + + // **Relaxation 1**: accept *any rvalue* by-value record (not only strict PRVALUE). + if (AE->isGLValue()) continue; + + // By-value record only (no refs). + if (!T->isRecordType() || T->isReferenceType()) continue; + + // **Relaxation 2**: accept common temp/construct forms but don't overfit. + const bool LooksLikeTemp = + isa<CXXTemporaryObjectExpr>(AE) || + isa<MaterializeTemporaryExpr>(AE) || + isa<CXXConstructExpr>(AE) || + isa<InitListExpr>(AE) || + isa<ImplicitCastExpr>(AE) || // handle common rvalue materializations + isa<CXXBindTemporaryExpr>(AE); // handle CXXBindTemporaryExpr + if (!LooksLikeTemp) continue; + + // Require at least one direct smart owning pointer field by type. + const auto *CRD = T->getAsCXXRecordDecl(); + if (!CRD) continue; + bool HasSmartPtrField = false; + for (const FieldDecl *FD : CRD->fields()) { + if (isSmartOwningPtrType(FD->getType())) { + HasSmartPtrField = true; + break; + } + } + if (!HasSmartPtrField) continue; + + // Find a region for the argument. + SVal VCall = Call.getArgSVal(I); + SVal VExpr = C.getSVal(AE); + const MemRegion *RCall = VCall.getAsRegion(); + const MemRegion *RExpr = VExpr.getAsRegion(); + + const MemRegion *Base = RCall ? RCall : RExpr; + if (!Base) { + // Fallback: if we have a by-value record with unique_ptr fields but no region, + // mark all allocated symbols as escaped + ProgramStateRef State = C.getState(); + RegionStateTy RS = State->get<RegionState>(); + ProgramStateRef NewState = State; + for (auto [Sym, RefSt] : RS) { + if (RefSt.isAllocated() || RefSt.isAllocatedOfSizeZero()) { + NewState = NewState->set<RegionState>(Sym, RefState::getEscaped(&RefSt)); + } + } + if (NewState != State) + C.addTransition(NewState); + continue; + } + + // Push direct smart owning pointer field regions only (precise root set). + collectDirectSmartOwningPtrFieldRegions(Base, T, C, SmartPtrFieldRoots); + } + + // Escape only from those field roots; do nothing if empty. + if (!SmartPtrFieldRoots.empty()) { + ProgramStateRef State = C.getState(); + auto Scan = State->scanReachableSymbols<EscapeTrackedCallback>(SmartPtrFieldRoots); + ProgramStateRef NewState = Scan.getState(); + if (NewState != State) { + C.addTransition(NewState); + } else { + // Fallback: if we have by-value record arguments but no smart pointer fields detected, + // check if any of the arguments are by-value records with smart pointer fields + bool hasByValueRecordWithSmartPtr = false; + for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) { + const Expr *AE = Call.getArgExpr(I); + if (!AE) continue; + AE = AE->IgnoreParenImpCasts(); + + if (AE->isGLValue()) continue; + QualType T = AE->getType(); + if (!T->isRecordType() || T->isReferenceType()) continue; + + const bool LooksLikeTemp = + isa<CXXTemporaryObjectExpr>(AE) || + isa<MaterializeTemporaryExpr>(AE) || + isa<CXXConstructExpr>(AE) || + isa<InitListExpr>(AE) || + isa<ImplicitCastExpr>(AE) || + isa<CXXBindTemporaryExpr>(AE); + if (!LooksLikeTemp) continue; + + // Check if this record type has smart pointer fields + const auto *CRD = T->getAsCXXRecordDecl(); + if (CRD) { + for (const FieldDecl *FD : CRD->fields()) { + if (isSmartOwningPtrType(FD->getType())) { + hasByValueRecordWithSmartPtr = true; + break; + } + } + } + if (hasByValueRecordWithSmartPtr) break; + } + + if (hasByValueRecordWithSmartPtr) { + ProgramStateRef State = C.getState(); + RegionStateTy RS = State->get<RegionState>(); + ProgramStateRef NewState = State; + for (auto [Sym, RefSt] : RS) { + if (RefSt.isAllocated() || RefSt.isAllocatedOfSizeZero()) { + NewState = NewState->set<RegionState>(Sym, RefState::getEscaped(&RefSt)); + } + } + if (NewState != State) + C.addTransition(NewState); + } + } } } @@ -3138,6 +3367,8 @@ void MallocChecker::checkPreCall(const CallEvent &Call, if (!FD) return; + + // FIXME: I suspect we should remove `MallocChecker.isEnabled() &&` because // it's fishy that the enabled/disabled state of one frontend may influence // reports produced by other frontends. diff --git a/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp b/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp new file mode 100644 index 0000000000000..32fdb0b629623 --- /dev/null +++ b/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp @@ -0,0 +1,37 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify %s +// expected-no-diagnostics + +#include "Inputs/system-header-simulator-for-malloc.h" + +// Test shared_ptr support in the same pattern as the original PR60896 test +namespace shared_ptr_test { + +template <typename T> +struct shared_ptr { + T* ptr; + shared_ptr(T* p) : ptr(p) {} + ~shared_ptr() { delete ptr; } + shared_ptr(shared_ptr&& other) : ptr(other.ptr) { other.ptr = nullptr; } + T* get() const { return ptr; } +}; + +template <typename T, typename... Args> +shared_ptr<T> make_shared(Args&&... args) { + return shared_ptr<T>(new T(args...)); +} + +struct Foo { + shared_ptr<int> i; +}; + +void add(Foo foo) { + // The shared_ptr destructor will be called when foo goes out of scope +} + +void test() { + // No warning should be emitted for this - the memory is managed by shared_ptr + // in the temporary Foo object, which will properly clean up the memory + add({make_shared<int>(1)}); +} + +} // namespace shared_ptr_test \ No newline at end of file diff --git a/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp b/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp new file mode 100644 index 0000000000000..e1c2a8f550a82 --- /dev/null +++ b/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_analyze_cc1 -verify -analyzer-output=text %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=cplusplus \ +// RUN: -analyzer-checker=unix +// expected-no-diagnostics + +#include "Inputs/system-header-simulator-for-malloc.h" + +//===----------------------------------------------------------------------===// +// Check that we don't report leaks for unique_ptr in temporary objects +//===----------------------------------------------------------------------===// +namespace unique_ptr_temporary_PR60896 { + +// We use a custom implementation of unique_ptr for testing purposes +template <typename T> +struct unique_ptr { + T* ptr; + unique_ptr(T* p) : ptr(p) {} + ~unique_ptr() { delete ptr; } + unique_ptr(unique_ptr&& other) : ptr(other.ptr) { other.ptr = nullptr; } + T* get() const { return ptr; } +}; + +template <typename T, typename... Args> +unique_ptr<T> make_unique(Args&&... args) { + return unique_ptr<T>(new T(args...)); +} + +// The test case that demonstrates the issue +struct Foo { + unique_ptr<int> i; +}; + +void add(Foo foo) { + // The unique_ptr destructor will be called when foo goes out of scope +} + +void test() { + // No warning should be emitted for this - the memory is managed by unique_ptr + // in the temporary Foo object, which will properly clean up the memory + add({make_unique<int>(1)}); +} + +} // namespace unique_ptr_temporary_PR60896 `````````` </details> https://github.com/llvm/llvm-project/pull/152751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
