[clang] 6ad47e1 - [analyzer] Catch leaking stack addresses via stack variables
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 27 02:31:37 PDT 2021
Author: Balazs Benics
Date: 2021-08-27T11:31:16+02:00
New Revision: 6ad47e1c4fbfa8a752cb711acdf242fed3f16a68
URL: https://github.com/llvm/llvm-project/commit/6ad47e1c4fbfa8a752cb711acdf242fed3f16a68
DIFF: https://github.com/llvm/llvm-project/commit/6ad47e1c4fbfa8a752cb711acdf242fed3f16a68.diff
LOG: [analyzer] Catch leaking stack addresses via stack variables
Not only global variables can hold references to dead stack variables.
Consider this example:
void write_stack_address_to(char **q) {
char local;
*q = &local;
}
void test_stack() {
char *p;
write_stack_address_to(&p);
}
The address of 'local' is assigned to 'p', which becomes a dangling
pointer after 'write_stack_address_to()' returns.
The StackAddrEscapeChecker was looking for bindings in the store which
referred to variables of the popped stack frame, but it only considered
global variables in this regard. This patch relaxes this, catching
stack variable bindings as well.
---
This patch also works for temporary objects like:
struct Bar {
const int &ref;
explicit Bar(int y) : ref(y) {
// Okay.
} // End of the constructor call, `ref` is dangling now. Warning!
};
void test() {
Bar{33}; // Temporary object, so the corresponding memregion is
// *not* a VarRegion.
}
---
The return value optimization aka. copy-elision might kick in but that
is modeled by passing an imaginary CXXThisRegion which refers to the
parent stack frame which is supposed to be the 'return slot'.
Objects residing in the 'return slot' outlive the scope of the inner
call, thus we should expect no warning about them - except if we
explicitly disable copy-elision.
Reviewed By: NoQ, martong
Differential Revision: https://reviews.llvm.org/D107078
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
clang/test/Analysis/copy-elision.cpp
clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
clang/test/Analysis/loop-block-counts.c
clang/test/Analysis/stack-addr-ps.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index b5c9356322fcc..d5e86e86424d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -11,9 +11,9 @@
//
//===----------------------------------------------------------------------===//
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/AST/ExprCXX.h"
#include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
@@ -303,21 +303,53 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
- const StackFrameContext *CurSFC;
+ const StackFrameContext *PoppedFrame;
+
+ /// 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
diff erent stack frame.
+ bool checkForDanglingStackVariable(const MemRegion *Referrer,
+ const MemRegion *Referred) {
+ const auto *ReferrerMemSpace =
+ Referrer->getMemorySpace()->getAs<StackSpaceRegion>();
+ const auto *ReferredMemSpace =
+ Referred->getMemorySpace()->getAs<StackSpaceRegion>();
+
+ if (!ReferrerMemSpace || !ReferredMemSpace)
+ return false;
+
+ const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
+ const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+
+ if (ReferrerMemSpace && ReferredMemSpace) {
+ if (ReferredFrame == PoppedFrame &&
+ ReferrerFrame->isParentOf(PoppedFrame)) {
+ V.emplace_back(Referrer, Referred);
+ return true;
+ }
+ }
+ return false;
+ }
public:
SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V;
- CallBack(CheckerContext &CC) : Ctx(CC), CurSFC(CC.getStackFrame()) {}
+ CallBack(CheckerContext &CC) : Ctx(CC), PoppedFrame(CC.getStackFrame()) {}
bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region,
SVal Val) override {
+ const MemRegion *VR = Val.getAsRegion();
+ if (!VR)
+ return true;
+
+ if (checkForDanglingStackVariable(Region, VR))
+ return true;
+ // Check the globals for the same.
if (!isa<GlobalsSpaceRegion>(Region->getMemorySpace()))
return true;
- const MemRegion *VR = Val.getAsRegion();
- if (VR && isa<StackSpaceRegion>(VR->getMemorySpace()) &&
- !isArcManagedBlock(VR, Ctx) && !isNotInCurrentFrame(VR, Ctx))
+ if (VR && VR->hasStackStorage() && !isArcManagedBlock(VR, Ctx) &&
+ !isNotInCurrentFrame(VR, Ctx))
V.emplace_back(Region, VR);
return true;
}
@@ -344,19 +376,41 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
"invalid after returning from the function");
for (const auto &P : Cb.V) {
+ const MemRegion *Referrer = P.first;
+ const MemRegion *Referred = P.second;
+
// Generate a report for this bug.
+ const StringRef CommonSuffix =
+ "upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
- SourceRange Range = genName(Out, P.second, Ctx.getASTContext());
- Out << " is still referred to by the ";
- if (isa<StaticGlobalSpaceRegion>(P.first->getMemorySpace()))
- Out << "static";
- else
- Out << "global";
- Out << " variable '";
- const VarRegion *VR = cast<VarRegion>(P.first->getBaseRegion());
- Out << *VR->getDecl()
- << "' upon returning to the caller. This will be a dangling reference";
+ const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
+
+ if (isa<CXXTempObjectRegion>(Referrer)) {
+ Out << " is still referred to by a temporary object on the stack "
+ << CommonSuffix;
+ auto Report =
+ std::make_unique<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
+ Ctx.emitReport(std::move(Report));
+ return;
+ }
+
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+ if (isa<StaticGlobalSpaceRegion>(Space))
+ return "static";
+ if (isa<GlobalsSpaceRegion>(Space))
+ return "global";
+ assert(isa<StackSpaceRegion>(Space));
+ return "stack";
+ }(Referrer->getMemorySpace());
+
+ // This cast supposed to succeed.
+ const VarRegion *ReferrerVar = cast<VarRegion>(Referrer->getBaseRegion());
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ Out << " is still referred to by the " << ReferrerMemorySpace
+ << " variable '" << ReferrerVarName << "' " << CommonSuffix;
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
if (Range.isValid())
diff --git a/clang/test/Analysis/copy-elision.cpp b/clang/test/Analysis/copy-elision.cpp
index 3d2055f6392c9..9da896fc2598d 100644
--- a/clang/test/Analysis/copy-elision.cpp
+++ b/clang/test/Analysis/copy-elision.cpp
@@ -1,7 +1,13 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 \
+// RUN: -analyzer-config eagerly-assume=false -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 \
+// RUN: -analyzer-config eagerly-assume=false -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 \
+// RUN: -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG \
+// RUN: -analyzer-config eagerly-assume=false -verify=expected,no-elide %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 \
+// RUN: -analyzer-config elide-constructors=false \
+// RUN: -analyzer-config eagerly-assume=false -verify %s
// Copy elision always occurs in C++17, otherwise it's under
// an on-by-default flag.
@@ -149,12 +155,21 @@ class ClassWithoutDestructor {
ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
return ClassWithoutDestructor(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithoutDestructor' is still \
+referred to by the stack variable 'v' upon returning to the caller}}
}
ClassWithoutDestructor make2(AddressVector<ClassWithoutDestructor> &v) {
return make1(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithoutDestructor' is still \
+referred to by the stack variable 'v' upon returning to the caller}}
}
ClassWithoutDestructor make3(AddressVector<ClassWithoutDestructor> &v) {
return make2(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithoutDestructor' is still \
+referred to by the stack variable 'v' upon returning to the caller}}
}
void testMultipleReturns() {
@@ -176,6 +191,9 @@ void testMultipleReturns() {
void consume(ClassWithoutDestructor c) {
c.push();
+ // expected-warning at -1 {{Address of stack memory associated with local \
+variable 'c' is still referred to by the stack variable 'v' upon returning \
+to the caller}}
}
void testArgumentConstructorWithoutDestructor() {
@@ -246,6 +264,9 @@ struct TestCtorInitializer {
ClassWithDestructor c;
TestCtorInitializer(AddressVector<ClassWithDestructor> &v)
: c(ClassWithDestructor(v)) {}
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithDestructor' is still referred \
+to by the stack variable 'v' upon returning to the caller}}
};
void testCtorInitializer() {
@@ -279,12 +300,21 @@ void testCtorInitializer() {
ClassWithDestructor make1(AddressVector<ClassWithDestructor> &v) {
return ClassWithDestructor(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithDestructor' is still referred \
+to by the stack variable 'v' upon returning to the caller}}
}
ClassWithDestructor make2(AddressVector<ClassWithDestructor> &v) {
return make1(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithDestructor' is still referred \
+to by the stack variable 'v' upon returning to the caller}}
}
ClassWithDestructor make3(AddressVector<ClassWithDestructor> &v) {
return make2(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::ClassWithDestructor' is still referred \
+to by the stack variable 'v' upon returning to the caller}}
}
void testMultipleReturnsWithDestructors() {
@@ -328,6 +358,9 @@ void testMultipleReturnsWithDestructors() {
void consume(ClassWithDestructor c) {
c.push();
+ // expected-warning at -1 {{Address of stack memory associated with local \
+variable 'c' is still referred to by the stack variable 'v' upon returning \
+to the caller}}
}
void testArgumentConstructorWithDestructor() {
@@ -364,4 +397,24 @@ void testArgumentConstructorWithDestructor() {
#endif
}
+struct Foo {
+ Foo(Foo **q) {
+ *q = this;
+ }
+};
+
+Foo make1(Foo **r) {
+ return Foo(r);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'address_vector_tests::Foo' is still referred to by the stack \
+variable 'z' upon returning to the caller}}
+}
+
+void test_copy_elision() {
+ Foo *z;
+ // If the copy elided, 'z' points to 'tmp', otherwise it's a dangling pointer.
+ Foo tmp = make1(&z);
+ (void)tmp;
+}
+
} // namespace address_vector_tests
diff --git a/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp b/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
index 5363831342fb1..fc067dd04428a 100644
--- a/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -71,6 +71,9 @@ struct UntypedAllocaTest {
void *allocaPtr;
int dontGetFilteredByNonPedanticMode = 0;
+ // expected-warning-re at +3 {{Address of stack memory allocated by call to \
+alloca() on line {{[0-9]+}} is still referred to by a temporary object on the \
+stack upon returning to the caller. This will be a dangling reference}}
UntypedAllocaTest() : allocaPtr(__builtin_alloca(sizeof(int))) {
// All good!
}
@@ -86,6 +89,9 @@ struct TypedAllocaTest1 {
TypedAllocaTest1() // expected-warning{{1 uninitialized field}}
: allocaPtr(static_cast<int *>(__builtin_alloca(sizeof(int)))) {}
+ // expected-warning-re at -2 {{Address of stack memory allocated by call to \
+alloca() on line {{[0-9]+}} is still referred to by a temporary object on the \
+stack upon returning to the caller. This will be a dangling reference}}
};
void fTypedAllocaTest1() {
@@ -96,6 +102,9 @@ struct TypedAllocaTest2 {
int *allocaPtr;
int dontGetFilteredByNonPedanticMode = 0;
+ // expected-warning-re at +5 {{Address of stack memory allocated by call to \
+alloca() on line {{[0-9]+}} is still referred to by a temporary object on the \
+stack upon returning to the caller. This will be a dangling reference}}
TypedAllocaTest2()
: allocaPtr(static_cast<int *>(__builtin_alloca(sizeof(int)))) {
*allocaPtr = 55555;
@@ -341,6 +350,9 @@ class VoidPointerRRefTest1 {
void *&&vptrrref; // expected-note {{here}}
public:
+ // expected-warning at +3 {{Address of stack memory associated with local \
+variable 'vptr' is still referred to by a temporary object on the stack \
+upon returning to the caller. This will be a dangling reference}}
VoidPointerRRefTest1(void *vptr, char) : vptrrref(static_cast<void *&&>(vptr)) { // expected-warning {{binding reference member 'vptrrref' to stack allocated parameter 'vptr'}}
// All good!
}
@@ -355,6 +367,9 @@ class VoidPointerRRefTest2 {
void **&&vpptrrref; // expected-note {{here}}
public:
+ // expected-warning at +3 {{Address of stack memory associated with local \
+variable 'vptr' is still referred to by a temporary object on the stack \
+upon returning to the caller. This will be a dangling reference}}
VoidPointerRRefTest2(void **vptr, char) : vpptrrref(static_cast<void **&&>(vptr)) { // expected-warning {{binding reference member 'vpptrrref' to stack allocated parameter 'vptr'}}
// All good!
}
@@ -369,6 +384,9 @@ class VoidPointerLRefTest {
void *&vptrrref; // expected-note {{here}}
public:
+ // expected-warning at +3 {{Address of stack memory associated with local \
+variable 'vptr' is still referred to by a temporary object on the stack \
+upon returning to the caller. This will be a dangling reference}}
VoidPointerLRefTest(void *vptr, char) : vptrrref(static_cast<void *&>(vptr)) { // expected-warning {{binding reference member 'vptrrref' to stack allocated parameter 'vptr'}}
// All good!
}
diff --git a/clang/test/Analysis/loop-block-counts.c b/clang/test/Analysis/loop-block-counts.c
index 66bb850780cb1..ab26864d2d959 100644
--- a/clang/test/Analysis/loop-block-counts.c
+++ b/clang/test/Analysis/loop-block-counts.c
@@ -5,6 +5,9 @@ void clang_analyzer_eval(int);
void callee(void **p) {
int x;
*p = &x;
+ // expected-warning at -1 {{Address of stack memory associated with local \
+variable 'x' is still referred to by the stack variable 'arr' upon \
+returning to the caller}}
}
void loop() {
diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp
index e1f06835c784f..956dcb0428e9b 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -137,3 +137,28 @@ namespace rdar13296133 {
}
}
+void write_stack_address_to(char **q) {
+ char local;
+ *q = &local;
+ // expected-warning at -1 {{Address of stack memory associated with local \
+variable 'local' is still referred to by the stack variable 'p' upon \
+returning to the caller}}
+}
+
+void test_stack() {
+ char *p;
+ write_stack_address_to(&p);
+}
+
+struct C {
+ ~C() {} // non-trivial class
+};
+
+C make1() {
+ C c;
+ return c; // no-warning
+}
+
+void test_copy_elision() {
+ C c1 = make1();
+}
More information about the cfe-commits
mailing list