[clang] [analyzer] Remove some false negatives in StackAddrEscapeChecker (PR #125638)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 4 02:24:33 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Michael Flanders (Flandini)
<details>
<summary>Changes</summary>
Fixes https://github.com/llvm/llvm-project/issues/123459.
Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in https://github.com/llvm/llvm-project/issues/123459.
The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also.
Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
---
Full diff: https://github.com/llvm/llvm-project/pull/125638.diff
3 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp (+135-53)
- (modified) clang/test/Analysis/stack-addr-ps.cpp (+55-20)
- (modified) clang/test/Analysis/stackaddrleak.cpp (+2-2)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index f4de3b500499c48..86f0949994cf6b9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -54,8 +54,8 @@ 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);
@@ -147,9 +147,22 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
return Regions;
}
-void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
- const MemRegion *R,
- const Expr *RetE) const {
+static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, SVal ReturnedVal,
+ const MemRegion *LeakedRegion) {
+ if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) {
+ if (isa<BlockDataRegion>(ReturnedRegion)) {
+ OS << " is captured by a returned block";
+ return;
+ }
+ }
+
+ // Generic message
+ OS << " returned to caller";
+}
+
+void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C,
+ const MemRegion *R,
+ const Expr *RetE) const {
ExplodedNode *N = C.generateNonFatalErrorNode();
if (!N)
return;
@@ -157,11 +170,15 @@ void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
BT_returnstack = std::make_unique<BugType>(
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<PathSensitiveBugReport>(*BT_returnstack, os.str(), N);
report->addRange(RetE->getSourceRange());
@@ -209,30 +226,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<BugType>(
- 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<PathSensitiveBugReport>(*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])
@@ -247,45 +240,134 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
}
}
-void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
- CheckerContext &C) const {
- if (!ChecksEnabled[CK_StackAddrEscapeChecker])
- return;
+/// 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;
+ SmallVector<const MemRegion *> &EscapingStackRegions;
- const Expr *RetE = RS->getRetValue();
- if (!RetE)
- return;
- RetE = RetE->IgnoreParens();
+public:
+ explicit FindStackRegionsSymbolVisitor(
+ CheckerContext &Ctxt,
+ SmallVector<const MemRegion *> &StorageForStackRegions)
+ : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()),
+ EscapingStackRegions(StorageForStackRegions) {}
- SVal V = C.getSVal(RetE);
- const MemRegion *R = V.getAsRegion();
- if (!R)
- return;
+ bool VisitSymbol(SymbolRef sym) override { return true; }
- if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
- checkReturnedBlockCaptures(*B, C);
+ bool VisitMemRegion(const MemRegion *MR) override {
+ SaveIfEscapes(MR);
- if (!isa<StackSpaceRegion>(R->getMemorySpace()) || isNotInCurrentFrame(R, C))
- return;
+ if (const BlockDataRegion *BDR = MR->getAs<BlockDataRegion>())
+ return VisitBlockDataRegionCaptures(BDR);
+
+ return true;
+ }
+
+private:
+ void SaveIfEscapes(const MemRegion *MR) {
+ const StackSpaceRegion *SSR =
+ MR->getMemorySpace()->getAs<StackSpaceRegion>();
+ 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) {
+ SaveIfEscapes(Region);
+ VisitMemRegion(Region);
+ }
+ }
+
+ return false;
+ }
+};
+
+/// 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<const MemRegion *>
+FilterReturnExpressionLeaks(const SmallVector<const MemRegion *> &MaybeEscaped,
+ CheckerContext &C, const Expr *RetE, SVal &RetVal) {
+
+ SmallVector<const MemRegion *> 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.)
if (const ExprWithCleanups *Cleanup = dyn_cast<ExprWithCleanups>(RetE))
RetE = Cleanup->getSubExpr();
- if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
- return;
+ bool IsConstructExpr =
+ isa<CXXConstructExpr>(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<ImplicitCastExpr>(RetE)) {
- if (isa<BlockDataRegion>(R) &&
- ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
- return;
- }
+ IsCopyAndAutoreleaseBlockObj =
+ isa_and_nonnull<BlockDataRegion>(RetRegion) &&
+ ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject;
+ }
+
+ for (const MemRegion *MR : 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<nonloc::LazyCompoundVal>() &&
+ isa<CXXTempObjectRegion>(MR))
+ continue;
+
+ WillEscape.push_back(MR);
}
- EmitStackError(C, R, RetE);
+ 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<const MemRegion *>
+FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) {
+ SmallVector<const MemRegion *> 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;
+
+ const Expr *RetE = RS->getRetValue();
+ if (!RetE)
+ return;
+ RetE = RetE->IgnoreParens();
+
+ SVal V = C.getSVal(RetE);
+
+ SmallVector<const MemRegion *> EscapedStackRegions =
+ FindEscapingStackRegions(C, RetE, V);
+
+ for (const MemRegion *ER : EscapedStackRegions)
+ EmitReturnLeakError(C, ER, 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 73e9dbeca460f60..392982d92a3f14c 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() {
@@ -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}} 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() {
+ 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 3daffb35a6cd9a6..a44fb1f7d2dd121 100644
--- a/clang/test/Analysis/stackaddrleak.cpp
+++ b/clang/test/Analysis/stackaddrleak.cpp
@@ -18,8 +18,8 @@ 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}} 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()();
-}
+}
\ No newline at end of file
``````````
</details>
https://github.com/llvm/llvm-project/pull/125638
More information about the cfe-commits
mailing list