[analyzer] Fixing a bug raising false positives of stack block object leaking under ARC

[ziqingluo-90]

Summary

When ARC is enabled, (objective-c) block objects are automatically retained and released thus they do not leak.
Without ARC, they still can leak from an expiring stack frame like other stack variables.

This patch simply adds the ARC checking condition onto the changes made in https://reviews.llvm.org/D107078. The checker already checks the same condition for global variables.

[NoQ]

Aha great, I see you've found that there's already an existing solution for this problem! I'm questioning this solution though, maybe a more general solution could have helped us avoid this problem altogether.

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

304–306

Aha ok, it sounds like we can no longer be sure that the block is on the stack at this point, did I get it right?

In this case I think it's more productive to have the block's memory space be UnknownSpaceRegion from the start, so that it fell through the memory space check, both here and at other call sites of isArcManagedBlock() (so it can be removed), and in any other code that relies on memory spaces (so this mistake is never made again).

[ziqingluo-90]

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

304–306

" ..., did I get it right?" Yes.

This suggestion makes sense to me. To my understanding, I need to modify the symbolic execution engine to address it. So shall I do it in a new patch?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

304–306

There's no need to make a new review, you can update the current review. The test stays the same after all, who knows maybe we'll end up reverting to this solution. It's nice to have all approaches considered and explored behind the same link that ultimately ends up in the commit message.

[ziqingluo-90]

Addressing @NoQ 's comment. Let the symbolic execution simulator put a block object in an UnknownSpaceRegion from the start if ARC is enabled. Because in such cases whether the block is on stack or in heap depends on the implementation. And, those blocks will be moved to the heap when necessary. So they will never trigger a stack address leak issue.

Removing those ARC checking in StackAddrEscapeChecker since they are no longer needed.

[NoQ]

Aha perfect, now the entire static analyzer knows how to work with these regions!

I have one tiny remark and I think we can commit.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

1079–1084

MemRegionManager maintains a reference to the current ASTContext, it's slightly easier to use it directly or through getContext().

There's also a general recommendation to avoid Decl::getASTContext() because it isn't an O(1) operation; instead, it traverses through parent decls all the way up to TranslationUnitDecl which maintains the actual reference to ASTContext. I'm not sure how bad this is, but if it can be easily avoided we probably should.

[ziqingluo-90]

In D131009#3744416, @NoQ wrote:

Aha perfect, now the entire static analyzer knows how to work with these regions!

I have one tiny remark and I think we can commit.

Thank you! I will make the change then directly commit.

[NoQ]

Works for me!

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B,
                                        CheckerContext &C) 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);
   static SmallVector<const MemRegion *, 4>
   getCapturedStackRegions(const BlockDataRegion &B, CheckerContext &C);
-  static bool isArcManagedBlock(const MemRegion *R, CheckerContext &C);
   static bool isNotInCurrentFrame(const MemRegion *R, CheckerContext &C);
 };
 } // namespace
 
 SourceRange StackAddrEscapeChecker::genName(raw_ostream &os, const MemRegion *R,
                                             ASTContext &Ctx) {
   // Get the base region, stripping away fields and elements.
   R = R->getBaseRegion();
if (const auto *CR = dyn_cast<CompoundLiteralRegion>(R)) {
     range = TOR->getExpr()->getSourceRange();
   } else {
     llvm_unreachable("Invalid region in ReturnStackAddressChecker.");
   }
 
   return range;
 }
 
-bool StackAddrEscapeChecker::isArcManagedBlock(const MemRegion *R,
-                                               CheckerContext &C) {
-  assert(R && "MemRegion should not be null");
-  return C.getASTContext().getLangOpts().ObjCAutoRefCount &&
-         isa<BlockDataRegion>(R);
-}
-
 bool StackAddrEscapeChecker::isNotInCurrentFrame(const MemRegion *R,
                                                  CheckerContext &C) {
   const StackSpaceRegion *S = cast<StackSpaceRegion>(R->getMemorySpace());
   return S->getStackFrame() != C.getStackFrame();
 }
 
 bool StackAddrEscapeChecker::isSemaphoreCaptured(const BlockDecl &B) const {
   if (!dispatch_semaphore_tII)
if (Range.isValid())
       Report->addRange(Range);
     C.emitReport(std::move(Report));
   }
 }
 
 void StackAddrEscapeChecker::checkReturnedBlockCaptures(
     const BlockDataRegion &B, CheckerContext &C) const {
   for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
-    if (isArcManagedBlock(Region, C) || isNotInCurrentFrame(Region, C))
+    if (isNotInCurrentFrame(Region, C))
       continue;
     ExplodedNode *N = C.generateNonFatalErrorNode();
     if (!N)
       continue;
     if (!BT_capturedstackret)
       BT_capturedstackret = std::make_unique<BuiltinBug>(
           CheckNames[CK_StackAddrEscapeChecker],
           "Address of stack-allocated memory is captured");
void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
   SVal V = C.getSVal(RetE);
   const MemRegion *R = V.getAsRegion();
   if (!R)
     return;
 
   if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
     checkReturnedBlockCaptures(*B, C);
 
-  if (!isa<StackSpaceRegion>(R->getMemorySpace()) ||
-      isNotInCurrentFrame(R, C) || isArcManagedBlock(R, C))
+  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();
   if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
private:
     CheckerContext &Ctx;
     const StackFrameContext *PoppedFrame;
 
     /// Look for stack variables referring to popped stack variables.
     /// Returns true only if it found some dangling stack variables
     /// referred by an other stack variable from different stack frame.
     bool checkForDanglingStackVariable(const MemRegion *Referrer,
                                        const MemRegion *Referred) {
       const auto *ReferrerMemSpace =
           Referrer->getMemorySpace()->getAs<StackSpaceRegion>();
       const auto *ReferredMemSpace =
           Referred->getMemorySpace()->getAs<StackSpaceRegion>();
 
       if (!ReferrerMemSpace || !ReferredMemSpace)
         return false;
 
       const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
       const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
 
bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region,
         return true;
 
       if (checkForDanglingStackVariable(Region, VR))
         return true;
 
       // Check the globals for the same.
       if (!isa<GlobalsSpaceRegion>(Region->getMemorySpace()))
         return true;
-      if (VR && VR->hasStackStorage() && !isArcManagedBlock(VR, Ctx) &&
-          !isNotInCurrentFrame(VR, Ctx))
+      if (VR && VR->hasStackStorage() && !isNotInCurrentFrame(VR, Ctx))
         V.emplace_back(Region, VR);
       return true;
     }
   };
 
   CallBack Cb(Ctx);
   State->getStateManager().getStoreManager().iterBindings(State->getStore(),
                                                           Cb);

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

MemRegionManager::getBlockDataRegion(const BlockCodeRegion *BC,
                                      unsigned blockCount) {
   const MemSpaceRegion *sReg = nullptr;
   const BlockDecl *BD = BC->getDecl();
   if (!BD->hasCaptures()) {
     // This handles 'static' blocks.
     sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
   }
   else {
-    if (LC) {
+    bool IsArcManagedBlock = Ctx.getLangOpts().ObjCAutoRefCount;
+
+    // ARC managed blocks can be initialized on stack or directly in heap
+    // depending on the implementations.  So we initialize them with
+    // UnknownRegion.
+    if (!IsArcManagedBlock && LC) {
       // FIXME: Once we implement scope handling, we want the parent region
       // to be the scope.
       const StackFrameContext *STC = LC->getStackFrame();
       assert(STC);
       sReg = getStackLocalsRegion(STC);
-    }
-    else {
+    } else {
       // We allow 'LC' to be NULL for cases where want BlockDataRegions
       // without context-sensitivity.
       sReg = getUnknownRegion();
     }
   }
 
   return getSubRegion<BlockDataRegion>(BC, LC, blockCount, sReg);
 }

clang/test/Analysis/stack-capture-leak-arc.mm

 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -fobjc-arc -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
 void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
 typedef long dispatch_once_t;
 void dispatch_once(dispatch_once_t *predicate, dispatch_block_t block);
 typedef long dispatch_time_t;
 void dispatch_after(dispatch_time_t when, dispatch_queue_t queue, dispatch_block_t block);
 void dispatch_barrier_sync(dispatch_queue_t queue, dispatch_block_t block);
+void f(int);
 
 extern dispatch_queue_t queue;
 extern dispatch_once_t *predicate;
 extern dispatch_time_t when;
 
 void test_block_expr_async() {
   int x = 123;
   int *p = &x;
for (int n = 0; n < 16; ++n) {
     // FIXME: Should not warn. The dispatch_barrier_sync() call ensures
     // that the block does not outlive 'buf'.
     dispatch_async(queue, ^{ // expected-warning{{Address of stack memory associated with local variable 'buf' is captured by an asynchronously-executed block}}
       (void)ptr;
     });
   }
   dispatch_barrier_sync(queue, ^{});
 }
+
+void output_block(dispatch_block_t * blk) {
+  int x = 0;
+  *blk = ^{ f(x); };
+}
+
+// Block objects themselves can never leak under ARC.
+void test_no_block_leak() {
+  __block dispatch_block_t blk;
+  int x = 0;
+  dispatch_block_t p = ^{
+    blk = ^{
+      f(x);
+    };
+  };
+  p();
+  blk();
+  output_block(&blk);
+  blk();
+}
+
+// Block objects do not leak under ARC but stack variables of
+// non-object kind indirectly referred by a block can leak.
+dispatch_block_t test_block_referencing_variable_leak() {
+  int x = 0;
+  __block int * p = &x;
+  __block int * q = &x;
+  
+  dispatch_async(queue, ^{// expected-warning {{Address of stack memory associated with local variable 'x' is captured by an asynchronously-executed block \
+[alpha.core.StackAddressAsyncEscape]}}
+      ++(*p);
+    });
+  return (dispatch_block_t) ^{// expected-warning {{Address of stack memory associated with local variable 'x' is captured by a returned block \
+[core.StackAddressEscape]}}
+    ++(*q);
+  };
+}

clang/test/Analysis/stack-capture-leak-no-arc.mm

 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
 void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
 extern dispatch_queue_t queue;
+void f(int);
 
 void test_block_inside_block_async_no_leak() {
   int x = 123;
   int *p = &x;
   void (^inner)(void) = ^void(void) {
     int y = x;
     ++y; 
   };
dispatch_block_t test_block_inside_block_async_leak() {
   void (^outer)(void) = ^void(void) {
     int z = x;
     ++z;
     inner(); 
   }; 
   return outer; // expected-warning-re{{Address of stack-allocated block declared on line {{.+}} is captured by a returned block}}
 }
 
+// The block literal defined in this function could leak once being
+// called.
+void output_block(dispatch_block_t * blk) {
+  int x = 0;
+  *blk = ^{ f(x); }; // expected-warning {{Address of stack-allocated block declared on line 43 is still referred to by the stack variable 'blk' upon returning to the caller.  This will be a dangling reference [core.StackAddressEscape]}}
+}
+
+// The block literal captures nothing thus is treated as a constant.
+void output_constant_block(dispatch_block_t * blk) {
+  *blk = ^{ };
+}
+
+// A block can leak if it captures at least one variable and is not
+// under ARC when its' stack frame expires.
+void test_block_leak() {
+  __block dispatch_block_t blk;
+  int x = 0;
+  dispatch_block_t p = ^{
+    blk = ^{ // expected-warning {{Address of stack-allocated block declared on line 57 is still referred to by the stack variable 'blk' upon returning to the caller.  This will be a dangling reference [core.StackAddressEscape]}}
+      f(x);
+    };
+  };
+
+  p();
+  blk();
+  output_block(&blk);
+  blk();
+}
+
+// A block captures nothing is a constant thus never leaks.
+void test_constant_block_no_leak() {
+  __block dispatch_block_t blk;
+  dispatch_block_t p = ^{
+    blk = ^{
+      f(0);
+    };
+  };
+  
+  p();
+  blk();
+  output_constant_block(&blk);
+  blk();
+}