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

[Gábor Horváth 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>() &&
----------------
Xazax-hun wrote:

I am wondering if this is the right solution here. I am a bit concerned that there are some other cases that we missed here. I think we are only interested in stack memory regions that are behind a pointer.

But this is also not entirely true. Imagine the following case:

```
DoesDeepCopy myFunc() {
   int local;
   DoesDeepCopy l(&local);
   return l;
};
```

Here, if `DoesDeepCopy`'s ctor would copy the value behind the pointer, the code above is fine. On the second though, I wonder if NRVO can actually kick in making this code unsafe. 

That being said, if the copy ctor is actually invoked, hopefully we would observe the effects of that, the fields would get invalidated, and we would not see the memory region from the stack. 

Overall, I think some scenarios here might need more tests. 

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