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@-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@-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@-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@-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@-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@-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@-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@-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@-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@-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@+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@-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@+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@+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@+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@+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@-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@-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(); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
