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

[Donát Nagy via cfe-commits]

================
@@ -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());
+        }
+      }
+    }
----------------
NagyDonat wrote:

I'm uncomfortable with the idea of using `getOriginRegion()` to propagate invalidation. I can accept using `getOriginRegion()` to insert a variable name into a bug report error message, because there it has limited potential to cause harm: it only affects the message.

However, if you accidentally invalidate the wrong variable, then you discard relevant information from the analyzer state, and that can cause hard-to-debug false positives. (In general an invalidation FP looks like (1) the analyzer enters an `if (ptr)` branch, then (2) `ptr` gets invalidated accidentally, then (3) the analyzer enters an `if (!ptr)` branch, because it forgot its earlier assumption.)

It is especially problematic to connect this (as far as I see widely applied) behavior change as the side effect of enabling an innocent-looking checker. The users would be very surprised if enabling this checker "magically" introduced a few hard-to-debug confusing false positives (which are emitted by totally unrelated checkers).

If you really want to propagate invalidation to origin regions, you should place this in the general invalidation logic of the engine (probably behind an analyzer config option). However if this is just a "nice to have" extra feature for your commit, then I'd suggest just skipping it.

(Disclaimer: I'm basing this review on a general "extra invalidations are problematic" principle, did not exactly analyze all the potential consequences of this particular source of invalidations. Perhaps @haoNoQ or somebody else can say more.)



https://github.com/llvm/llvm-project/pull/105648