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

leaking in ARC mode

When ARC (automatic reference count) 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.
With this commit, the static analyzer now puts a block object in an
"unknown" region if ARC is enabled because it is up to the
implementation to choose whether to put the object on stack initially
(then move to heap when needed) or in heap directly under ARC.
Therefore, the `StackAddrEscapeChecker` has no need to know
specifically about ARC at all and it will not report errors on objects
in "unknown" regions.

Reviewed By: NoQ (Artem Dergachev)

Differential Revision: https://reviews.llvm.org/D131009

diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index e6cea0f..c4b7411 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -61,7 +61,6 @@ private:
                              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
@@ -110,13 +109,6 @@ SourceRange StackAddrEscapeChecker::genName(raw_ostream &os, const MemRegion *R,
   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());
@@ -214,7 +206,7 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures(
 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)
@@ -267,8 +259,7 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
   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
@@ -348,8 +339,7 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
       // 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;
     }

diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index bb64cbc..482862a 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1076,14 +1076,18 @@ MemRegionManager::getBlockDataRegion(const BlockCodeRegion *BC,
     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();

diff --git a/clang/test/Analysis/stack-capture-leak-arc.mm b/clang/test/Analysis/stack-capture-leak-arc.mm
index 1ffee93..921487a 100644 (file)
--- a/clang/test/Analysis/stack-capture-leak-arc.mm
+++ b/clang/test/Analysis/stack-capture-leak-arc.mm
@@ -8,6 +8,7 @@ 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;
@@ -187,3 +188,40 @@ void test_dispatch_barrier_sync() {
   }
   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);
+  };
+}

diff --git a/clang/test/Analysis/stack-capture-leak-no-arc.mm b/clang/test/Analysis/stack-capture-leak-no-arc.mm
index 33829f5..84bbc5f 100644 (file)
--- a/clang/test/Analysis/stack-capture-leak-no-arc.mm
+++ b/clang/test/Analysis/stack-capture-leak-no-arc.mm
@@ -4,6 +4,7 @@ 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;
@@ -35,3 +36,46 @@ dispatch_block_t test_block_inside_block_async_leak() {
   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();
+}