[analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (#105648)

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

---------

This is the last of the 3 stacked PRs, it must not be merged before
#105652 and
#105653

Author: necto

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

@@ -286,6 +286,10 @@ class ExprEngine {
             const Stmt *DiagnosticStmt = nullptr,
             ProgramPoint::Kind K = ProgramPoint::PreStmtPurgeDeadSymbolsKind);
 
+  /// A tag to track convenience transitions, which can be removed at cleanup.
+  /// This tag applies to a node created after removeDead.
+  static const ProgramPointTag *cleanupNodeTag();
+
   /// processCFGElement - Called by CoreEngine. Used to generate new successor
   ///  nodes by processing the 'effects' of a CFG element.
   void processCFGElement(const CFGElement E, ExplodedNode *Pred,

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

@@ -305,6 +305,14 @@ static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
   return nullptr;
 }
 
+const MemRegion *getOriginBaseRegion(const MemRegion *Reg) {
+  Reg = Reg->getBaseRegion();
+  while (const auto *SymReg = dyn_cast<SymbolicRegion>(Reg)) {
+    Reg = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+  }
+  return Reg;
+}
+
 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()) {
@@ -340,19 +349,45 @@ std::optional<std::string> printReferrer(const MemRegion *Referrer) {
   return buf;
 }
 
+/// Check whether \p Region refers to a freshly minted symbol after an opaque
+/// function call.
+bool isInvalidatedSymbolRegion(const MemRegion *Region) {
+  const auto *SymReg = Region->getAs<SymbolicRegion>();
+  if (!SymReg)
+    return false;
+  SymbolRef Symbol = SymReg->getSymbol();
+
+  const auto *DerS = dyn_cast<SymbolDerived>(Symbol);
+  return DerS && isa_and_nonnull<SymbolConjured>(DerS->getParentSymbol());
+}
+
 void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
                                               CheckerContext &Ctx) const {
   if (!ChecksEnabled[CK_StackAddrEscapeChecker])
     return;
 
   ExplodedNode *Node = Ctx.getPredecessor();
 
+  bool ExitingTopFrame =
+      Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+  if (ExitingTopFrame &&
+      Node->getLocation().getTag() == ExprEngine::cleanupNodeTag() &&
+      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
@@ -368,24 +403,51 @@ 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;
     }
 
+    // Keep track of the variables that were invalidated through an opaque
+    // function call. Even if the initial values of such variables were bound to
+    // an address of a local variable, we cannot claim anything now, at the
+    // function exit, so skip them to avoid false positives.
+    void recordInInvalidatedRegions(const MemRegion *Region) {
+      if (isInvalidatedSymbolRegion(Region))
+        ExcludedRegions.insert(getOriginBaseRegion(Region));
+    }
+
   public:
     SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V;
+    // ExcludedRegions are skipped from reporting.
+    // I.e., if a referrer in this set, skip the related bug report.
+    // It is useful to avoid false positive for the variables that were
+    // reset to a conjured value after an opaque function call.
+    llvm::SmallPtrSet<const MemRegion *, 4> ExcludedRegions;
 
-    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 {
+      recordInInvalidatedRegions(Region);
       const MemRegion *VR = Val.getAsRegion();
       if (!VR)
         return true;
@@ -394,15 +456,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);
@@ -423,6 +486,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.ExcludedRegions.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/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -1072,8 +1072,6 @@ void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out,
       CleanedState, SFC, SymReaper);
 
   // Process any special transfer function for dead symbols.
-  // A tag to track convenience transitions, which can be removed at cleanup.
-  static SimpleProgramPointTag cleanupTag(TagProviderName, "Clean Node");
   // Call checkers with the non-cleaned state so that they could query the
   // values of the soon to be dead symbols.
   ExplodedNodeSet CheckedSet;
@@ -1102,10 +1100,15 @@ void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out,
     // generate a transition to that state.
     ProgramStateRef CleanedCheckerSt =
         StateMgr.getPersistentStateWithGDM(CleanedState, CheckerState);
-    Bldr.generateNode(DiagnosticStmt, I, CleanedCheckerSt, &cleanupTag, K);
+    Bldr.generateNode(DiagnosticStmt, I, CleanedCheckerSt, cleanupNodeTag(), K);
   }
 }
 
+const ProgramPointTag *ExprEngine::cleanupNodeTag() {
+  static SimpleProgramPointTag cleanupTag(TagProviderName, "Clean Node");
+  return &cleanupTag;
+}
+
 void ExprEngine::ProcessStmt(const Stmt *currStmt, ExplodedNode *Pred) {
   // Reclaim any unnecessary nodes in the ExplodedGraph.
   G.reclaimRecentlyAllocatedNodes();

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