[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

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 different 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())

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

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!
   }

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() {

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();
+}