-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[analyzer] MallocChecker – Fix false positive leak for smart pointers in temporary objects #152751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
7d1c700
f6d1a05
650b0f7
0cc838f
333e5ba
07cfed9
66bf4e6
2dc6776
83173d0
fddc1b4
617a02a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -52,6 +52,9 @@ | |||||||
#include "clang/AST/DeclTemplate.h" | ||||||||
#include "clang/AST/Expr.h" | ||||||||
#include "clang/AST/ExprCXX.h" | ||||||||
#include "clang/AST/TemplateBase.h" | ||||||||
#include "clang/AST/Type.h" | ||||||||
|
||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
#include "clang/AST/ParentMap.h" | ||||||||
#include "clang/ASTMatchers/ASTMatchFinder.h" | ||||||||
#include "clang/ASTMatchers/ASTMatchers.h" | ||||||||
|
@@ -78,6 +81,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 +1100,41 @@ 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 { | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
ProgramStateRef State; | ||||||||
|
||||||||
public: | ||||||||
explicit EscapeTrackedCallback(ProgramStateRef S) : State(std::move(S)) {} | ||||||||
ProgramStateRef getState() const { return State; } | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose we no longer need this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||||||||
|
||||||||
bool VisitSymbol(SymbolRef Sym) override { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be in the private part, if we made There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved |
||||||||
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 +3107,217 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper, | |||||||
C.addTransition(state->set<RegionState>(RS), N); | ||||||||
} | ||||||||
|
||||||||
static QualType canonicalStrip(QualType QT) { | ||||||||
return QT.getCanonicalType().getUnqualifiedType(); | ||||||||
} | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
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; | ||||||||
} | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
// 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) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied |
||||||||
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; | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
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; | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
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); | ||||||||
} | ||||||||
Comment on lines
+3214
to
+3220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we iterate fields. What about fields from base classes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added base class field support: |
||||||||
} | ||||||||
|
||||||||
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; | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||||||||
|
||||||||
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 | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
if (!LooksLikeTemp) | ||||||||
continue; | ||||||||
|
||||||||
// Require at least one direct smart owning pointer field by type. | ||||||||
const auto *CRD = T->getAsCXXRecordDecl(); | ||||||||
if (!CRD) | ||||||||
continue; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can this fail if we already know its of a record type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type checking was moved earlier and simplified, eliminating the redundant null check. |
||||||||
bool HasSmartPtrField = false; | ||||||||
for (const FieldDecl *FD : CRD->fields()) { | ||||||||
if (isSmartOwningPtrType(FD->getType())) { | ||||||||
HasSmartPtrField = true; | ||||||||
break; | ||||||||
} | ||||||||
} | ||||||||
if (!HasSmartPtrField) | ||||||||
continue; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hunk should be hoisted into a dedicated function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed — the smart pointer field check is now hoisted into a dedicated |
||||||||
|
||||||||
// 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)); | ||||||||
} | ||||||||
} | ||||||||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
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)); | ||||||||
} | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of code duplication again and again. |
||||||||
if (NewState != State) | ||||||||
C.addTransition(NewState); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify %s | ||
// expected-no-diagnostics | ||
|
||
steakhal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; } | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure some header simulator already has a unique_ptr somewhere. We should use that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at |
||
|
||
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)}); | ||
} | ||
Comment on lines
+68
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also test this with multiple owning arguments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the test |
||
|
||
} // namespace unique_ptr_temporary_PR60896 |
Uh oh!
There was an error while loading. Please reload this page.