[clang] [analyzer] Remove some false negatives in StackAddrEscapeChecker (PR #125638)

[Michael Flanders via cfe-commits]

================
@@ -247,45 +240,134 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
   }
 }
 
-void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
-                                          CheckerContext &C) const {
-  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
-    return;
+/// A visitor made for use with a ScanReachableSymbols scanner, used
+/// for finding stack regions within an SVal that live on the current
+/// stack frame of the given checker context. This visitor excludes
+/// NonParamVarRegion that data is bound to in a BlockDataRegion's
+/// bindings, since these are likely uninteresting, e.g., in case a
+/// temporary is constructed on the stack, but it captures values
+/// that would leak.
+class FindStackRegionsSymbolVisitor final : public SymbolVisitor {
+  CheckerContext &Ctxt;
+  const StackFrameContext *StackFrameContext;
+  SmallVector<const MemRegion *> &EscapingStackRegions;
 
-  const Expr *RetE = RS->getRetValue();
-  if (!RetE)
-    return;
-  RetE = RetE->IgnoreParens();
+public:
+  explicit FindStackRegionsSymbolVisitor(
+      CheckerContext &Ctxt,
+      SmallVector<const MemRegion *> &StorageForStackRegions)
+      : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()),
+        EscapingStackRegions(StorageForStackRegions) {}
 
-  SVal V = C.getSVal(RetE);
-  const MemRegion *R = V.getAsRegion();
-  if (!R)
-    return;
+  bool VisitSymbol(SymbolRef sym) override { return true; }
 
-  if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
-    checkReturnedBlockCaptures(*B, C);
+  bool VisitMemRegion(const MemRegion *MR) override {
+    SaveIfEscapes(MR);
 
-  if (!isa<StackSpaceRegion>(R->getMemorySpace()) || isNotInCurrentFrame(R, C))
-    return;
+    if (const BlockDataRegion *BDR = MR->getAs<BlockDataRegion>())
+      return VisitBlockDataRegionCaptures(BDR);
+
+    return true;
+  }
+
+private:
+  void SaveIfEscapes(const MemRegion *MR) {
+    const StackSpaceRegion *SSR =
+        MR->getMemorySpace()->getAs<StackSpaceRegion>();
+    if (SSR && SSR->getStackFrame() == StackFrameContext)
+      EscapingStackRegions.push_back(MR);
+  }
+
+  bool VisitBlockDataRegionCaptures(const BlockDataRegion *BDR) {
+    for (auto Var : BDR->referenced_vars()) {
+      SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion());
+      const MemRegion *Region = Val.getAsRegion();
+      if (Region) {
+        SaveIfEscapes(Region);
+        VisitMemRegion(Region);
+      }
+    }
+
+    return false;
+  }
+};
+
+/// Given some memory regions that are flagged by FindStackRegionsSymbolVisitor,
+/// this function filters out memory regions that are being returned that are
+/// likely not true leaks:
+/// 1. If returning a block data region that has stack memory space
+/// 2. If returning a constructed object that has stack memory space
+static SmallVector<const MemRegion *>
+FilterReturnExpressionLeaks(const SmallVector<const MemRegion *> &MaybeEscaped,
+                            CheckerContext &C, const Expr *RetE, SVal &RetVal) {
+
+  SmallVector<const MemRegion *> WillEscape;
+
+  const MemRegion *RetRegion = RetVal.getAsRegion();
 
   // Returning a record by value is fine. (In this case, the returned
   // expression will be a copy-constructor, possibly wrapped in an
   // ExprWithCleanups node.)
   if (const ExprWithCleanups *Cleanup = dyn_cast<ExprWithCleanups>(RetE))
     RetE = Cleanup->getSubExpr();
-  if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
-    return;
+  bool IsConstructExpr =
+      isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType();
 
   // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied
   // so the stack address is not escaping here.
+  bool IsCopyAndAutoreleaseBlockObj = false;
   if (const auto *ICE = dyn_cast<ImplicitCastExpr>(RetE)) {
-    if (isa<BlockDataRegion>(R) &&
-        ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
-      return;
-    }
+    IsCopyAndAutoreleaseBlockObj =
+        isa_and_nonnull<BlockDataRegion>(RetRegion) &&
+        ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject;
+  }
+
+  for (const MemRegion *MR : MaybeEscaped) {
+    if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr))
+      continue;
+
+    // If this is a construct expr of an unelided return value copy, then don't
+    // warn about returning a region that currently lives on the stack.
+    if (IsConstructExpr && RetVal.getAs<nonloc::LazyCompoundVal>() &&
----------------
Flandini wrote:

> I think we are only interested in stack memory regions that are behind a pointer.

I think there are still some cases where warning on the returned pointer is also wanted, like this one, or maybe more complicated versions of this that normal compilation warnings wouldn't catch
```
int *test() {
  int x = 14;
  return &x;
}
```

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