[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:

This line is only for removing warnings. It is a little loose and maybe could cause some false negatives. Is there a way here to relate the lazy compound val that would be returned and the temp object region in an example like this from copy-elision.cpp test:
```
template <typename T> struct AddressVector {
  T *buf[20];
  int len;

  AddressVector() : len(0) {}

  void push(T *t) {
    buf[len] = t;
    ++len;
  }
};

class ClassWithoutDestructor {
  AddressVector<ClassWithoutDestructor> &v;

public:
  ClassWithoutDestructor(AddressVector<ClassWithoutDestructor> &v) : v(v) {
    push();
  }

  ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { push(); }
  ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) { push(); }

  void push() { v.push(this); }
};

ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
  return ClassWithoutDestructor(v); 
  // no-elide-warning at -1 {{Address of stack memory associated with temporary \
object of type 'ClassWithoutDestructor' is still \
referred to by the caller variable 'v' upon returning to the caller}}
}
```
Without some check like this, then the line with `return ClassWithoutDestructor(v); ` will warn, and I think it should not under `-analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -std=c++11`. Is there a better way to relate the lazy compound val of the return expr and the temp object region that is created?


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