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

Fixes llvm#123459.

Previously, when the StackAddrEscapeChecker checked return values, it
did not scan into the structure of the return SVal. Now it does, and we
can catch some more false negatives that were already mocked out in the
tests in addition to those mentioned in
llvm#123459.

The warning message at the moment for these newly caught leaks is not
great. I think they would be better if they had a better trace of why
and how the region leaks. If y'all are happy with these changes, I would
try to improve these warnings and work on normalizing this SVal checking
on the `checkEndFunction` side of the checker also.

Two of the stack address leak test cases now have two warnings, one
warning from return expression checking and another from`
checkEndFunction` `iterBindings` checking. For these two cases, I prefer
the warnings from the return expression checking, but I couldn't figure
out a way to drop the `checkEndFunction` without breaking other
`checkEndFunction` warnings that we do want. Thoughts here?

Author: Flandini

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

@@ -54,8 +54,8 @@ class StackAddrEscapeChecker
                                   CheckerContext &C) const;
   void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B,
                                        CheckerContext &C) const;
-  void EmitStackError(CheckerContext &C, const MemRegion *R,
-                      const Expr *RetE) const;
+  void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion,
+                           const Expr *RetE) const;
   bool isSemaphoreCaptured(const BlockDecl &B) const;
   static SourceRange genName(raw_ostream &os, const MemRegion *R,
                              ASTContext &Ctx);
@@ -147,21 +147,38 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
   return Regions;
 }
 
-void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
-                                            const MemRegion *R,
-                                            const Expr *RetE) const {
+static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, SVal ReturnedVal,
+                                      const MemRegion *LeakedRegion) {
+  if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) {
+    if (isa<BlockDataRegion>(ReturnedRegion)) {
+      OS << " is captured by a returned block";
+      return;
+    }
+  }
+
+  // Generic message
+  OS << " returned to caller";
+}
+
+void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C,
+                                                 const MemRegion *R,
+                                                 const Expr *RetE) const {
   ExplodedNode *N = C.generateNonFatalErrorNode();
   if (!N)
     return;
   if (!BT_returnstack)
     BT_returnstack = std::make_unique<BugType>(
         CheckNames[CK_StackAddrEscapeChecker],
         "Return of address to stack-allocated memory");
+
   // Generate a report for this bug.
   SmallString<128> buf;
   llvm::raw_svector_ostream os(buf);
+
+  // Error message formatting
   SourceRange range = genName(os, R, C.getASTContext());
-  os << " returned to caller";
+  EmitReturnedAsPartOfError(os, C.getSVal(RetE), R);
+
   auto report =
       std::make_unique<PathSensitiveBugReport>(*BT_returnstack, os.str(), N);
   report->addRange(RetE->getSourceRange());
@@ -209,30 +226,6 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures(
   }
 }
 
-void StackAddrEscapeChecker::checkReturnedBlockCaptures(
-    const BlockDataRegion &B, CheckerContext &C) const {
-  for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
-    if (isNotInCurrentFrame(Region, C))
-      continue;
-    ExplodedNode *N = C.generateNonFatalErrorNode();
-    if (!N)
-      continue;
-    if (!BT_capturedstackret)
-      BT_capturedstackret = std::make_unique<BugType>(
-          CheckNames[CK_StackAddrEscapeChecker],
-          "Address of stack-allocated memory is captured");
-    SmallString<128> Buf;
-    llvm::raw_svector_ostream Out(Buf);
-    SourceRange Range = genName(Out, Region, C.getASTContext());
-    Out << " is captured by a returned block";
-    auto Report = std::make_unique<PathSensitiveBugReport>(*BT_capturedstackret,
-                                                           Out.str(), N);
-    if (Range.isValid())
-      Report->addRange(Range);
-    C.emitReport(std::move(Report));
-  }
-}
-
 void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
                                           CheckerContext &C) const {
   if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
@@ -247,45 +240,128 @@ 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;
+  SmallVectorImpl<const MemRegion *> &EscapingStackRegions;
 
-  const Expr *RetE = RS->getRetValue();
-  if (!RetE)
-    return;
-  RetE = RetE->IgnoreParens();
+public:
+  explicit FindStackRegionsSymbolVisitor(
+      CheckerContext &Ctxt,
+      SmallVectorImpl<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 SmallVectorImpl<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;
+
+    WillEscape.push_back(MR);
   }
 
-  EmitStackError(C, R, RetE);
+  return WillEscape;
+}
+
+/// For use in finding regions that live on the checker context's current
+/// stack frame, deep in the SVal representing the return value.
+static SmallVector<const MemRegion *>
+FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) {
+  SmallVector<const MemRegion *> FoundStackRegions;
+
+  FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions);
+  ScanReachableSymbols Scanner(C.getState(), Finder);
+  Scanner.scan(RetVal);
+
+  return FilterReturnExpressionLeaks(FoundStackRegions, C, RetE, RetVal);
+}
+
+void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
+                                          CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+    return;
+
+  const Expr *RetE = RS->getRetValue();
+  if (!RetE)
+    return;
+  RetE = RetE->IgnoreParens();
+
+  SVal V = C.getSVal(RetE);
+
+  SmallVector<const MemRegion *> EscapedStackRegions =
+      FindEscapingStackRegions(C, RetE, V);
+
+  for (const MemRegion *ER : EscapedStackRegions)
+    EmitReturnLeakError(C, ER, RetE);
 }
 
 static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {

clang/test/Analysis/copy-elision.cpp

@@ -154,20 +154,26 @@ class ClassWithoutDestructor {
   void push() { v.push(this); }
 };
 
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary 
 ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
-  return ClassWithoutDestructor(v);
+  return ClassWithoutDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
   // no-elide-warning@-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}}
 }
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary 
 ClassWithoutDestructor make2(AddressVector<ClassWithoutDestructor> &v) {
-  return make1(v);
+  return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
   // no-elide-warning@-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}}
 }
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary 
 ClassWithoutDestructor make3(AddressVector<ClassWithoutDestructor> &v) {
-  return make2(v);
+  return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
   // no-elide-warning@-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}}
@@ -298,21 +304,26 @@ to by the caller variable 'v' upon returning to the caller}}
 #endif
 }
 
-
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary
 ClassWithDestructor make1(AddressVector<ClassWithDestructor> &v) {
-  return ClassWithDestructor(v);
+  return ClassWithDestructor(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithDestructor' is still referred \
 to by the caller variable 'v' upon returning to the caller}}
 }
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary
 ClassWithDestructor make2(AddressVector<ClassWithDestructor> &v) {
-  return make1(v);
+  return make1(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithDestructor' is still referred \
 to by the caller variable 'v' upon returning to the caller}}
 }
+// Two warnings on no-elide: arg v holds the address of the temporary, and we
+// are returning an object which holds v which holds the address of the temporary
 ClassWithDestructor make3(AddressVector<ClassWithDestructor> &v) {
-  return make2(v);
+  return make2(v); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
   // no-elide-warning@-1 {{Address of stack memory associated with temporary \
 object of type 'ClassWithDestructor' is still referred \
 to by the caller variable 'v' upon returning to the caller}}