From 5bf5270d9237715114f560c4fb4b17ff3e29a7f0 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 27 Jan 2025 11:35:03 -0600 Subject: [PATCH 01/40] wip --- .../Checkers/StackAddrEscapeChecker.cpp | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index f4de3b500499c..59b47371df0d2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -20,6 +20,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h" #include "llvm/ADT/SmallString.h" #include "llvm/Support/raw_ostream.h" using namespace clang; @@ -233,6 +234,43 @@ void StackAddrEscapeChecker::checkReturnedBlockCaptures( } } +class FindEscapingStackAddrsVisitor : public FullSValVisitor +{ + const StackFrameContext *Ctxt; + + SmallVector Escaped; + + bool IsTopFrame; + +public: + explicit FindEscapingStackAddrsVisitor(CheckerContext &CC, bool IsTopFrame) : + Ctxt(CC.getStackFrame()), IsTopFrame(IsTopFrame) {} + + bool CheckForEscapes(SVal V) { return Visit(V); } + + bool VisitSymbolVal(nonloc::SymbolVal SymV) { + return Visit(SymV.getSymbol()); + } + + bool VisitLocAsInteger(nonloc::LocAsInteger LAI) { + return Visit(LAI.getAsRegion()); + } + + bool VisitMemRegionVal(loc::MemRegionVal MRVal) { + return Visit(MRVal.getRegion()); + } + + bool VisitMemRegion(const MemRegion *R) { + const MemSpaceRegion *MS = R->getMemorySpace(); + const StackSpaceRegion *SSR = dyn_cast(MS); + if (SSR && Ctxt == SSR->getStackFrame()) { + Escaped.push_back(R); + return true; + } + return false; + } +}; + void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker]) @@ -257,7 +295,12 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, return; RetE = RetE->IgnoreParens(); + FindEscapingStackAddrsVisitor EscapeFinder(C, C.getPredecessor()->getLocationContext()->inTopFrame()); + SVal V = C.getSVal(RetE); + + bool AnyEscapes = EscapeFinder.CheckForEscapes(V); + const MemRegion *R = V.getAsRegion(); if (!R) return; @@ -402,7 +445,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, bool checkForDanglingStackVariable(const MemRegion *Referrer, const MemRegion *Referred) { const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer); - const auto *ReferredMemSpace = + const auto *ReferredMemSpace = Referred->getMemorySpace()->getAs(); if (!ReferrerMemSpace || !ReferredMemSpace) From ff3478d8a1df07611d6e8d5aa00eed280db492a7 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Wed, 29 Jan 2025 00:34:45 -0600 Subject: [PATCH 02/40] wip --- .../Checkers/StackAddrEscapeChecker.cpp | 143 ++++++++++-------- clang/test/Analysis/stack-addr-ps.cpp | 73 ++++++--- clang/test/Analysis/stackaddrleak.cpp | 2 +- 3 files changed, 136 insertions(+), 82 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 59b47371df0d2..b6ef375936ccc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -22,6 +22,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h" #include "llvm/ADT/SmallString.h" +#include "llvm/Support/Debug.h" #include "llvm/Support/raw_ostream.h" using namespace clang; using namespace ento; @@ -210,6 +211,7 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( } } + void StackAddrEscapeChecker::checkReturnedBlockCaptures( const BlockDataRegion &B, CheckerContext &C) const { for (const MemRegion *Region : getCapturedStackRegions(B, C)) { @@ -234,43 +236,6 @@ void StackAddrEscapeChecker::checkReturnedBlockCaptures( } } -class FindEscapingStackAddrsVisitor : public FullSValVisitor -{ - const StackFrameContext *Ctxt; - - SmallVector Escaped; - - bool IsTopFrame; - -public: - explicit FindEscapingStackAddrsVisitor(CheckerContext &CC, bool IsTopFrame) : - Ctxt(CC.getStackFrame()), IsTopFrame(IsTopFrame) {} - - bool CheckForEscapes(SVal V) { return Visit(V); } - - bool VisitSymbolVal(nonloc::SymbolVal SymV) { - return Visit(SymV.getSymbol()); - } - - bool VisitLocAsInteger(nonloc::LocAsInteger LAI) { - return Visit(LAI.getAsRegion()); - } - - bool VisitMemRegionVal(loc::MemRegionVal MRVal) { - return Visit(MRVal.getRegion()); - } - - bool VisitMemRegion(const MemRegion *R) { - const MemSpaceRegion *MS = R->getMemorySpace(); - const StackSpaceRegion *SSR = dyn_cast(MS); - if (SSR && Ctxt == SSR->getStackFrame()) { - Escaped.push_back(R); - return true; - } - return false; - } -}; - void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker]) @@ -285,6 +250,33 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, } } +class FindStackRegionsSymbolVisitor final : public SymbolVisitor { + const StackFrameContext *StackFrameContext; + SmallVector &EscapingStackRegions; +public: + explicit FindStackRegionsSymbolVisitor(CheckerContext &Ctxt, SmallVector &StorageForStackRegions) + : StackFrameContext(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} + + bool VisitSymbol(SymbolRef sym) override { return true; } + + bool VisitMemRegion(const MemRegion *MR) override { + llvm::dbgs() << "Visiting region: "; + MR->dumpToStream(llvm::dbgs()); + llvm::dbgs() << '\n'; + + const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); + if (SSR && SSR->getStackFrame() == StackFrameContext) + EscapingStackRegions.push_back(MR); + return true; + } +}; + +static void FindEscapingStackRegions(CheckerContext &C, SmallVector &EscapedRegionsStorage, SVal SVal) { + FindStackRegionsSymbolVisitor Finder(C, EscapedRegionsStorage); + ScanReachableSymbols Scanner(C.getState(), Finder); + Scanner.scan(SVal); +} + void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) @@ -295,40 +287,67 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, return; RetE = RetE->IgnoreParens(); - FindEscapingStackAddrsVisitor EscapeFinder(C, C.getPredecessor()->getLocationContext()->inTopFrame()); - SVal V = C.getSVal(RetE); - bool AnyEscapes = EscapeFinder.CheckForEscapes(V); + SmallVector EscapedRegions; + FindEscapingStackRegions(C, EscapedRegions, V); + + for (const MemRegion *ER : EscapedRegions) { + llvm::dbgs() << "Escaped region: "; + ER->dumpToStream(llvm::dbgs()); + llvm::dbgs() << '\n'; + + const MemRegion *R = ER; + while (true) { + switch (R->getKind()) { + case MemRegion::ElementRegionKind: + case MemRegion::FieldRegionKind: + case MemRegion::ObjCIvarRegionKind: + case MemRegion::CXXBaseObjectRegionKind: + case MemRegion::CXXDerivedObjectRegionKind: { + const SubRegion *SR = cast(R); + R = SR->getSuperRegion(); + llvm::dbgs() << "SuperRegion: "; + R->dumpToStream(llvm::dbgs()); + llvm::dbgs() << '\n'; + continue; + } + default: + break; + } + break; + } - const MemRegion *R = V.getAsRegion(); - if (!R) - return; + const VarRegion *Base = dyn_cast(ER->getBaseRegion()); + if (Base) { + Base->getDecl()->getBeginLoc().dump(C.getSourceManager()); + } - if (const BlockDataRegion *B = dyn_cast(R)) - checkReturnedBlockCaptures(*B, C); + EmitStackError(C, ER, RetE); + } - if (!isa(R->getMemorySpace()) || isNotInCurrentFrame(R, C)) - return; + // if (const BlockDataRegion *B = dyn_cast(R)) + // checkReturnedBlockCaptures(*B, C); // Returning a record by value is fine. (In this case, the returned // expression will be a copy-constructor, possibly wrapped in an // ExprWithCleanups node.) - if (const ExprWithCleanups *Cleanup = dyn_cast(RetE)) - RetE = Cleanup->getSubExpr(); - if (isa(RetE) && RetE->getType()->isRecordType()) - return; - - // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied - // so the stack address is not escaping here. - if (const auto *ICE = dyn_cast(RetE)) { - if (isa(R) && - ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) { - return; - } - } - - EmitStackError(C, R, RetE); + // if (const ExprWithCleanups *Cleanup = dyn_cast(RetE)) + // RetE = Cleanup->getSubExpr(); + // if (isa(RetE) && RetE->getType()->isRecordType()) + // return; + + // // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied + // // so the stack address is not escaping here. + // if (const auto *ICE = dyn_cast(RetE)) { + // const MemRegion *R = V.getAsRegion(); + // if (R && isa(R) && + // ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) { + // return; + // } + // } + + // EmitStackError(C, R, RetE); } static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) { diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 73e9dbeca460f..cfa887da025a8 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -410,16 +410,16 @@ void** returned_arr_of_ptr_top() { int* p = &local; void** arr = new void*[2]; arr[1] = p; - return arr; -} // no-warning False Negative + return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}} +} void** returned_arr_of_ptr_callee() { int local = 42; int* p = &local; void** arr = new void*[2]; arr[1] = p; - return arr; -} // no-warning False Negative + return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}} +} void returned_arr_of_ptr_caller() { void** arr = returned_arr_of_ptr_callee(); @@ -466,16 +466,16 @@ void** returned_arr_of_ptr_top(int idx) { int* p = &local; void** arr = new void*[2]; arr[idx] = p; - return arr; -} // no-warning False Negative + return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}} +} void** returned_arr_of_ptr_callee(int idx) { int local = 42; int* p = &local; void** arr = new void*[2]; arr[idx] = p; - return arr; -} // no-warning False Negative + return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}} +} void returned_arr_of_ptr_caller(int idx) { void** arr = returned_arr_of_ptr_callee(idx); @@ -525,14 +525,25 @@ S returned_struct_with_ptr_top() { int local = 42; S s; s.p = &local; - return s; -} // no-warning False Negative, requires traversing returned LazyCompoundVals + return s; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}} +} S returned_struct_with_ptr_callee() { int local = 42; S s; s.p = &local; - return s; // expected-warning{{'local' is still referred to by the caller variable 's'}} + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} + +S leak_through_field_of_returned_object() { + int local = 14; + S s{&local}; + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} + +S leak_through_compound_literal() { + int local = 0; + return (S) { &local }; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} } void returned_struct_with_ptr_caller() { @@ -555,6 +566,30 @@ void static_struct_with_ptr() { } } // namespace leaking_via_struct_with_ptr +namespace leaking_via_nested_structs_with_ptr { +struct Inner { + int *ptr; +}; + +struct Outer { + Inner I; +}; + +struct Deriving : public Outer {}; + +Outer leaks_through_nested_objects() { + int local = 0; + Outer O{&local}; + return O; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} + +Deriving leaks_through_base_objects() { + int local = 0; + Deriving D{&local}; + return D; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} +} // namespace leaking_via_nested_structs_with_ptr + namespace leaking_via_ref_to_struct_with_ptr { struct S { int* p; @@ -613,15 +648,15 @@ S* returned_ptr_to_struct_with_ptr_top() { int local = 42; S* s = new S; s->p = &local; - return s; -} // no-warning False Negative + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} S* returned_ptr_to_struct_with_ptr_callee() { int local = 42; S* s = new S; s->p = &local; - return s; -} // no-warning False Negative + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} void returned_ptr_to_struct_with_ptr_caller() { S* s = returned_ptr_to_struct_with_ptr_callee(); @@ -676,15 +711,15 @@ S* returned_ptr_to_struct_with_ptr_top() { int local = 42; S* s = new S[2]; s[1].p = &local; - return s; -} // no-warning False Negative + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} S* returned_ptr_to_struct_with_ptr_callee() { int local = 42; S* s = new S[2]; s[1].p = &local; - return s; -} // no-warning False Negative + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} +} void returned_ptr_to_struct_with_ptr_caller() { S* s = returned_ptr_to_struct_with_ptr_callee(); diff --git a/clang/test/Analysis/stackaddrleak.cpp b/clang/test/Analysis/stackaddrleak.cpp index 3daffb35a6cd9..82697d94a7f60 100644 --- a/clang/test/Analysis/stackaddrleak.cpp +++ b/clang/test/Analysis/stackaddrleak.cpp @@ -22,4 +22,4 @@ myfunction create_func() { } void gh_66221() { create_func()(); -} +} \ No newline at end of file From 9d58c46b7754dd2a12f6d48a2ae76220d8080ea9 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Wed, 29 Jan 2025 20:49:26 -0600 Subject: [PATCH 03/40] Changes to checkPreStmt --- .../Checkers/StackAddrEscapeChecker.cpp | 102 ++++++++---------- clang/test/Analysis/stackaddrleak.cpp | 2 +- 2 files changed, 44 insertions(+), 60 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index b6ef375936ccc..ed9fa80a10de6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -29,7 +29,8 @@ using namespace ento; namespace { class StackAddrEscapeChecker - : public Checker, + : public Checker, check::EndFunction> { mutable IdentifierInfo *dispatch_semaphore_tII = nullptr; mutable std::unique_ptr BT_stackleak; @@ -260,9 +261,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } bool VisitMemRegion(const MemRegion *MR) override { - llvm::dbgs() << "Visiting region: "; - MR->dumpToStream(llvm::dbgs()); - llvm::dbgs() << '\n'; + llvm::dbgs() << "Visiting region: " << MR << '\n'; const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); if (SSR && SSR->getStackFrame() == StackFrameContext) @@ -277,8 +276,44 @@ static void FindEscapingStackRegions(CheckerContext &C, SmallVector &MaybeEscaped, + CheckerContext &C, + const Expr *RetE, + SVal &RetVal) { + + // Returning a record by value is fine. (In this case, the returned + // expression will be a copy-constructor, possibly wrapped in an + // ExprWithCleanups node.) + if (const ExprWithCleanups *Cleanup = dyn_cast(RetE)) + RetE = Cleanup->getSubExpr(); + bool IsConstructExpr = isa(RetE) && RetE->getType()->isRecordType(); + + const MemRegion *RetRegion = RetVal.getAsRegion(); + + // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied + // so the stack address is not escaping here. + bool IsCopyAndAutoreleaseBlockObj = false; + if (const auto *ICE = dyn_cast(RetE)) { + IsCopyAndAutoreleaseBlockObj = RetRegion && + isa(RetRegion) && + ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject; + } + + // Assuming MR is never nullptr + auto ShouldNotEmitError = [=](const MemRegion *MR) -> bool { + // If this particular MR is one of these special cases, then + // don't emit an error for this MR, but this still allows emitting these + // errors for MRs captured by, e.g., the temporary object. + if (RetRegion == MR) { + return IsConstructExpr || IsCopyAndAutoreleaseBlockObj;; + } + return false; + }; + + std::ignore = std::remove_if(MaybeEscaped.begin(), MaybeEscaped.end(), ShouldNotEmitError); +} + +void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) return; @@ -290,64 +325,13 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, SVal V = C.getSVal(RetE); SmallVector EscapedRegions; + FindEscapingStackRegions(C, EscapedRegions, V); + FilterReturnExpressionLeaks(EscapedRegions, C, RetE, V); for (const MemRegion *ER : EscapedRegions) { - llvm::dbgs() << "Escaped region: "; - ER->dumpToStream(llvm::dbgs()); - llvm::dbgs() << '\n'; - - const MemRegion *R = ER; - while (true) { - switch (R->getKind()) { - case MemRegion::ElementRegionKind: - case MemRegion::FieldRegionKind: - case MemRegion::ObjCIvarRegionKind: - case MemRegion::CXXBaseObjectRegionKind: - case MemRegion::CXXDerivedObjectRegionKind: { - const SubRegion *SR = cast(R); - R = SR->getSuperRegion(); - llvm::dbgs() << "SuperRegion: "; - R->dumpToStream(llvm::dbgs()); - llvm::dbgs() << '\n'; - continue; - } - default: - break; - } - break; - } - - const VarRegion *Base = dyn_cast(ER->getBaseRegion()); - if (Base) { - Base->getDecl()->getBeginLoc().dump(C.getSourceManager()); - } - EmitStackError(C, ER, RetE); } - - // if (const BlockDataRegion *B = dyn_cast(R)) - // checkReturnedBlockCaptures(*B, C); - - // Returning a record by value is fine. (In this case, the returned - // expression will be a copy-constructor, possibly wrapped in an - // ExprWithCleanups node.) - // if (const ExprWithCleanups *Cleanup = dyn_cast(RetE)) - // RetE = Cleanup->getSubExpr(); - // if (isa(RetE) && RetE->getType()->isRecordType()) - // return; - - // // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied - // // so the stack address is not escaping here. - // if (const auto *ICE = dyn_cast(RetE)) { - // const MemRegion *R = V.getAsRegion(); - // if (R && isa(R) && - // ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) { - // return; - // } - // } - - // EmitStackError(C, R, RetE); } static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) { diff --git a/clang/test/Analysis/stackaddrleak.cpp b/clang/test/Analysis/stackaddrleak.cpp index 82697d94a7f60..733a86be1091a 100644 --- a/clang/test/Analysis/stackaddrleak.cpp +++ b/clang/test/Analysis/stackaddrleak.cpp @@ -18,7 +18,7 @@ struct myfunction { myfunction create_func() { int n; auto c = [&n] {}; - return c; // expected-warning {{Address of stack memory associated with local variable 'n' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference}} + return c; // expected-warning {{Address of stack memory associated with local variable 'n' returned to caller}} } void gh_66221() { create_func()(); From e9e0ae889368e7be77e3ac5804b5a45f6e28b319 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Wed, 29 Jan 2025 20:49:56 -0600 Subject: [PATCH 04/40] start separating checkEndFunction and checkPreStmt --- .../Checkers/StackAddrEscapeChecker.cpp | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index ed9fa80a10de6..5234f2b27806e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -434,8 +434,8 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, Node = Node->getFirstPred(); } - // Iterate over all bindings to global variables and see if it contains - // a memory region in the stack space. + // Iterate over all bindings to global variables and stack arguments + // see if they contain a memory region in the current stack frame. class CallBack : public StoreManager::BindingsHandler { private: CheckerContext &Ctx; @@ -503,20 +503,33 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region, SVal Val) override { recordInInvalidatedRegions(Region); - const MemRegion *VR = Val.getAsRegion(); - if (!VR) - return true; - if (checkForDanglingStackVariable(Region, VR)) - return true; + const MemRegion *ReferrerMR = getStackOrGlobalSpaceRegion(Region); - // Check the globals for the same. - if (!isa_and_nonnull( - getStackOrGlobalSpaceRegion(Region))) + if (!isa_and_nonnull(ReferrerMR)) return true; - if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx)) - V.emplace_back(Region, VR); + + SmallVector EscapingStackAddrs; + FindEscapingStackRegions(Ctx, EscapingStackAddrs, Val); + + for (const MemRegion *Escapee : EscapingStackAddrs) { + V.emplace_back(Region, Escapee); + } + return true; + // const MemRegion *VR = Val.getAsRegion(); + // if (!VR) + // return true; + + // if (checkForDanglingStackVariable(Region, VR)) + // return true; + + // // Check the globals for the same. + // if (!isa_and_nonnull()) + // return true; + // if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx)) + // V.emplace_back(Region, VR); + // return true; } }; From 4edaa02c00546429b84cc4837cacafd8d7cf26d1 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Wed, 29 Jan 2025 23:00:43 -0600 Subject: [PATCH 05/40] changes to checkEndFunction --- .../Checkers/StackAddrEscapeChecker.cpp | 66 ++++++++----------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 5234f2b27806e..7bc99d681802e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -261,7 +261,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } bool VisitMemRegion(const MemRegion *MR) override { - llvm::dbgs() << "Visiting region: " << MR << '\n'; + // llvm::dbgs() << "Visiting region: " << MR << '\n'; const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); if (SSR && SSR->getStackFrame() == StackFrameContext) @@ -445,38 +445,39 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, /// Look for stack variables referring to popped stack variables. /// Returns true only if it found some dangling stack variables /// referred by an other stack variable from different stack frame. - bool checkForDanglingStackVariable(const MemRegion *Referrer, - const MemRegion *Referred) { - const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer); - const auto *ReferredMemSpace = - Referred->getMemorySpace()->getAs(); + /// Here, ReferrerMS is either a StackSpaceRegion or GlobalSpaceRegion, + /// and Referred has StackSpaceRegion that is the same as PoppedFrame. + bool isDanglingStackVariable(const MemRegion *Referrer, const MemSpaceRegion *ReferrerMemSpace, + const MemRegion *Referred) { + // Case 1: The referrer is a global and Referred is in the current stack context + if (isa(ReferrerMemSpace)) + return true; - if (!ReferrerMemSpace || !ReferredMemSpace) + const auto *ReferredMemSpace = Referred->getMemorySpace()->getAs(); + if (!ReferredMemSpace) return false; - const auto *ReferrerStackSpace = - ReferrerMemSpace->getAs(); - + const auto *ReferrerStackSpace = ReferrerMemSpace->getAs(); if (!ReferrerStackSpace) return false; - if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame(); - ReferredFrame != PoppedFrame) { - return false; - } - + // Case 2: The referrer stack space is a parent of the referred stack space, e.g., + // in case of leaking a local in a lambda to the outer scope if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) { - V.emplace_back(Referrer, Referred); return true; } + + // Case 3: The referrer is a memregion of a stack argument, e.g., an out + // argument, this is a top-level function, and referred is this top level + // stack space if (isa(ReferrerMemSpace) && // Not a simple ptr (int*) but something deeper, e.g. int** isa(Referrer->getBaseRegion()) && ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) { // Output parameter of a top-level function - V.emplace_back(Referrer, Referred); return true; } + return false; } @@ -500,36 +501,23 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, CallBack(CheckerContext &CC, bool TopFrame) : Ctx(CC), PoppedFrame(CC.getStackFrame()), TopFrame(TopFrame) {} - bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region, + bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, SVal Val) override { - recordInInvalidatedRegions(Region); - - const MemRegion *ReferrerMR = getStackOrGlobalSpaceRegion(Region); + recordInInvalidatedRegions(Referrer); - if (!isa_and_nonnull(ReferrerMR)) + const MemSpaceRegion *ReferrerMS = getStackOrGlobalSpaceRegion(Referrer); + if (!ReferrerMS) return true; - SmallVector EscapingStackAddrs; - FindEscapingStackRegions(Ctx, EscapingStackAddrs, Val); + SmallVector PotentialEscapingAddrs; + FindEscapingStackRegions(Ctx, PotentialEscapingAddrs, Val); - for (const MemRegion *Escapee : EscapingStackAddrs) { - V.emplace_back(Region, Escapee); + for (const MemRegion *Escapee : PotentialEscapingAddrs) { + if (isDanglingStackVariable(Referrer, ReferrerMS, Escapee)) + V.emplace_back(Referrer, Escapee); } return true; - // const MemRegion *VR = Val.getAsRegion(); - // if (!VR) - // return true; - - // if (checkForDanglingStackVariable(Region, VR)) - // return true; - - // // Check the globals for the same. - // if (!isa_and_nonnull()) - // return true; - // if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx)) - // V.emplace_back(Region, VR); - // return true; } }; From 78be69dfbccd5d03c0ed5b5128aaa337e3621587 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Thu, 30 Jan 2025 00:10:20 -0600 Subject: [PATCH 06/40] wip --- .../Checkers/StackAddrEscapeChecker.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 7bc99d681802e..439963d744ba9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -261,8 +261,6 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } bool VisitMemRegion(const MemRegion *MR) override { - // llvm::dbgs() << "Visiting region: " << MR << '\n'; - const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); if (SSR && SSR->getStackFrame() == StackFrameContext) EscapingStackRegions.push_back(MR); @@ -270,7 +268,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { } }; -static void FindEscapingStackRegions(CheckerContext &C, SmallVector &EscapedRegionsStorage, SVal SVal) { +static void FindEscapingStackRegions(SmallVector &EscapedRegionsStorage, CheckerContext &C, SVal SVal) { FindStackRegionsSymbolVisitor Finder(C, EscapedRegionsStorage); ScanReachableSymbols Scanner(C.getState(), Finder); Scanner.scan(SVal); @@ -326,7 +324,7 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SmallVector EscapedRegions; - FindEscapingStackRegions(C, EscapedRegions, V); + FindEscapingStackRegions(EscapedRegions, C, V); FilterReturnExpressionLeaks(EscapedRegions, C, RetE, V); for (const MemRegion *ER : EscapedRegions) { @@ -463,9 +461,8 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, // Case 2: The referrer stack space is a parent of the referred stack space, e.g., // in case of leaking a local in a lambda to the outer scope - if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) { + if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) return true; - } // Case 3: The referrer is a memregion of a stack argument, e.g., an out // argument, this is a top-level function, and referred is this top level @@ -503,6 +500,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, SVal Val) override { + llvm::dbgs() << "HandleBinding: (" << Referrer << ", " << Val << ")\n"; recordInInvalidatedRegions(Referrer); const MemSpaceRegion *ReferrerMS = getStackOrGlobalSpaceRegion(Referrer); @@ -510,7 +508,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, return true; SmallVector PotentialEscapingAddrs; - FindEscapingStackRegions(Ctx, PotentialEscapingAddrs, Val); + FindEscapingStackRegions(PotentialEscapingAddrs, Ctx, Val); for (const MemRegion *Escapee : PotentialEscapingAddrs) { if (isDanglingStackVariable(Referrer, ReferrerMS, Escapee)) From 21e5c9d27cf1c6048dafde2c78feae6a53d4ab7a Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 18:27:08 -0600 Subject: [PATCH 07/40] wip, fixed lambdas test failures --- .../Checkers/StackAddrEscapeChecker.cpp | 135 ++++++++++++------ 1 file changed, 89 insertions(+), 46 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 439963d744ba9..032caf0a19f54 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -213,29 +213,29 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( } -void StackAddrEscapeChecker::checkReturnedBlockCaptures( - const BlockDataRegion &B, CheckerContext &C) const { - for (const MemRegion *Region : getCapturedStackRegions(B, C)) { - if (isNotInCurrentFrame(Region, C)) - continue; - ExplodedNode *N = C.generateNonFatalErrorNode(); - if (!N) - continue; - if (!BT_capturedstackret) - BT_capturedstackret = std::make_unique( - CheckNames[CK_StackAddrEscapeChecker], - "Address of stack-allocated memory is captured"); - SmallString<128> Buf; - llvm::raw_svector_ostream Out(Buf); - SourceRange Range = genName(Out, Region, C.getASTContext()); - Out << " is captured by a returned block"; - auto Report = std::make_unique(*BT_capturedstackret, - Out.str(), N); - if (Range.isValid()) - Report->addRange(Range); - C.emitReport(std::move(Report)); - } -} +// void StackAddrEscapeChecker::checkReturnedBlockCaptures( +// const BlockDataRegion &B, CheckerContext &C) const { +// for (const MemRegion *Region : getCapturedStackRegions(B, C)) { +// if (isNotInCurrentFrame(Region, C)) +// continue; +// ExplodedNode *N = C.generateNonFatalErrorNode(); +// if (!N) +// continue; +// if (!BT_capturedstackret) +// BT_capturedstackret = std::make_unique( +// CheckNames[CK_StackAddrEscapeChecker], +// "Address of stack-allocated memory is captured"); +// SmallString<128> Buf; +// llvm::raw_svector_ostream Out(Buf); +// SourceRange Range = genName(Out, Region, C.getASTContext()); +// Out << " is captured by a returned block"; +// auto Report = std::make_unique(*BT_capturedstackret, +// Out.str(), N); +// if (Range.isValid()) +// Report->addRange(Range); +// C.emitReport(std::move(Report)); +// } +// } void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { @@ -264,21 +264,27 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); if (SSR && SSR->getStackFrame() == StackFrameContext) EscapingStackRegions.push_back(MR); - return true; + return true; } }; -static void FindEscapingStackRegions(SmallVector &EscapedRegionsStorage, CheckerContext &C, SVal SVal) { - FindStackRegionsSymbolVisitor Finder(C, EscapedRegionsStorage); - ScanReachableSymbols Scanner(C.getState(), Finder); +static SmallVector FindEscapingStackRegions(CheckerContext &C, SVal SVal) { + SmallVector FoundStackRegions; + + FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions); + ScanReachableSymbols Scanner(C.getState(), Finder); Scanner.scan(SVal); + + return FoundStackRegions; } -static void FilterReturnExpressionLeaks(SmallVector &MaybeEscaped, +static SmallVector FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, CheckerContext &C, const Expr *RetE, SVal &RetVal) { + SmallVector WillEscape; + // Returning a record by value is fine. (In this case, the returned // expression will be a copy-constructor, possibly wrapped in an // ExprWithCleanups node.) @@ -297,18 +303,59 @@ static void FilterReturnExpressionLeaks(SmallVector &MaybeEsc ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject; } - // Assuming MR is never nullptr - auto ShouldNotEmitError = [=](const MemRegion *MR) -> bool { - // If this particular MR is one of these special cases, then - // don't emit an error for this MR, but this still allows emitting these - // errors for MRs captured by, e.g., the temporary object. - if (RetRegion == MR) { - return IsConstructExpr || IsCopyAndAutoreleaseBlockObj;; + for (const MemRegion *MR : MaybeEscaped) { + if (IsCopyAndAutoreleaseBlockObj) { + if (MR == RetRegion) + continue; + + const SubRegion *SR = dyn_cast(MR); + if (SR && SR->isSubRegionOf(RetRegion)) + continue; } - return false; - }; - std::ignore = std::remove_if(MaybeEscaped.begin(), MaybeEscaped.end(), ShouldNotEmitError); + if (RetRegion == MR && (IsConstructExpr || isa(MR))) + continue; + + if (isa(MR->getBaseRegion())) + continue; + + WillEscape.push_back(MR); + } + + return WillEscape; + + // // Assuming MR is never nullptr + // auto ShouldNotEmitError = [=](const MemRegion *MR) -> bool { + // // If we are checking the region that is returned (compared to + // // a region stored somewhere deeper in the return value), then + // // there are some potential false positives with reporting this + // // returned region: + // // 1. If we are returning a temp obj in an inlined call, back to this + // // same stack frame context: + // if (IsCopyAndAutoreleaseBlockObj) { + // if (MR == RetRegion) + // return true; + + // const SubRegion *SR = dyn_cast(MR); + // if (SR && SR->isSubRegionOf(RetRegion)) + // return true; + // } + + // if (RetRegion == MR) { + // return IsConstructExpr || isa(MR); + // } + + // if (isa(MR->getBaseRegion())) + // return true; + + // return false; + // }; + + // const auto NewEndIter = std::remove_if(MaybeEscaped.begin(), MaybeEscaped.end(), ShouldNotEmitError); + + // llvm::dbgs() << "New num escaped regions: " << std::distance(MaybeEscaped.begin(), NewEndIter) << '\n'; + + // return SmallVector() } void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { @@ -322,10 +369,9 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SVal V = C.getSVal(RetE); - SmallVector EscapedRegions; + SmallVector MaybeEscapedRegions = FindEscapingStackRegions(C, V); - FindEscapingStackRegions(EscapedRegions, C, V); - FilterReturnExpressionLeaks(EscapedRegions, C, RetE, V); + SmallVector EscapedRegions = FilterReturnExpressionLeaks(MaybeEscapedRegions, C, RetE, V); for (const MemRegion *ER : EscapedRegions) { EmitStackError(C, ER, RetE); @@ -498,17 +544,14 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, CallBack(CheckerContext &CC, bool TopFrame) : Ctx(CC), PoppedFrame(CC.getStackFrame()), TopFrame(TopFrame) {} - bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, - SVal Val) override { - llvm::dbgs() << "HandleBinding: (" << Referrer << ", " << Val << ")\n"; + bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, SVal Val) override { recordInInvalidatedRegions(Referrer); const MemSpaceRegion *ReferrerMS = getStackOrGlobalSpaceRegion(Referrer); if (!ReferrerMS) return true; - SmallVector PotentialEscapingAddrs; - FindEscapingStackRegions(PotentialEscapingAddrs, Ctx, Val); + SmallVector PotentialEscapingAddrs = FindEscapingStackRegions(Ctx, Val); for (const MemRegion *Escapee : PotentialEscapingAddrs) { if (isDanglingStackVariable(Referrer, ReferrerMS, Escapee)) From a0bcc4d00e38c4668388d73b741afbe8f2e51de7 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 18:27:22 -0600 Subject: [PATCH 08/40] formatting --- .../Checkers/StackAddrEscapeChecker.cpp | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 032caf0a19f54..1e2801faefe0b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -212,7 +212,6 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( } } - // void StackAddrEscapeChecker::checkReturnedBlockCaptures( // const BlockDataRegion &B, CheckerContext &C) const { // for (const MemRegion *Region : getCapturedStackRegions(B, C)) { @@ -229,7 +228,8 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( // llvm::raw_svector_ostream Out(Buf); // SourceRange Range = genName(Out, Region, C.getASTContext()); // Out << " is captured by a returned block"; -// auto Report = std::make_unique(*BT_capturedstackret, +// auto Report = +// std::make_unique(*BT_capturedstackret, // Out.str(), N); // if (Range.isValid()) // Report->addRange(Range); @@ -268,20 +268,20 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { } }; -static SmallVector FindEscapingStackRegions(CheckerContext &C, SVal SVal) { +static SmallVector +FindEscapingStackRegions(CheckerContext &C, SVal SVal) { SmallVector FoundStackRegions; FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions); - ScanReachableSymbols Scanner(C.getState(), Finder); + ScanReachableSymbols Scanner(C.getState(), Finder); Scanner.scan(SVal); return FoundStackRegions; } -static SmallVector FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, - CheckerContext &C, - const Expr *RetE, - SVal &RetVal) { +static SmallVector +FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, + CheckerContext &C, const Expr *RetE, SVal &RetVal) { SmallVector WillEscape; @@ -351,9 +351,11 @@ static SmallVector FilterReturnExpressionLeaks(const SmallVec // return false; // }; - // const auto NewEndIter = std::remove_if(MaybeEscaped.begin(), MaybeEscaped.end(), ShouldNotEmitError); + // const auto NewEndIter = std::remove_if(MaybeEscaped.begin(), + // MaybeEscaped.end(), ShouldNotEmitError); - // llvm::dbgs() << "New num escaped regions: " << std::distance(MaybeEscaped.begin(), NewEndIter) << '\n'; + // llvm::dbgs() << "New num escaped regions: " << + // std::distance(MaybeEscaped.begin(), NewEndIter) << '\n'; // return SmallVector() } @@ -369,9 +371,11 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SVal V = C.getSVal(RetE); - SmallVector MaybeEscapedRegions = FindEscapingStackRegions(C, V); + SmallVector MaybeEscapedRegions = + FindEscapingStackRegions(C, V); - SmallVector EscapedRegions = FilterReturnExpressionLeaks(MaybeEscapedRegions, C, RetE, V); + SmallVector EscapedRegions = + FilterReturnExpressionLeaks(MaybeEscapedRegions, C, RetE, V); for (const MemRegion *ER : EscapedRegions) { EmitStackError(C, ER, RetE); @@ -544,14 +548,16 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, CallBack(CheckerContext &CC, bool TopFrame) : Ctx(CC), PoppedFrame(CC.getStackFrame()), TopFrame(TopFrame) {} - bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, SVal Val) override { + bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, + SVal Val) override { recordInInvalidatedRegions(Referrer); const MemSpaceRegion *ReferrerMS = getStackOrGlobalSpaceRegion(Referrer); if (!ReferrerMS) return true; - SmallVector PotentialEscapingAddrs = FindEscapingStackRegions(Ctx, Val); + SmallVector PotentialEscapingAddrs = + FindEscapingStackRegions(Ctx, Val); for (const MemRegion *Escapee : PotentialEscapingAddrs) { if (isDanglingStackVariable(Referrer, ReferrerMS, Escapee)) From a94ccd60d08c40a4c4a3dd54f64c44db63d59d9e Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 18:47:26 -0600 Subject: [PATCH 09/40] some cleanup --- .../Checkers/StackAddrEscapeChecker.cpp | 45 +++---------------- 1 file changed, 5 insertions(+), 40 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 1e2801faefe0b..b8879ae5ab526 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -313,51 +313,16 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, continue; } - if (RetRegion == MR && (IsConstructExpr || isa(MR))) - continue; + // if (RetRegion == MR && (IsConstructExpr || isa(MR))) + // continue; - if (isa(MR->getBaseRegion())) - continue; + // if (isa(MR->getBaseRegion())) + // continue; WillEscape.push_back(MR); } return WillEscape; - - // // Assuming MR is never nullptr - // auto ShouldNotEmitError = [=](const MemRegion *MR) -> bool { - // // If we are checking the region that is returned (compared to - // // a region stored somewhere deeper in the return value), then - // // there are some potential false positives with reporting this - // // returned region: - // // 1. If we are returning a temp obj in an inlined call, back to this - // // same stack frame context: - // if (IsCopyAndAutoreleaseBlockObj) { - // if (MR == RetRegion) - // return true; - - // const SubRegion *SR = dyn_cast(MR); - // if (SR && SR->isSubRegionOf(RetRegion)) - // return true; - // } - - // if (RetRegion == MR) { - // return IsConstructExpr || isa(MR); - // } - - // if (isa(MR->getBaseRegion())) - // return true; - - // return false; - // }; - - // const auto NewEndIter = std::remove_if(MaybeEscaped.begin(), - // MaybeEscaped.end(), ShouldNotEmitError); - - // llvm::dbgs() << "New num escaped regions: " << - // std::distance(MaybeEscaped.begin(), NewEndIter) << '\n'; - - // return SmallVector() } void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { @@ -595,7 +560,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, // Generate a report for this bug. const StringRef CommonSuffix = - " upon returning to the caller. This will be a dangling reference"; + " upon returning to the caller. This will be a dangling reference"; SmallString<128> Buf; llvm::raw_svector_ostream Out(Buf); const SourceRange Range = genName(Out, Referred, Ctx.getASTContext()); From b72b98cd31a9a121c48098ef1f27a0eab0f02bf1 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 19:16:11 -0600 Subject: [PATCH 10/40] emit returned by part added --- .../Checkers/StackAddrEscapeChecker.cpp | 119 ++++++++---------- 1 file changed, 54 insertions(+), 65 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index b8879ae5ab526..5d31c4410fe3d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -57,8 +57,7 @@ class StackAddrEscapeChecker CheckerContext &C) const; void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B, CheckerContext &C) const; - void EmitStackError(CheckerContext &C, const MemRegion *R, - const Expr *RetE) const; + void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion, const Expr *RetE) const; bool isSemaphoreCaptured(const BlockDecl &B) const; static SourceRange genName(raw_ostream &os, const MemRegion *R, ASTContext &Ctx); @@ -150,7 +149,19 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B, return Regions; } -void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, +static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, + SVal ReturnedVal, + const MemRegion *LeakedRegion) { + if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) { + if (isa(ReturnedRegion)) + OS << " is captured by a returned block"; + } else { + // Generic message + OS << " returned to caller"; + } +} + +void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C, const MemRegion *R, const Expr *RetE) const { ExplodedNode *N = C.generateNonFatalErrorNode(); @@ -160,11 +171,15 @@ void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, BT_returnstack = std::make_unique( CheckNames[CK_StackAddrEscapeChecker], "Return of address to stack-allocated memory"); + // Generate a report for this bug. SmallString<128> buf; llvm::raw_svector_ostream os(buf); + + // Error message formatting SourceRange range = genName(os, R, C.getASTContext()); - os << " returned to caller"; + EmitReturnedAsPartOfError(os, C.getSVal(RetE), R); + auto report = std::make_unique(*BT_returnstack, os.str(), N); report->addRange(RetE->getSourceRange()); @@ -212,31 +227,6 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( } } -// void StackAddrEscapeChecker::checkReturnedBlockCaptures( -// const BlockDataRegion &B, CheckerContext &C) const { -// for (const MemRegion *Region : getCapturedStackRegions(B, C)) { -// if (isNotInCurrentFrame(Region, C)) -// continue; -// ExplodedNode *N = C.generateNonFatalErrorNode(); -// if (!N) -// continue; -// if (!BT_capturedstackret) -// BT_capturedstackret = std::make_unique( -// CheckNames[CK_StackAddrEscapeChecker], -// "Address of stack-allocated memory is captured"); -// SmallString<128> Buf; -// llvm::raw_svector_ostream Out(Buf); -// SourceRange Range = genName(Out, Region, C.getASTContext()); -// Out << " is captured by a returned block"; -// auto Report = -// std::make_unique(*BT_capturedstackret, -// Out.str(), N); -// if (Range.isValid()) -// Report->addRange(Range); -// C.emitReport(std::move(Report)); -// } -// } - void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker]) @@ -342,9 +332,8 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SmallVector EscapedRegions = FilterReturnExpressionLeaks(MaybeEscapedRegions, C, RetE, V); - for (const MemRegion *ER : EscapedRegions) { - EmitStackError(C, ER, RetE); - } + for (const MemRegion *ER : EscapedRegions) + EmitReturnLeakError(C, ER, RetE); } static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) { @@ -447,8 +436,8 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, Node = Node->getFirstPred(); } - // Iterate over all bindings to global variables and stack arguments - // see if they contain a memory region in the current stack frame. + // Iterate over all bindings to global variables and see if it contains + // a memory region in the stack space. class CallBack : public StoreManager::BindingsHandler { private: CheckerContext &Ctx; @@ -458,38 +447,38 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, /// Look for stack variables referring to popped stack variables. /// Returns true only if it found some dangling stack variables /// referred by an other stack variable from different stack frame. - /// Here, ReferrerMS is either a StackSpaceRegion or GlobalSpaceRegion, - /// and Referred has StackSpaceRegion that is the same as PoppedFrame. - bool isDanglingStackVariable(const MemRegion *Referrer, const MemSpaceRegion *ReferrerMemSpace, - const MemRegion *Referred) { - // Case 1: The referrer is a global and Referred is in the current stack context - if (isa(ReferrerMemSpace)) - return true; + bool checkForDanglingStackVariable(const MemRegion *Referrer, + const MemRegion *Referred) { + const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer); + const auto *ReferredMemSpace = + Referred->getMemorySpace()->getAs(); - const auto *ReferredMemSpace = Referred->getMemorySpace()->getAs(); - if (!ReferredMemSpace) + if (!ReferrerMemSpace || !ReferredMemSpace) return false; - const auto *ReferrerStackSpace = ReferrerMemSpace->getAs(); + const auto *ReferrerStackSpace = + ReferrerMemSpace->getAs(); + if (!ReferrerStackSpace) return false; - // Case 2: The referrer stack space is a parent of the referred stack space, e.g., - // in case of leaking a local in a lambda to the outer scope - if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) - return true; + if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame(); + ReferredFrame != PoppedFrame) { + return false; + } - // Case 3: The referrer is a memregion of a stack argument, e.g., an out - // argument, this is a top-level function, and referred is this top level - // stack space + if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) { + V.emplace_back(Referrer, Referred); + return true; + } if (isa(ReferrerMemSpace) && // Not a simple ptr (int*) but something deeper, e.g. int** isa(Referrer->getBaseRegion()) && ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) { // Output parameter of a top-level function + V.emplace_back(Referrer, Referred); return true; } - return false; } @@ -513,22 +502,22 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, CallBack(CheckerContext &CC, bool TopFrame) : Ctx(CC), PoppedFrame(CC.getStackFrame()), TopFrame(TopFrame) {} - bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Referrer, + bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region, SVal Val) override { - recordInInvalidatedRegions(Referrer); - - const MemSpaceRegion *ReferrerMS = getStackOrGlobalSpaceRegion(Referrer); - if (!ReferrerMS) + recordInInvalidatedRegions(Region); + const MemRegion *VR = Val.getAsRegion(); + if (!VR) return true; - SmallVector PotentialEscapingAddrs = - FindEscapingStackRegions(Ctx, Val); - - for (const MemRegion *Escapee : PotentialEscapingAddrs) { - if (isDanglingStackVariable(Referrer, ReferrerMS, Escapee)) - V.emplace_back(Referrer, Escapee); - } + if (checkForDanglingStackVariable(Region, VR)) + return true; + // Check the globals for the same. + if (!isa_and_nonnull( + getStackOrGlobalSpaceRegion(Region))) + return true; + if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx)) + V.emplace_back(Region, VR); return true; } }; @@ -560,7 +549,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, // Generate a report for this bug. const StringRef CommonSuffix = - " upon returning to the caller. This will be a dangling reference"; + " upon returning to the caller. This will be a dangling reference"; SmallString<128> Buf; llvm::raw_svector_ostream Out(Buf); const SourceRange Range = genName(Out, Referred, Ctx.getASTContext()); From b78b96cb172b3b7faadff6043f276a1deab780ca Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 19:49:15 -0600 Subject: [PATCH 11/40] add back IsConstructExpr bool in filtering --- .../Checkers/StackAddrEscapeChecker.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 5d31c4410fe3d..6983e5baefa54 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -153,12 +153,14 @@ static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, SVal ReturnedVal, const MemRegion *LeakedRegion) { if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) { - if (isa(ReturnedRegion)) + if (isa(ReturnedRegion)) { OS << " is captured by a returned block"; - } else { - // Generic message - OS << " returned to caller"; + return; + } } + + // Generic message + OS << " returned to caller"; } void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C, @@ -294,6 +296,8 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, } for (const MemRegion *MR : MaybeEscaped) { + // In this case, the block_data region being returned isn't a leak, + // and all of its subregions are not leaks if (IsCopyAndAutoreleaseBlockObj) { if (MR == RetRegion) continue; @@ -303,11 +307,8 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, continue; } - // if (RetRegion == MR && (IsConstructExpr || isa(MR))) - // continue; - - // if (isa(MR->getBaseRegion())) - // continue; + if (RetRegion == MR && IsConstructExpr) + continue; WillEscape.push_back(MR); } From a078753487d3366f5c0a83e4c6b919a24191e252 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sun, 2 Feb 2025 22:33:29 -0600 Subject: [PATCH 12/40] wip --- .../Checkers/StackAddrEscapeChecker.cpp | 21 ++++++++++++++----- clang/test/Analysis/stack-addr-ps.cpp | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 6983e5baefa54..dab14aa88e975 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -244,16 +244,27 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, } class FindStackRegionsSymbolVisitor final : public SymbolVisitor { + CheckerContext &Ctxt; const StackFrameContext *StackFrameContext; SmallVector &EscapingStackRegions; public: explicit FindStackRegionsSymbolVisitor(CheckerContext &Ctxt, SmallVector &StorageForStackRegions) - : StackFrameContext(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} + : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} bool VisitSymbol(SymbolRef sym) override { return true; } bool VisitMemRegion(const MemRegion *MR) override { - const StackSpaceRegion *SSR = dyn_cast(MR->getMemorySpace()); + if (const BlockDataRegion *BDR = MR->getAs()) { + for (auto Var : BDR->referenced_vars()) { + SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); + const MemRegion *Region = Val.getAsRegion(); + if (Region && isa(Region->getMemorySpace())) + EscapingStackRegions.push_back(Region); + } + return false; + } + + const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); if (SSR && SSR->getStackFrame() == StackFrameContext) EscapingStackRegions.push_back(MR); return true; @@ -302,9 +313,9 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, if (MR == RetRegion) continue; - const SubRegion *SR = dyn_cast(MR); - if (SR && SR->isSubRegionOf(RetRegion)) - continue; + // const SubRegion *SR = dyn_cast(MR); + // if (SR && SR->isSubRegionOf(RetRegion)) + // continue; } if (RetRegion == MR && IsConstructExpr) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index cfa887da025a8..8413757b57a92 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -251,7 +251,7 @@ void* lambda_to_context_direct_pointer_uncalled() { int local = 42; p = &local; // no-warning: analyzed only as top-level, ignored explicitly by the checker }; - return new MyFunction(&lambda); + return new MyFunction(&lambda); // expected-warning{{Address of stack memory associated with local variable 'lambda' returned to caller}} } void lambda_to_context_direct_pointer_lifetime_extended() { From 891f5fd0bf9a8bc2168e9ca60b7461beb676d960 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 12:45:06 -0600 Subject: [PATCH 13/40] cleanup --- .../Checkers/StackAddrEscapeChecker.cpp | 56 ++++++++----------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index dab14aa88e975..cb8dfcad21cbc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -258,8 +258,10 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { for (auto Var : BDR->referenced_vars()) { SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); const MemRegion *Region = Val.getAsRegion(); - if (Region && isa(Region->getMemorySpace())) + if (Region && isa(Region->getMemorySpace())) { EscapingStackRegions.push_back(Region); + VisitMemRegion(Region); + } } return false; } @@ -271,23 +273,14 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { } }; -static SmallVector -FindEscapingStackRegions(CheckerContext &C, SVal SVal) { - SmallVector FoundStackRegions; - - FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions); - ScanReachableSymbols Scanner(C.getState(), Finder); - Scanner.scan(SVal); - - return FoundStackRegions; -} - static SmallVector FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, CheckerContext &C, const Expr *RetE, SVal &RetVal) { SmallVector WillEscape; + const MemRegion *RetRegion = RetVal.getAsRegion(); + // Returning a record by value is fine. (In this case, the returned // expression will be a copy-constructor, possibly wrapped in an // ExprWithCleanups node.) @@ -295,30 +288,17 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, RetE = Cleanup->getSubExpr(); bool IsConstructExpr = isa(RetE) && RetE->getType()->isRecordType(); - const MemRegion *RetRegion = RetVal.getAsRegion(); - // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied // so the stack address is not escaping here. bool IsCopyAndAutoreleaseBlockObj = false; if (const auto *ICE = dyn_cast(RetE)) { - IsCopyAndAutoreleaseBlockObj = RetRegion && - isa(RetRegion) && + IsCopyAndAutoreleaseBlockObj = + isa_and_nonnull(RetRegion) && ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject; } for (const MemRegion *MR : MaybeEscaped) { - // In this case, the block_data region being returned isn't a leak, - // and all of its subregions are not leaks - if (IsCopyAndAutoreleaseBlockObj) { - if (MR == RetRegion) - continue; - - // const SubRegion *SR = dyn_cast(MR); - // if (SR && SR->isSubRegionOf(RetRegion)) - // continue; - } - - if (RetRegion == MR && IsConstructExpr) + if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr)) continue; WillEscape.push_back(MR); @@ -327,6 +307,17 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, return WillEscape; } +static SmallVector +FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) { + SmallVector FoundStackRegions; + + FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions); + ScanReachableSymbols Scanner(C.getState(), Finder); + Scanner.scan(RetVal); + + return FilterReturnExpressionLeaks(FoundStackRegions, C, RetE, RetVal); +} + void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) return; @@ -338,13 +329,10 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SVal V = C.getSVal(RetE); - SmallVector MaybeEscapedRegions = - FindEscapingStackRegions(C, V); - - SmallVector EscapedRegions = - FilterReturnExpressionLeaks(MaybeEscapedRegions, C, RetE, V); + SmallVector EscapedStackRegions = + FindEscapingStackRegions(C, RetE, V); - for (const MemRegion *ER : EscapedRegions) + for (const MemRegion *ER : EscapedStackRegions) EmitReturnLeakError(C, ER, RetE); } From 6a7cb9641ef1de7e65e004adf83c1a4b0a80c69c Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 17:17:31 -0600 Subject: [PATCH 14/40] fix objc stack-allocated-capturing block return --- .../Checkers/StackAddrEscapeChecker.cpp | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index cb8dfcad21cbc..4d0f5dbc896ab 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -253,22 +253,27 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } - bool VisitMemRegion(const MemRegion *MR) override { - if (const BlockDataRegion *BDR = MR->getAs()) { - for (auto Var : BDR->referenced_vars()) { - SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); - const MemRegion *Region = Val.getAsRegion(); - if (Region && isa(Region->getMemorySpace())) { - EscapingStackRegions.push_back(Region); - VisitMemRegion(Region); - } + bool VisitBlockDataRegion(const BlockDataRegion *BDR) { + for (auto Var : BDR->referenced_vars()) { + SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); + const MemRegion *Region = Val.getAsRegion(); + if (Region && isa(Region->getMemorySpace())) { + EscapingStackRegions.push_back(Region); + VisitMemRegion(Region); } - return false; } + return false; + } + + bool VisitMemRegion(const MemRegion *MR) override { const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); if (SSR && SSR->getStackFrame() == StackFrameContext) EscapingStackRegions.push_back(MR); + + if (const BlockDataRegion *BDR = MR->getAs()) + return VisitBlockDataRegion(BDR); + return true; } }; @@ -330,7 +335,7 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SVal V = C.getSVal(RetE); SmallVector EscapedStackRegions = - FindEscapingStackRegions(C, RetE, V); + FindEscapingStackRegions(C, RetE, V); for (const MemRegion *ER : EscapedStackRegions) EmitReturnLeakError(C, ER, RetE); From 6d29efbee22f6b788906aa424816c6d43e6bd142 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 18:12:28 -0600 Subject: [PATCH 15/40] why does this work --- .../StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 4d0f5dbc896ab..b64a5b4214d02 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -253,7 +253,8 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } - bool VisitBlockDataRegion(const BlockDataRegion *BDR) { + /// Visits the captured region values + bool VisitBlockDataRegionCaptures(const BlockDataRegion *BDR) { for (auto Var : BDR->referenced_vars()) { SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); const MemRegion *Region = Val.getAsRegion(); @@ -272,7 +273,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { EscapingStackRegions.push_back(MR); if (const BlockDataRegion *BDR = MR->getAs()) - return VisitBlockDataRegion(BDR); + return VisitBlockDataRegionCaptures(BDR); return true; } @@ -306,6 +307,10 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr)) continue; + if (const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs()) + if (SSR->getStackFrame() != C.getStackFrame()) + continue; + WillEscape.push_back(MR); } From 296555e3ef2430fc9b71ef67d3c220e69ea0c22f Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 18:31:59 -0600 Subject: [PATCH 16/40] double up these test expectations --- clang/test/Analysis/stack-addr-ps.cpp | 2 +- clang/test/Analysis/stackaddrleak.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 8413757b57a92..392982d92a3f1 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -532,7 +532,7 @@ S returned_struct_with_ptr_callee() { int local = 42; S s; s.p = &local; - return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} + return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} expected-warning{{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 's' upon returning to the caller. This will be a dangling reference}} } S leak_through_field_of_returned_object() { diff --git a/clang/test/Analysis/stackaddrleak.cpp b/clang/test/Analysis/stackaddrleak.cpp index 733a86be1091a..a44fb1f7d2dd1 100644 --- a/clang/test/Analysis/stackaddrleak.cpp +++ b/clang/test/Analysis/stackaddrleak.cpp @@ -18,7 +18,7 @@ struct myfunction { myfunction create_func() { int n; auto c = [&n] {}; - return c; // expected-warning {{Address of stack memory associated with local variable 'n' returned to caller}} + return c; // expected-warning {{Address of stack memory associated with local variable 'n' returned to caller}} expected-warning{{Address of stack memory associated with local variable 'n' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference}} } void gh_66221() { create_func()(); From 932c14fc3537988771ee8b86e213b3c21da93e2e Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 20:24:37 -0600 Subject: [PATCH 17/40] check everytime visitor adds to escaped region --- .../Checkers/StackAddrEscapeChecker.cpp | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index b64a5b4214d02..d18e551d6497f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -253,30 +253,34 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { bool VisitSymbol(SymbolRef sym) override { return true; } - /// Visits the captured region values + bool VisitMemRegion(const MemRegion *MR) override { + SaveIfEscapes(MR); + + if (const BlockDataRegion *BDR = MR->getAs()) + return VisitBlockDataRegionCaptures(BDR); + + return true; + } + +private: + void SaveIfEscapes(const MemRegion *MR) { + const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); + if (SSR && SSR->getStackFrame() == StackFrameContext) + EscapingStackRegions.push_back(MR); + } + bool VisitBlockDataRegionCaptures(const BlockDataRegion *BDR) { for (auto Var : BDR->referenced_vars()) { SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion()); const MemRegion *Region = Val.getAsRegion(); - if (Region && isa(Region->getMemorySpace())) { - EscapingStackRegions.push_back(Region); + if (Region) { + SaveIfEscapes(Region); VisitMemRegion(Region); } } return false; } - - bool VisitMemRegion(const MemRegion *MR) override { - const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); - if (SSR && SSR->getStackFrame() == StackFrameContext) - EscapingStackRegions.push_back(MR); - - if (const BlockDataRegion *BDR = MR->getAs()) - return VisitBlockDataRegionCaptures(BDR); - - return true; - } }; static SmallVector From 9d8fc1a2a588ca1491d0601c54904b51306fbf78 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 22:57:45 -0600 Subject: [PATCH 18/40] fix comments --- .../Checkers/StackAddrEscapeChecker.cpp | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index d18e551d6497f..919ef932b40c2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -243,6 +243,13 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, } } +/// A visitor made for use with a ScanReachableSymbols scanner, used +/// for finding stack regions within an SVal that live on the current +/// stack frame of the given checker context. This visitor excludes +/// NonParamVarRegion that data is bound to in a BlockDataRegion's +/// bindings, since these are likely uninteresting, e.g., in case a +/// temporary is constructed on the stack, but it captures values +/// that would leak. class FindStackRegionsSymbolVisitor final : public SymbolVisitor { CheckerContext &Ctxt; const StackFrameContext *StackFrameContext; @@ -283,6 +290,11 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { } }; +/// Given some memory regions that are flagged by FindStackRegionsSymbolVisitor, +/// this function filters out memory regions that are being returned that are +/// likely not true leaks: +/// 1. If returning a block data region that has stack memory space +/// 2. If returning a constructed object that has stack memory space static SmallVector FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, CheckerContext &C, const Expr *RetE, SVal &RetVal) { @@ -311,9 +323,10 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr)) continue; - if (const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs()) - if (SSR->getStackFrame() != C.getStackFrame()) - continue; + // If this is a construct expr of an unelided return value copy, then don't + // warn about returning a region that currently lives on the stack. + if (IsConstructExpr && RetVal.getAs() && isa(MR)) + continue; WillEscape.push_back(MR); } @@ -321,6 +334,8 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, return WillEscape; } +/// For use in finding regions that live on the checker context's current +/// stack frame, deep in the SVal representing the return value. static SmallVector FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) { SmallVector FoundStackRegions; From f909ae127d2814a295980d0e03eb6a014ca6d68e Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 23:05:13 -0600 Subject: [PATCH 19/40] formatting --- .../Checkers/StackAddrEscapeChecker.cpp | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 919ef932b40c2..2b1bd97c78743 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -29,8 +29,7 @@ using namespace ento; namespace { class StackAddrEscapeChecker - : public Checker, + : public Checker, check::EndFunction> { mutable IdentifierInfo *dispatch_semaphore_tII = nullptr; mutable std::unique_ptr BT_stackleak; @@ -57,7 +56,8 @@ class StackAddrEscapeChecker CheckerContext &C) const; void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B, CheckerContext &C) const; - void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion, const Expr *RetE) const; + void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion, + const Expr *RetE) const; bool isSemaphoreCaptured(const BlockDecl &B) const; static SourceRange genName(raw_ostream &os, const MemRegion *R, ASTContext &Ctx); @@ -149,8 +149,7 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B, return Regions; } -static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, - SVal ReturnedVal, +static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, SVal ReturnedVal, const MemRegion *LeakedRegion) { if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) { if (isa(ReturnedRegion)) { @@ -164,8 +163,8 @@ static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, } void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C, - const MemRegion *R, - const Expr *RetE) const { + const MemRegion *R, + const Expr *RetE) const { ExplodedNode *N = C.generateNonFatalErrorNode(); if (!N) return; @@ -243,26 +242,30 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, } } -/// A visitor made for use with a ScanReachableSymbols scanner, used +/// A visitor made for use with a ScanReachableSymbols scanner, used /// for finding stack regions within an SVal that live on the current /// stack frame of the given checker context. This visitor excludes /// NonParamVarRegion that data is bound to in a BlockDataRegion's /// bindings, since these are likely uninteresting, e.g., in case a /// temporary is constructed on the stack, but it captures values -/// that would leak. +/// that would leak. class FindStackRegionsSymbolVisitor final : public SymbolVisitor { CheckerContext &Ctxt; const StackFrameContext *StackFrameContext; SmallVector &EscapingStackRegions; + public: - explicit FindStackRegionsSymbolVisitor(CheckerContext &Ctxt, SmallVector &StorageForStackRegions) - : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} + explicit FindStackRegionsSymbolVisitor( + CheckerContext &Ctxt, + SmallVector &StorageForStackRegions) + : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()), + EscapingStackRegions(StorageForStackRegions) {} bool VisitSymbol(SymbolRef sym) override { return true; } bool VisitMemRegion(const MemRegion *MR) override { SaveIfEscapes(MR); - + if (const BlockDataRegion *BDR = MR->getAs()) return VisitBlockDataRegionCaptures(BDR); @@ -271,7 +274,8 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { private: void SaveIfEscapes(const MemRegion *MR) { - const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); + const StackSpaceRegion *SSR = + MR->getMemorySpace()->getAs(); if (SSR && SSR->getStackFrame() == StackFrameContext) EscapingStackRegions.push_back(MR); } @@ -308,15 +312,16 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, // ExprWithCleanups node.) if (const ExprWithCleanups *Cleanup = dyn_cast(RetE)) RetE = Cleanup->getSubExpr(); - bool IsConstructExpr = isa(RetE) && RetE->getType()->isRecordType(); + bool IsConstructExpr = + isa(RetE) && RetE->getType()->isRecordType(); // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied // so the stack address is not escaping here. bool IsCopyAndAutoreleaseBlockObj = false; if (const auto *ICE = dyn_cast(RetE)) { IsCopyAndAutoreleaseBlockObj = - isa_and_nonnull(RetRegion) && - ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject; + isa_and_nonnull(RetRegion) && + ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject; } for (const MemRegion *MR : MaybeEscaped) { @@ -325,7 +330,8 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, // If this is a construct expr of an unelided return value copy, then don't // warn about returning a region that currently lives on the stack. - if (IsConstructExpr && RetVal.getAs() && isa(MR)) + if (IsConstructExpr && RetVal.getAs() && + isa(MR)) continue; WillEscape.push_back(MR); @@ -347,7 +353,8 @@ FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) { return FilterReturnExpressionLeaks(FoundStackRegions, C, RetE, RetVal); } -void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { +void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, + CheckerContext &C) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) return; @@ -359,7 +366,7 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext & SVal V = C.getSVal(RetE); SmallVector EscapedStackRegions = - FindEscapingStackRegions(C, RetE, V); + FindEscapingStackRegions(C, RetE, V); for (const MemRegion *ER : EscapedStackRegions) EmitReturnLeakError(C, ER, RetE); From ef7949263cd8c47d1ec4b3ed38bec3517a53c532 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 3 Feb 2025 23:06:23 -0600 Subject: [PATCH 20/40] remove old includes --- clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 2b1bd97c78743..86f0949994cf6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -20,9 +20,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h" #include "llvm/ADT/SmallString.h" -#include "llvm/Support/Debug.h" #include "llvm/Support/raw_ostream.h" using namespace clang; using namespace ento; From cb702c92b06c9a9cfb68ecd9b6d81a8520d6f08b Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Tue, 4 Feb 2025 14:17:27 -0600 Subject: [PATCH 21/40] restore whitespace at end of file --- clang/test/Analysis/stackaddrleak.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/stackaddrleak.cpp b/clang/test/Analysis/stackaddrleak.cpp index a44fb1f7d2dd1..df202a2fee776 100644 --- a/clang/test/Analysis/stackaddrleak.cpp +++ b/clang/test/Analysis/stackaddrleak.cpp @@ -22,4 +22,4 @@ myfunction create_func() { } void gh_66221() { create_func()(); -} \ No newline at end of file +} From d5c123d0a98fbf769c5400bc4de9345f29894606 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Tue, 4 Feb 2025 14:35:25 -0600 Subject: [PATCH 22/40] add basic int* and int return assign expr tests --- clang/test/Analysis/stack-addr-ps.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 392982d92a3f1..cdd6e08f579b6 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -90,6 +90,12 @@ int *mf() { return &x; // expected-warning{{Address of stack memory associated with local variable 's1' returned}} expected-warning {{address of stack memory associated with local variable 's1' returned}} } +int *return_assign_expr_leak() { + int x = 1; + int *y; + return y = &x; // expected-warning{{Address of stack memory associated with local variable 'x' returned}} +} + void *lf() { label: void *const &x = &&label; // expected-note {{binding reference variable 'x' here}} @@ -912,3 +918,14 @@ void top_malloc_no_crash_fn() { free(pptr); } } // namespace alloca_region_pointer + +namespace true_negatives_return_expressions { +void return_void() { + return void(); // no-warning +} + +int return_safe_assign_expr() { + int y, x = 14; + return y = x; // no-warning +} +} \ No newline at end of file From 19cdb1bbe3977b8d48ed80a263e4ded3cff00f50 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Tue, 4 Feb 2025 16:01:55 -0600 Subject: [PATCH 23/40] return comma-separated expression --- clang/test/Analysis/stack-addr-ps.cpp | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index cdd6e08f579b6..edaec8ec4be48 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -96,6 +96,12 @@ int *return_assign_expr_leak() { return y = &x; // expected-warning{{Address of stack memory associated with local variable 'x' returned}} } +// Additional diagnostic from -Wreturn-stack-address in the simple case? +int *return_comma_separated_expressions_leak() { + int x = 1; + return (x=14), &x; // expected-warning{{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning{{address of stack memory associated with local variable 'x' returned}} +} + void *lf() { label: void *const &x = &&label; // expected-note {{binding reference variable 'x' here}} @@ -920,12 +926,26 @@ void top_malloc_no_crash_fn() { } // namespace alloca_region_pointer namespace true_negatives_return_expressions { +struct Container { int *x; }; + void return_void() { return void(); // no-warning } -int return_safe_assign_expr() { +int return_assign_expr_safe() { int y, x = 14; return y = x; // no-warning } + +int return_comma_separated_expressions_safe() { + int x = 1; + int *y; + return (y=&x), (x=15); // no-warning +} + +int return_comma_separated_expressions_container_safe() { + int x = 1; + Container Other; + return Other = Container{&x}, x = 14; // no-warning +} } \ No newline at end of file From cf0a5f94fb7ee3f5f6ed80054bd7df721a0005d8 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Thu, 6 Feb 2025 19:07:36 -0600 Subject: [PATCH 24/40] Add expected warning to copy-elision.cpp no-elide tests --- clang/test/Analysis/copy-elision.cpp | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/clang/test/Analysis/copy-elision.cpp b/clang/test/Analysis/copy-elision.cpp index cd941854fc7f1..e69f52f351f1b 100644 --- a/clang/test/Analysis/copy-elision.cpp +++ b/clang/test/Analysis/copy-elision.cpp @@ -154,20 +154,26 @@ class ClassWithoutDestructor { void push() { v.push(this); } }; +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithoutDestructor make1(AddressVector &v) { - return ClassWithoutDestructor(v); + return ClassWithoutDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithoutDestructor' is still \ referred to by the caller variable 'v' upon returning to the caller}} } +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithoutDestructor make2(AddressVector &v) { - return make1(v); + return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithoutDestructor' is still \ referred to by the caller variable 'v' upon returning to the caller}} } +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithoutDestructor make3(AddressVector &v) { - return make2(v); + return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithoutDestructor' is still \ referred to by the caller variable 'v' upon returning to the caller}} @@ -298,21 +304,26 @@ to by the caller variable 'v' upon returning to the caller}} #endif } - +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithDestructor make1(AddressVector &v) { - return ClassWithDestructor(v); + return ClassWithDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithDestructor' is still referred \ to by the caller variable 'v' upon returning to the caller}} } +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithDestructor make2(AddressVector &v) { - return make1(v); + return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithDestructor' is still referred \ to by the caller variable 'v' upon returning to the caller}} } +// Two warnings on no-elide: arg v holds the address of the temporary, and we +// are returning an object which holds v which holds the address of the temporary ClassWithDestructor make3(AddressVector &v) { - return make2(v); + return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}} // no-elide-warning@-1 {{Address of stack memory associated with temporary \ object of type 'ClassWithDestructor' is still referred \ to by the caller variable 'v' upon returning to the caller}} From 02b24e8fa42b23b39c346688772cb8169c12275d Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Thu, 6 Feb 2025 19:08:58 -0600 Subject: [PATCH 25/40] Add simple true negative test case from code review --- clang/test/Analysis/stack-addr-ps.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index edaec8ec4be48..c691b49602077 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -928,6 +928,11 @@ void top_malloc_no_crash_fn() { namespace true_negatives_return_expressions { struct Container { int *x; }; +int test2() { + int x = 14; + return x; // no-warning +} + void return_void() { return void(); // no-warning } From e3b9ec94a183c626bfc6b0c9f085b3d4b8758303 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Thu, 6 Feb 2025 19:09:33 -0600 Subject: [PATCH 26/40] Remove temp object check in return expr checking --- .../lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 86f0949994cf6..4f8099e1a6f85 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -326,12 +326,6 @@ FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr)) continue; - // If this is a construct expr of an unelided return value copy, then don't - // warn about returning a region that currently lives on the stack. - if (IsConstructExpr && RetVal.getAs() && - isa(MR)) - continue; - WillEscape.push_back(MR); } From 5b67313529ea745bd142a94c37f0f1e5c145a565 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Thu, 6 Feb 2025 19:10:30 -0600 Subject: [PATCH 27/40] Add newline end of file --- clang/test/Analysis/stack-addr-ps.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index c691b49602077..7e5e6842cf22c 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -953,4 +953,4 @@ int return_comma_separated_expressions_container_safe() { Container Other; return Other = Container{&x}, x = 14; // no-warning } -} \ No newline at end of file +} From 6c2425ff430e7d2c863b9e3a795b2e8b52ce5f85 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Fri, 7 Feb 2025 11:45:57 -0600 Subject: [PATCH 28/40] Add testcase: return of unknown symbol on stack --- clang/test/Analysis/stack-addr-ps.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 7e5e6842cf22c..1160b40b1982c 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -953,4 +953,11 @@ int return_comma_separated_expressions_container_safe() { Container Other; return Other = Container{&x}, x = 14; // no-warning } + +int make_x(); +int return_symbol_safe() { + int x = make_x(); + clang_analyzer_dump(x); // expected-warning-re {{conj_$2{int, {{.+}}}}} + return x; // no-warning +} } From 84ce3a12e20a0acc429894e4bd6478e5f36403a1 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Fri, 7 Feb 2025 11:47:38 -0600 Subject: [PATCH 29/40] Add testcase of false negative returning symbolic arg --- clang/test/Analysis/stack-addr-ps.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 1160b40b1982c..f2d6e762c150d 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -352,6 +352,13 @@ void param_ptr_to_ptr_to_ptr_callee(void*** ppp) { **ppp = &local; // expected-warning{{local variable 'local' is still referred to by the caller variable 'pp'}} } +void ***param_ptr_to_ptr_to_ptr_return(void ***ppp) { + int local = 0; + **ppp = &local; + return ppp; + // expected-warning@-1 {{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 'ppp' upon returning to the caller. This will be a dangling reference}} +} + void param_ptr_to_ptr_to_ptr_caller(void** pp) { param_ptr_to_ptr_to_ptr_callee(&pp); } @@ -960,4 +967,8 @@ int return_symbol_safe() { clang_analyzer_dump(x); // expected-warning-re {{conj_$2{int, {{.+}}}}} return x; // no-warning } + +int *return_symbolic_region_safe(int **ptr) { + return *ptr; // no-warning +} } From d1870073b0c23290807bdbd906f53a2b3f68b909 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Fri, 7 Feb 2025 22:00:00 -0600 Subject: [PATCH 30/40] Add testcase: bind rval ref to temporary --- clang/test/Analysis/stack-addr-ps.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index f2d6e762c150d..3e839478e6e50 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -190,6 +190,11 @@ void test_copy_elision() { C c1 = make1(); } +C&& return_bind_rvalue_reference_to_temporary() { + return C(); // expected-warning{{Address of stack memory associated with temporary object of type 'C' returned to caller}} + // expected-warning@-1{{returning reference to local temporary object}} -Wreturn-stack-address +} + namespace leaking_via_direct_pointer { void* returned_direct_pointer_top() { int local = 42; From abac2ed17731ae9542aed46d2ee3b63f23bcc07e Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Sat, 8 Feb 2025 20:05:21 -0600 Subject: [PATCH 31/40] Add testcases: return argptr, conditional expressions, comparison expressions --- clang/test/Analysis/stack-addr-ps.cpp | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index 3e839478e6e50..ab8c91b217d6d 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -976,4 +976,36 @@ int return_symbol_safe() { int *return_symbolic_region_safe(int **ptr) { return *ptr; // no-warning } + +int *return_arg_ptr(int *arg) { + return arg; // no-warning +} + +// This has a false positive with -Wreturn-stack-address, but CSA should not +// warn on this +int *return_conditional_never_false(int *arg) { + int x = 14; + return true ? arg : &x; // expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +// This has a false positive with -Wreturn-stack-address, but CSA should not +// warn on this +int *return_conditional_never_true(int *arg) { + int x = 14; + return false ? &x : arg; // expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +int *return_conditional_path_split(int *arg, bool b) { + int x = 14; + return b ? arg : &x; // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory associated with local variable 'x' returned}} -Wreturn-stack-address +} + +// compare of two symbolic regions +// maybe in some implementation, make_ptr returns interior pointers that are comparable +int *make_ptr(); +bool return_symbolic_exxpression() { + int *a = make_ptr(); + int *b = make_ptr(); + return a < b; // no-warning +} } From e63d78a812eee50e05120f2d43bf58f35f04af7d Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 10:48:59 -0600 Subject: [PATCH 32/40] Add testscases from SemaCXX/return-stack-addr.cpp --- clang/test/Analysis/stack-addr-ps.cpp | 54 +++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index ab8c91b217d6d..c442fc4b96ae3 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -164,6 +164,18 @@ namespace rdar13296133 { } } // namespace rdar13296133 +void* ret_cpp_static_cast(short x) { + return static_cast(&x); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory}} +} + +int* ret_cpp_reinterpret_cast(double x) { + return reinterpret_cast(&x); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack me}} +} + +int* ret_cpp_const_cast(const int x) { + return const_cast(&x); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory}} +} + void write_stack_address_to(char **q) { char local; *q = &local; @@ -937,6 +949,39 @@ void top_malloc_no_crash_fn() { } } // namespace alloca_region_pointer +// These all warn with -Wreturn-stack-address also +namespace return_address_of_true_positives { +int* ret_local_addrOf() { + int x = 1; + return &*&x; // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +int* ret_local_addrOf_paren() { + int x = 1; + return (&(*(&x))); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +int* ret_local_addrOf_ptr_arith() { + int x = 1; + return &*(&x+1); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +int* ret_local_addrOf_ptr_arith2() { + int x = 1; + return &*(&x+1); // expected-warning {{Address of stack memory associated with local variable 'x' returned to caller}} expected-warning {{address of stack memory associated with local variable 'x' returned}} +} + +int* ret_local_field() { + struct { int x; } a; + return &a.x; // expected-warning {{Address of stack memory associated with local variable 'a' returned to caller}} expected-warning {{address of stack memory associated with local variable 'a' returned}} +} + +int& ret_local_field_ref() { + struct { int x; } a; + return a.x; // expected-warning {{Address of stack memory associated with local variable 'a' returned to caller}} expected-warning {{reference to stack memory associated with local variable 'a' returned}} +} +} //namespace return_address_of_true_positives + namespace true_negatives_return_expressions { struct Container { int *x; }; @@ -1008,4 +1053,13 @@ bool return_symbolic_exxpression() { int *b = make_ptr(); return a < b; // no-warning } + +int *return_static() { + static int x = 0; + return &x; // no-warning +} + +int* return_cpp_reinterpret_cast_no_warning(long x) { + return reinterpret_cast(x); // no-warning +} } From 628fd5252bd6637d413b6995d54e3698ceac8cfe Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 11:07:18 -0600 Subject: [PATCH 33/40] Add lifetimebound attribute test case --- clang/test/Analysis/stackaddrleak.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/clang/test/Analysis/stackaddrleak.c b/clang/test/Analysis/stackaddrleak.c index 5f508275ba9c8..f580955b0b328 100644 --- a/clang/test/Analysis/stackaddrleak.c +++ b/clang/test/Analysis/stackaddrleak.c @@ -56,3 +56,15 @@ void assignAsBool(void) { int x; b = &x; } // no-warning + +int *f(int* p __attribute__((lifetimebound))); +int *g() { + int i; + return f(&i); // expected-warning {{address of stack memory associated with local variable 'i' returned}} +} + +int *f_no_lifetime_bound(int *p); +int *g_no_lifetime_bound() { + int i = 0; + return f_no_lifetime_bound(&i); // no-warning +} \ No newline at end of file From f938dd59edb52603ed936823a59d9eee0618015b Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 11:07:44 -0600 Subject: [PATCH 34/40] Add TODO on testcase for musttail attribute --- clang/test/Analysis/stack-addr-ps.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index c442fc4b96ae3..aa51e5954a2ad 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -1063,3 +1063,22 @@ int* return_cpp_reinterpret_cast_no_warning(long x) { return reinterpret_cast(x); // no-warning } } + +// TODO: The tail call expression tree must not hold a reference to an arg or stack local: +// "The lifetimes of all local variables and function parameters end immediately before the +// [tail] call to the function. This means that it is undefined behaviour to pass a pointer or +// reference to a local variable to the called function, which is not the case without the +// attribute." +// These only warn from -Wreturn-stack-address for now +namespace with_attr_musttail { +void TakesIntAndPtr(int, int *); +void PassAddressOfLocal(int a, int *b) { + int c; + [[clang::musttail]] return TakesIntAndPtr(0, &c); // expected-warning {{address of stack memory associated with local variable 'c' pass\ +ed to musttail function}} +} +void PassAddressOfParam(int a, int *b) { + [[clang::musttail]] return TakesIntAndPtr(0, &a); // expected-warning {{address of stack memory associated with parameter 'a' passed to\ + musttail function}} +} +} // namespace with_attr_musttail \ No newline at end of file From a778a1949829e8815299505d672a9f8b89c720d8 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 11:08:22 -0600 Subject: [PATCH 35/40] Add false negative comments --- clang/test/Analysis/stack-addr-ps.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index aa51e5954a2ad..ddbf678adc1c0 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -1075,10 +1075,10 @@ void TakesIntAndPtr(int, int *); void PassAddressOfLocal(int a, int *b) { int c; [[clang::musttail]] return TakesIntAndPtr(0, &c); // expected-warning {{address of stack memory associated with local variable 'c' pass\ -ed to musttail function}} +ed to musttail function}} False-negative on CSA } void PassAddressOfParam(int a, int *b) { [[clang::musttail]] return TakesIntAndPtr(0, &a); // expected-warning {{address of stack memory associated with parameter 'a' passed to\ - musttail function}} + musttail function}} False-negative on CSA } } // namespace with_attr_musttail \ No newline at end of file From 8bf192dca4d4bd6b159a0dd6f01d8b4a522818af Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 11:20:05 -0600 Subject: [PATCH 36/40] Take references to SmallVectorImpl rather than SmallVector --- .../lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 4f8099e1a6f85..dcd2ad327c8d8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -250,12 +250,12 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, class FindStackRegionsSymbolVisitor final : public SymbolVisitor { CheckerContext &Ctxt; const StackFrameContext *StackFrameContext; - SmallVector &EscapingStackRegions; + SmallVectorImpl &EscapingStackRegions; public: explicit FindStackRegionsSymbolVisitor( CheckerContext &Ctxt, - SmallVector &StorageForStackRegions) + SmallVectorImpl &StorageForStackRegions) : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} @@ -298,7 +298,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { /// 1. If returning a block data region that has stack memory space /// 2. If returning a constructed object that has stack memory space static SmallVector -FilterReturnExpressionLeaks(const SmallVector &MaybeEscaped, +FilterReturnExpressionLeaks(const SmallVectorImpl &MaybeEscaped, CheckerContext &C, const Expr *RetE, SVal &RetVal) { SmallVector WillEscape; From f911a26c0c2096843be67278208c200140899069 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 11:20:37 -0600 Subject: [PATCH 37/40] formatting --- .../lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index dcd2ad327c8d8..826dc42e4f599 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -297,9 +297,9 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { /// likely not true leaks: /// 1. If returning a block data region that has stack memory space /// 2. If returning a constructed object that has stack memory space -static SmallVector -FilterReturnExpressionLeaks(const SmallVectorImpl &MaybeEscaped, - CheckerContext &C, const Expr *RetE, SVal &RetVal) { +static SmallVector FilterReturnExpressionLeaks( + const SmallVectorImpl &MaybeEscaped, CheckerContext &C, + const Expr *RetE, SVal &RetVal) { SmallVector WillEscape; From 4dca6141d6a036a3f67276a8ea48d494b0c6a793 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 12:00:26 -0600 Subject: [PATCH 38/40] Add newline at end of file --- clang/test/Analysis/stack-addr-ps.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp index ddbf678adc1c0..bf988d0a16959 100644 --- a/clang/test/Analysis/stack-addr-ps.cpp +++ b/clang/test/Analysis/stack-addr-ps.cpp @@ -1081,4 +1081,4 @@ void PassAddressOfParam(int a, int *b) { [[clang::musttail]] return TakesIntAndPtr(0, &a); // expected-warning {{address of stack memory associated with parameter 'a' passed to\ musttail function}} False-negative on CSA } -} // namespace with_attr_musttail \ No newline at end of file +} // namespace with_attr_musttail From 9ac7689edb48b317708d92e0588b231bb314b8d8 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 12:01:14 -0600 Subject: [PATCH 39/40] Add newline at end of file --- clang/test/Analysis/stackaddrleak.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/stackaddrleak.c b/clang/test/Analysis/stackaddrleak.c index f580955b0b328..f8101525401b0 100644 --- a/clang/test/Analysis/stackaddrleak.c +++ b/clang/test/Analysis/stackaddrleak.c @@ -67,4 +67,4 @@ int *f_no_lifetime_bound(int *p); int *g_no_lifetime_bound() { int i = 0; return f_no_lifetime_bound(&i); // no-warning -} \ No newline at end of file +} From f73a7f52f93050ab98f9b47cdac1c801fc807781 Mon Sep 17 00:00:00 2001 From: Michael Flanders Date: Mon, 10 Feb 2025 16:37:03 -0600 Subject: [PATCH 40/40] [analyzer] Fix StackFrameContext define in stack addr escape checker --- .../lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 826dc42e4f599..c9df15ceb3b40 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -249,14 +249,14 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, /// that would leak. class FindStackRegionsSymbolVisitor final : public SymbolVisitor { CheckerContext &Ctxt; - const StackFrameContext *StackFrameContext; + const StackFrameContext *PoppedStackFrame; SmallVectorImpl &EscapingStackRegions; public: explicit FindStackRegionsSymbolVisitor( CheckerContext &Ctxt, SmallVectorImpl &StorageForStackRegions) - : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()), + : Ctxt(Ctxt), PoppedStackFrame(Ctxt.getStackFrame()), EscapingStackRegions(StorageForStackRegions) {} bool VisitSymbol(SymbolRef sym) override { return true; } @@ -274,7 +274,7 @@ class FindStackRegionsSymbolVisitor final : public SymbolVisitor { void SaveIfEscapes(const MemRegion *MR) { const StackSpaceRegion *SSR = MR->getMemorySpace()->getAs(); - if (SSR && SSR->getStackFrame() == StackFrameContext) + if (SSR && SSR->getStackFrame() == PoppedStackFrame) EscapingStackRegions.push_back(MR); }