[analyzer] Detect leaks on top-level via output params, indirect globals

Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
  ```
  void top(int **out) {
    int local = 42;
    *out = &local; // Noncompliant
  }
  ```
- Indirect global pointers
  ```
  int **global;

  void top() {
    int local = 42;
    *global = &local; // Noncompliant
  }
  ```

Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp

To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.

The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.

CPP-4734

Author: necto

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

@@ -305,6 +305,14 @@ static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
   return nullptr;
 }
 
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+  Referrer = Referrer->getBaseRegion();
+  while (const auto *SymReg = dyn_cast<SymbolicRegion>(Referrer)) {
+    Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+  }
+  return Referrer;
+}
+
 std::optional<std::string> printReferrer(const MemRegion *Referrer) {
   assert(Referrer);
   const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional<std::string> printReferrer(const MemRegion *Referrer) {
     if (isa<GlobalsSpaceRegion>(Space))
       return "global";
     assert(isa<StackSpaceRegion>(Space));
-    return "stack";
+    // This case covers top-level and inlined analyses.
+    return "caller";
   }(getStackOrGlobalSpaceRegion(Referrer));
 
   while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
 
   ExplodedNode *Node = Ctx.getPredecessor();
 
+  bool ExitingTopFrame =
+      Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+  if (ExitingTopFrame && Node->getLocation().getTag() &&
+      Node->getLocation().getTag()->getTagDescription() ==
+          "ExprEngine : Clean Node" &&
+      Node->getFirstPred()) {
+    // When finishing analysis of a top-level function, engine proactively
+    // removes dead symbols thus preventing this checker from looking through
+    // the output parameters. Take 1 step back, to the node where these symbols
+    // and their bindings are still present
+    Node = Node->getFirstPred();
+  }
+
   // Iterate over all bindings to global variables and see if it contains
   // a memory region in the stack space.
   class CallBack : public StoreManager::BindingsHandler {
   private:
     CheckerContext &Ctx;
     const StackFrameContext *PoppedFrame;
+    const bool TopFrame;
 
     /// Look for stack variables referring to popped stack variables.
     /// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
 
       const auto *ReferrerStackSpace =
           ReferrerMemSpace->getAs<StackSpaceRegion>();
+
       if (!ReferrerStackSpace)
         return false;
 
-      if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
-          ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+      if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+          ReferredFrame != PoppedFrame) {
+        return false;
+      }
+
+      if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+        V.emplace_back(Referrer, Referred);
+        return true;
+      }
+      if (isa<StackArgumentsSpaceRegion>(ReferrerMemSpace) &&
+          ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
+        // Output parameter of a top-level function
         V.emplace_back(Referrer, Referred);
         return true;
       }
       return false;
     }
 
+    void updateInvalidatedRegions(const MemRegion *Region) {
+      if (const auto *SymReg = Region->getAs<SymbolicRegion>()) {
+        SymbolRef Symbol = SymReg->getSymbol();
+        if (const auto *DerS = dyn_cast<SymbolDerived>(Symbol);
+            DerS && isa_and_nonnull<SymbolConjured>(DerS->getParentSymbol())) {
+          InvalidatedRegions.insert(Symbol->getOriginRegion()->getBaseRegion());
+        }
+      }
+    }
+
   public:
     SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V;
+    llvm::SmallPtrSet<const MemRegion *, 4> InvalidatedRegions;
 
-    CallBack(CheckerContext &CC) : Ctx(CC), PoppedFrame(CC.getStackFrame()) {}
+    CallBack(CheckerContext &CC, bool TopFrame)
+        : Ctx(CC), PoppedFrame(CC.getStackFrame()), TopFrame(TopFrame) {}
 
     bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region,
                        SVal Val) override {
+      updateInvalidatedRegions(Region);
       const MemRegion *VR = Val.getAsRegion();
       if (!VR)
         return true;
@@ -395,15 +443,16 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
         return true;
 
       // Check the globals for the same.
-      if (!isa<GlobalsSpaceRegion>(Region->getMemorySpace()))
+      if (!isa_and_nonnull<GlobalsSpaceRegion>(
+              getStackOrGlobalSpaceRegion(Region)))
         return true;
       if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx))
         V.emplace_back(Region, VR);
       return true;
     }
   };
 
-  CallBack Cb(Ctx);
+  CallBack Cb(Ctx, ExitingTopFrame);
   ProgramStateRef State = Node->getState();
   State->getStateManager().getStoreManager().iterBindings(State->getStore(),
                                                           Cb);
@@ -424,6 +473,9 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
   for (const auto &P : Cb.V) {
     const MemRegion *Referrer = P.first->getBaseRegion();
     const MemRegion *Referred = P.second;
+    if (Cb.InvalidatedRegions.contains(getOriginBaseRegion(Referrer))) {
+      continue;
+    }
 
     // Generate a report for this bug.
     const StringRef CommonSuffix =

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

@@ -2436,8 +2436,19 @@ PathSensitiveBugReport::getLocation() const {
     if (auto FE = P.getAs<FunctionExitPoint>()) {
       if (const ReturnStmt *RS = FE->getStmt())
         return PathDiagnosticLocation::createBegin(RS, SM, LC);
+
+      // If we are exiting a destructor call, it is more useful to point to the
+      // next stmt which is usually the temporary declaration.
+      // For non-destructor and non-top-level calls, the next stmt will still
+      // refer to the last executed stmt of the body.
+      S = ErrorNode->getNextStmtForDiagnostics();
+      // If next stmt is not found, it is likely the end of a top-level function
+      // analysis. find the last execution statement then.
+      if (!S)
+        S = ErrorNode->getPreviousStmtForDiagnostics();
     }
-    S = ErrorNode->getNextStmtForDiagnostics();
+    if (!S)
+      S = ErrorNode->getNextStmtForDiagnostics();
   }
 
   if (S) {

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp

@@ -376,7 +376,7 @@ const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
 
 const Stmt *ExplodedNode::getPreviousStmtForDiagnostics() const {
   for (const ExplodedNode *N = getFirstPred(); N; N = N->getFirstPred())
-    if (const Stmt *S = N->getStmtForDiagnostics())
+    if (const Stmt *S = N->getStmtForDiagnostics(); S && !isa<CompoundStmt>(S))
       return S;
 
   return nullptr;

clang/test/Analysis/copy-elision.cpp

@@ -158,19 +158,19 @@ ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
   return ClassWithoutDestructor(v);
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithoutDestructor' is still \
-referred to by the stack variable 'v' upon returning to the caller}}
+referred to by the caller 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 'ClassWithoutDestructor' is still \
-referred to by the stack variable 'v' upon returning to the caller}}
+referred to by the caller 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 'ClassWithoutDestructor' is still \
-referred to by the stack variable 'v' upon returning to the caller}}
+referred to by the caller variable 'v' upon returning to the caller}}
 }
 
 void testMultipleReturns() {
@@ -193,7 +193,7 @@ 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 \
+variable 'c' is still referred to by the caller variable 'v' upon returning \
 to the caller}}
 }
 
@@ -267,7 +267,7 @@ struct TestCtorInitializer {
     : c(ClassWithDestructor(v)) {}
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithDestructor' is still referred \
-to by the stack variable 'v' upon returning to the caller}}
+to by the caller variable 'v' upon returning to the caller}}
 };
 
 void testCtorInitializer() {
@@ -303,19 +303,19 @@ ClassWithDestructor make1(AddressVector<ClassWithDestructor> &v) {
   return ClassWithDestructor(v);
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithDestructor' is still referred \
-to by the stack variable 'v' upon returning to the caller}}
+to by the caller 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 'ClassWithDestructor' is still referred \
-to by the stack variable 'v' upon returning to the caller}}
+to by the caller 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 'ClassWithDestructor' is still referred \
-to by the stack variable 'v' upon returning to the caller}}
+to by the caller variable 'v' upon returning to the caller}}
 }
 
 void testMultipleReturnsWithDestructors() {
@@ -360,7 +360,7 @@ 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 \
+variable 'c' is still referred to by the caller variable 'v' upon returning \
 to the caller}}
 }
 
@@ -407,7 +407,7 @@ struct Foo {
 Foo make1(Foo **r) {
   return Foo(r);
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
-object of type 'Foo' is still referred to by the stack \
+object of type 'Foo' is still referred to by the caller \
 variable 'z' upon returning to the caller}}
 }
 

clang/test/Analysis/incorrect-checker-names.cpp

@@ -16,5 +16,5 @@ char const *p;
 void f0() {
   char const str[] = "This will change";
   p = str;
-} // expected-warning{{Address of stack memory associated with local variable 'str' is still referred to by the global variable 'p' upon returning to the caller.  This will be a dangling reference [core.StackAddressEscape]}}
-// expected-note@-1{{Address of stack memory associated with local variable 'str' is still referred to by the global variable 'p' upon returning to the caller.  This will be a dangling reference}}
+} // expected-warning@-1{{Address of stack memory associated with local variable 'str' is still referred to by the global variable 'p' upon returning to the caller.  This will be a dangling reference [core.StackAddressEscape]}}
+// expected-note@-2{{Address of stack memory associated with local variable 'str' is still referred to by the global variable 'p' upon returning to the caller.  This will be a dangling reference}}

clang/test/Analysis/loop-block-counts.c

@@ -6,7 +6,7 @@ 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 \
+variable 'x' is still referred to by the caller variable 'arr' upon \
 returning to the caller}}
 }
 

clang/test/Analysis/stack-addr-ps.c

@@ -98,13 +98,13 @@ void callTestRegister(void) {
 
 void top_level_leaking(int **out) {
   int local = 42;
-  *out = &local; // no-warning FIXME
+  *out = &local; // expected-warning{{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 'out'}}
 }
 
 void callee_leaking_via_param(int **out) {
   int local = 1;
   *out = &local;
-  // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
+  // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 'ptr'}}
 }
 
 void caller_for_leaking_callee() {
@@ -115,7 +115,7 @@ void caller_for_leaking_callee() {
 void callee_nested_leaking(int **out) {
   int local = 1;
   *out = &local;
-  // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
+  // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 'ptr'}}
 }
 
 void caller_mid_for_nested_leaking(int **mid) {