Revert "[analyzer] Remove some false negatives in StackAddrEscapeChec… (llvm#126614)

…ker (llvm#125638)"

This reverts commit 7ba3c55.

Co-authored-by: Gabor Horvath <[email protected]>

Author: 2 people

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

@@ -54,8 +54,8 @@ class StackAddrEscapeChecker
                                   CheckerContext &C) const;
   void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B,
                                        CheckerContext &C) const;
-  void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion,
-                           const Expr *RetE) const;
+  void EmitStackError(CheckerContext &C, const MemRegion *R,
+                      const Expr *RetE) const;
   bool isSemaphoreCaptured(const BlockDecl &B) const;
   static SourceRange genName(raw_ostream &os, const MemRegion *R,
                              ASTContext &Ctx);
@@ -147,38 +147,21 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
   return Regions;
 }
 
-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 {
+void StackAddrEscapeChecker::EmitStackError(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());
-  EmitReturnedAsPartOfError(os, C.getSVal(RetE), R);
-
+  os << " returned to caller";
   auto report =
       std::make_unique<PathSensitiveBugReport>(*BT_returnstack, os.str(), N);
   report->addRange(RetE->getSourceRange());
@@ -226,6 +209,30 @@ 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])
@@ -240,128 +247,45 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
   }
 }
 
-/// 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;
-
-public:
-  explicit FindStackRegionsSymbolVisitor(
-      CheckerContext &Ctxt,
-      SmallVectorImpl<const MemRegion *> &StorageForStackRegions)
-      : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()),
-        EscapingStackRegions(StorageForStackRegions) {}
-
-  bool VisitSymbol(SymbolRef sym) override { return true; }
-
-  bool VisitMemRegion(const MemRegion *MR) override {
-    SaveIfEscapes(MR);
-
-    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);
-      }
-    }
+void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
+                                          CheckerContext &C) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+    return;
 
-    return false;
-  }
-};
+  const Expr *RetE = RS->getRetValue();
+  if (!RetE)
+    return;
+  RetE = RetE->IgnoreParens();
 
-/// 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) {
+  SVal V = C.getSVal(RetE);
+  const MemRegion *R = V.getAsRegion();
+  if (!R)
+    return;
 
-  SmallVector<const MemRegion *> WillEscape;
+  if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
+    checkReturnedBlockCaptures(*B, C);
 
-  const MemRegion *RetRegion = RetVal.getAsRegion();
+  if (!isa<StackSpaceRegion>(R->getMemorySpace()) || isNotInCurrentFrame(R, C))
+    return;
 
   // 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();
-  bool IsConstructExpr =
-      isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType();
+  if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
+    return;
 
   // 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)) {
-    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);
+    if (isa<BlockDataRegion>(R) &&
+        ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
+      return;
+    }
   }
 
-  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);
+  EmitStackError(C, R, RetE);
 }
 
 static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {

clang/test/Analysis/copy-elision.cpp

@@ -154,26 +154,20 @@ 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+  return ClassWithoutDestructor(v);
   // 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+  return make1(v);
   // 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithoutDestructor' returned to caller}}
+  return make2(v);
   // 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}}
@@ -304,26 +298,21 @@ 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+  return ClassWithDestructor(v);
   // 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+  return make1(v);
   // 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); // no-elide-warning{{Address of stack memory associated with temporary object of type 'ClassWithDestructor' returned to caller}}
+  return make2(v);
   // 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}}