Revert "[analyzer] Remove some false negatives in StackAddrEscapeChec… #126614
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ker (llvm#125638)" This reverts commit 7ba3c55.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Gábor Horváth (Xazax-hun) Changes…ker (#125638)" This reverts commit 7ba3c55. Patch is 27.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126614.diff 5 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 826dc42e4f59948..f4de3b500499c48 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 EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion,
- const Expr *RetE) const;
+ void EmitStackError(CheckerContext &C, const MemRegion *R,
+ const Expr *RetE) const;
bool isSemaphoreCaptured(const BlockDecl &B) const;
static SourceRange genName(raw_ostream &os, const MemRegion *R,
ASTContext &Ctx);
@@ -147,22 +147,9 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
return Regions;
}
-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 {
+void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
+ const MemRegion *R,
+ const Expr *RetE) const {
ExplodedNode *N = C.generateNonFatalErrorNode();
if (!N)
return;
@@ -170,15 +157,11 @@ void StackAddrEscapeChecker::EmitReturnLeakError(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());
- EmitReturnedAsPartOfError(os, C.getSVal(RetE), R);
-
+ os << " returned to caller";
auto report =
std::make_unique<PathSensitiveBugReport>(*BT_returnstack, os.str(), N);
report->addRange(RetE->getSourceRange());
@@ -226,6 +209,30 @@ 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])
@@ -240,128 +247,45 @@ 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;
- SmallVectorImpl<const MemRegion *> &EscapingStackRegions;
-
-public:
- explicit FindStackRegionsSymbolVisitor(
- CheckerContext &Ctxt,
- SmallVectorImpl<const MemRegion *> &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<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);
- }
- }
+void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
+ CheckerContext &C) const {
+ if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+ return;
- return false;
- }
-};
+ const Expr *RetE = RS->getRetValue();
+ if (!RetE)
+ return;
+ RetE = RetE->IgnoreParens();
-/// 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 SmallVectorImpl<const MemRegion *> &MaybeEscaped, CheckerContext &C,
- const Expr *RetE, SVal &RetVal) {
+ SVal V = C.getSVal(RetE);
+ const MemRegion *R = V.getAsRegion();
+ if (!R)
+ return;
- SmallVector<const MemRegion *> WillEscape;
+ if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
+ checkReturnedBlockCaptures(*B, C);
- const MemRegion *RetRegion = RetVal.getAsRegion();
+ if (!isa<StackSpaceRegion>(R->getMemorySpace()) || isNotInCurrentFrame(R, C))
+ return;
// 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();
- bool IsConstructExpr =
- isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType();
+ if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
+ return;
// 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)) {
- IsCopyAndAutoreleaseBlockObj =
- isa_and_nonnull<BlockDataRegion>(RetRegion) &&
- ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject;
- }
-
- for (const MemRegion *MR : MaybeEscaped) {
- if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr))
- continue;
-
- WillEscape.push_back(MR);
+ if (isa<BlockDataRegion>(R) &&
+ ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
+ return;
+ }
}
- 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);
+ EmitStackError(C, R, RetE);
}
static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
diff --git a/clang/test/Analysis/copy-elision.cpp b/clang/test/Analysis/copy-elision.cpp
index e69f52f351f1bc0..cd941854fc7f196 100644
--- a/clang/test/Analysis/copy-elision.cpp
+++ b/clang/test/Analysis/copy-elision.cpp
@@ -154,26 +154,20 @@ 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<ClassWithoutDestructor> &v) {
- return ClassWithoutDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+ return ClassWithoutDestructor(v);
// 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<ClassWithoutDestructor> &v) {
- return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+ return make1(v);
// 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<ClassWithoutDestructor> &v) {
- return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+ return make2(v);
// 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}}
@@ -304,26 +298,21 @@ 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<ClassWithDestructor> &v) {
- return ClassWithDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+ return ClassWithDestructor(v);
// 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<ClassWithDestructor> &v) {
- return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+ return make1(v);
// 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<ClassWithDestructor> &v) {
- return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+ return make2(v);
// 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}}
diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp
index bf988d0a1695947..73e9dbeca460f60 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -90,18 +90,6 @@ 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}}
-}
-
-// 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}}
@@ -164,18 +152,6 @@ namespace rdar13296133 {
}
} // namespace rdar13296133
-void* ret_cpp_static_cast(short x) {
- return static_cast<void*>(&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<int*>(&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<int*>(&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;
@@ -202,11 +178,6 @@ 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;
@@ -280,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); // expected-warning{{Address of stack memory associated with local variable 'lambda' returned to caller}}
+ return new MyFunction(&lambda);
}
void lambda_to_context_direct_pointer_lifetime_extended() {
@@ -369,13 +340,6 @@ 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);
}
@@ -446,16 +410,16 @@ void** returned_arr_of_ptr_top() {
int* p = &local;
void** arr = new void*[2];
arr[1] = p;
- return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
-}
+ return arr;
+} // no-warning False Negative
void** returned_arr_of_ptr_callee() {
int local = 42;
int* p = &local;
void** arr = new void*[2];
arr[1] = p;
- return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
-}
+ return arr;
+} // no-warning False Negative
void returned_arr_of_ptr_caller() {
void** arr = returned_arr_of_ptr_callee();
@@ -502,16 +466,16 @@ void** returned_arr_of_ptr_top(int idx) {
int* p = &local;
void** arr = new void*[2];
arr[idx] = p;
- return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
-}
+ return arr;
+} // no-warning False Negative
void** returned_arr_of_ptr_callee(int idx) {
int local = 42;
int* p = &local;
void** arr = new void*[2];
arr[idx] = p;
- return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
-}
+ return arr;
+} // no-warning False Negative
void returned_arr_of_ptr_caller(int idx) {
void** arr = returned_arr_of_ptr_callee(idx);
@@ -561,25 +525,14 @@ S returned_struct_with_ptr_top() {
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;
+} // no-warning False Negative, requires traversing returned LazyCompoundVals
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}} 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}}
+ return s; // expected-warning{{'local' is still referred to by the caller variable 's'}}
}
void returned_struct_with_ptr_caller() {
@@ -602,30 +555,6 @@ 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;
@@ -684,15 +613,15 @@ S* returned_ptr_to_struct_with_ptr_top() {
int local = 42;
S* s = new S;
s->p = &local;
- r...
[truncated]
|
Flandini
added a commit
to Flandini/llvm-project
that referenced
this pull request
Feb 10, 2025
…c… (llvm#126614) This reverts commit 3c6d1dd. Reapply recent stack addr escape checker changes after fixing buildbot errors.
|
Usually reverts should have a link to the logs of an example build bot failure. |
|
Ah, sorry! Will include the log next time. |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ker (#125638)"
This reverts commit 7ba3c55.