Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7d1c700
[clang-analyzer] Add regression test for PR60896
ivanmurashko Aug 8, 2025
f6d1a05
[clang-analizer] MallocChecker: fix false positive leak for unique_pt…
ivanmurashko Aug 8, 2025
650b0f7
[clang-analyzer] MallocChecker: extend false positive leak fix to sup…
ivanmurashko Aug 8, 2025
0cc838f
[clang-analyzer] Apply clang-format
ivanmurashko Aug 8, 2025
333e5ba
[analyzer][test] Refactor smart pointer leak suppression and combine …
ivanmurashko Aug 9, 2025
07cfed9
[analyzer] MallocChecker: Address minor style and review comments
ivanmurashko Aug 9, 2025
66bf4e6
[analyzer] MallocChecker: Factor out smart pointer name check
ivanmurashko Aug 10, 2025
2dc6776
[clang-analyzer] Fix addTransition misuse - consolidate state updates
ivanmurashko Aug 10, 2025
83173d0
[analyzer][test] Add multiple owning arguments test case
ivanmurashko Aug 10, 2025
fddc1b4
[analyzer] Simplify MallocChecker::checkPostCall
ivanmurashko Aug 11, 2025
617a02a
[analyzer] scanReachableSymbols is expensive, so we use a single visi…
ivanmurashko Aug 11, 2025
c007afd
[analyzer] Fix overly broad escape logic in MallocChecker for mixed o…
ivanmurashko Aug 15, 2025
bb5eacc
[analyzer] Refactor MallocChecker::checkPostCall to improve readability
ivanmurashko Aug 15, 2025
db60663
[analyzer][test] Reorganize NewDeleteLeaks-PR60896 test into unified …
ivanmurashko Aug 15, 2025
7df1f75
[analyzer] Extract isSmartPtrName helper, add comments, and improve r…
ivanmurashko Aug 15, 2025
bb0d4f1
[analyzer] Fix addTransition API misuse in MallocChecker::checkPostCall
ivanmurashko Aug 15, 2025
a166bb3
[analyzer] Rename function and use set semantics in MallocChecker
ivanmurashko Aug 22, 2025
b4b9062
[analyzer] Fix out-of-bounds access in handleSmartPointerConstructorA…
ivanmurashko Aug 22, 2025
b8f4620
Update clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
ivanmurashko Aug 22, 2025
08a24ef
[analyzer] Refactor smart pointer detection and fix naming consistency
ivanmurashko Aug 28, 2025
f35ac73
Update clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
ivanmurashko Aug 30, 2025
d01533e
Update clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
ivanmurashko Aug 30, 2025
10569b6
Update clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
ivanmurashko Aug 30, 2025
f41c0bc
Update clang/test/Analysis/NewDeleteLeaks-PR60896.cpp
ivanmurashko Aug 30, 2025
c955216
Update clang/test/Analysis/NewDeleteLeaks-PR60896.cpp
ivanmurashko Aug 30, 2025
7c6ffa8
[analyzer] Eliminate duplicate traversal logic in MallocChecker smart…
ivanmurashko Aug 29, 2025
64d3aef
[analyzer] Make EscapeTrackedCallback::VisitSymbol public
ivanmurashko Aug 29, 2025
ee1c3ab
LIT tests updates to reflect the heuristic behaviour in the tests (sh…
ivanmurashko Aug 30, 2025
692db9e
[analyzer] Refactoring (name change): SmartOwningPtr -> SmartPtr
ivanmurashko Aug 30, 2025
49b7d31
[analyzer] Rename Base variables in MallocChecker to avoid confusion …
ivanmurashko Aug 30, 2025
d472fc4
[analyzer] Move variadic constructor test to separate namespace and u…
ivanmurashko Aug 30, 2025
6dd7671
Update clang/test/Analysis/NewDeleteLeaks-PR60896.cpp
ivanmurashko Sep 3, 2025
196abdb
[analyzer] Document dual behavior of hasSmartPtrField function
ivanmurashko Sep 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
229 changes: 227 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,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"
Expand Down Expand Up @@ -1096,6 +1097,47 @@ 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.
class EscapeTrackedCallback final : public SymbolVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this class declaration wrapped in an anonymous namespace?
https://llvm.org/docs/CodingStandards.html#restrict-visibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the EscapeTrackedCallback class is properly wrapped in an anonymous namespace that starts at line 1094. It shares the same anonymous namespace with the StopTrackingCallback class defined just above it.

ProgramStateRef State;

explicit EscapeTrackedCallback(ProgramStateRef S) : State(std::move(S)) {}

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;
}

public:
/// Escape tracked regions reachable from the given roots.
static ProgramStateRef
EscapeTrackedRegionsReachableFrom(ArrayRef<const MemRegion *> Roots,
ProgramStateRef State) {
EscapeTrackedCallback Visitor(State);
for (const MemRegion *R : Roots) {
State->scanReachableSymbols(loc::MemRegionVal(R), Visitor);
}
return Visitor.State;
}

friend class SymbolVisitor;
};
} // end anonymous namespace

static bool isStandardNew(const FunctionDecl *FD) {
Expand Down Expand Up @@ -3068,11 +3110,195 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
C.addTransition(state->set<RegionState>(RS), N);
}

// 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 = QT->getCanonicalTypeUnqualified();

auto isSmartPtrName = [](StringRef Name) {
return Name == "unique_ptr" || Name == "shared_ptr";
};

// First try TemplateSpecializationType (for std smart pointers)
if (const auto *TST = QT->getAs<TemplateSpecializationType>()) {
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
if (!isWithinStdNamespace(ND))
return false;

return isSmartPtrName(ND->getName());
}

// Also try RecordType (for custom smart pointer implementations)
if (const auto *RD = QT->getAsCXXRecordDecl()) {
// Accept any custom unique_ptr or shared_ptr implementation
return (isSmartPtrName(RD->getName()));
}

return false;
}

static bool hasSmartPtrField(const CXXRecordDecl *CRD) {
// Check direct fields
if (llvm::any_of(CRD->fields(), [](const FieldDecl *FD) {
return isSmartOwningPtrType(FD->getType());
}))
return true;

// Check fields from base classes
for (const CXXBaseSpecifier &Base : CRD->bases()) {
if (const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl()) {
if (hasSmartPtrField(BaseDecl))
return true;
}
}
return false;
}

static bool isRvalueByValueRecord(const Expr *AE) {
if (AE->isGLValue())
return false;

QualType T = AE->getType();
if (!T->isRecordType() || T->isReferenceType())
return false;

// Accept common temp/construct forms but don't overfit.
return isa<CXXTemporaryObjectExpr, MaterializeTemporaryExpr, CXXConstructExpr,
InitListExpr, ImplicitCastExpr, CXXBindTemporaryExpr>(AE);
}

static bool isRvalueByValueRecordWithSmartPtr(const Expr *AE) {
if (!isRvalueByValueRecord(AE))
return false;

const auto *CRD = AE->getType()->getAsCXXRecordDecl();
return CRD && hasSmartPtrField(CRD);
}

static ProgramStateRef escapeAllAllocatedSymbols(ProgramStateRef State) {
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));
}
}
return NewState;
}

static void collectDirectSmartOwningPtrFieldRegions(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this function no longer only collects direct field regions, as it also traverses the base subobjects.
Should we rename this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! You're absolutely right. I've renamed the function from collectDirectSmartOwningPtrFieldRegions to collectSmartOwningPtrFieldRegions to better reflect that it now traverses the entire inheritance hierarchy, not just direct fields (commit a166bb3).

const MemRegion *Base, QualType RecQT, CheckerContext &C,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name const MemRegion *Base is a bit unfortunate because it appears to refer to the frequently used method MemRegion::getBaseRegion which uses the word "base" in a different meaning:

  • SomeRegion.getBaseRegion() returns the largest cohesive block of memory that contains the given region: e.g. calling it on the field region array[5].field.otherfield would return the region corresponding to array.
  • This variable name is called "base" because (at least in the recursive calls) it is the subobject corresponding to a certain base class.

(Perhaps this would be more clear for others, but during the first review I completely missed that this variable is not connected to getBaseRegion.)

To clarify the naming, I'd suggest:

  • renaming this parameter cost MemRegion *Base to e.g. const MemRegion *Reg (a natural name for "the" region);
  • later in the body of the function renaming BaseRegion to e.g. BaseObjRegion (to highlight that it's a region corresponding to a base object).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I applied the name change in 49b7d31.

SmallVectorImpl<const MemRegion *> &Out) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt out have set semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I've changed the parameter to use llvm::SmallPtrSetImpl<const MemRegion *> to ensure each region is only collected once, preventing duplicates from inheritance traversal (commit a166bb3).

if (!Base)
return;
const auto *CRD = RecQT->getAsCXXRecordDecl();
if (!CRD)
return;

// Collect direct fields
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);
}

// Collect fields from base classes
for (const CXXBaseSpecifier &BaseSpec : CRD->bases()) {
if (const CXXRecordDecl *BaseDecl =
BaseSpec.getType()->getAsCXXRecordDecl()) {
// Get the base class region
SVal BaseL = C.getState()->getLValue(BaseDecl, Base->getAs<SubRegion>(),
BaseSpec.isVirtual());
if (const MemRegion *BaseRegion = BaseL.getAsRegion()) {
// Recursively collect fields from this base class
collectDirectSmartOwningPtrFieldRegions(BaseRegion, BaseSpec.getType(),
C, Out);
}
}
}
}

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;
ProgramStateRef State = C.getState();
bool needsStateUpdate = false;

for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) {
const Expr *AE = Call.getArgExpr(I);
if (!AE)
continue;
AE = AE->IgnoreParenImpCasts();

if (!isRvalueByValueRecordWithSmartPtr(AE))
continue;

// Find a region for the argument.
SVal ArgVal = Call.getArgSVal(I);
const MemRegion *ArgRegion = ArgVal.getAsRegion();
if (!ArgRegion) {
// Fallback: if we have a by-value record with smart pointer fields but no
// region, mark all allocated symbols as escaped
State = escapeAllAllocatedSymbols(State);
needsStateUpdate = true;
continue;
}

// Push direct smart owning pointer field regions only (precise root set).
collectDirectSmartOwningPtrFieldRegions(ArgRegion, AE->getType(), C,
SmartPtrFieldRoots);
}

// Escape only from those field roots; do nothing if empty.
if (!SmartPtrFieldRoots.empty()) {
ProgramStateRef NewState =
EscapeTrackedCallback::EscapeTrackedRegionsReachableFrom(
SmartPtrFieldRoots, State);
if (NewState != State) {
State = NewState;
needsStateUpdate = true;
} 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 (isRvalueByValueRecordWithSmartPtr(AE)) {
hasByValueRecordWithSmartPtr = true;
break;
}
}

if (hasByValueRecordWithSmartPtr) {
State = escapeAllAllocatedSymbols(State);
needsStateUpdate = true;
}
}
}

// Apply all state changes in a single transition
if (needsStateUpdate) {
C.addTransition(State);
}
}

Expand Down Expand Up @@ -3194,7 +3420,6 @@ void MallocChecker::checkEscapeOnReturn(const ReturnStmt *S,
if (!Sym)
// If we are returning a field of the allocated struct or an array element,
// the callee could still free the memory.
// TODO: This logic should be a part of generic symbol escape callback.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a random obsolete TODO, or is it fixed by the this PR? (No action excepted, just curious.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is not directly related to the PR. I believe that the TODO comment became obsolete a long time ago, and I deleted it for that reason.

The TODO comment was added in February 2012 (see 4ca45b1) when fixing a false positive in the malloc checker related to returning fields of allocated structs or array elements via pointer arithmetic. The author of the commit embedded escape-on-return logic directly in checkPreStmt but recognized this was architecturally suboptimal, suggesting it should be part of a "generic symbol escape callback" instead.

This TODO was rendered unnecessary when commit 122171e refactored the code by extracting the escape logic into a dedicated checkEscapeOnReturn method that could be reused by multiple callbacks (checkPreStmt and checkEndFunction). This addressed the architectural concern without requiring a complete redesign of the symbol escape callback system.

if (const MemRegion *MR = RetVal.getAsRegion())
if (isa<FieldRegion, ElementRegion>(MR))
if (const SymbolicRegion *BMR =
Expand Down
Loading