[analyzer] Diagnose stack leaks via block captures

[alexander-shaposhnikov]

Summary

This diff extends StackAddrEscapeChecker to catch stack addr leaks via block captures if the block is executed
asynchronously.
Test plan: make check-all

[alexander-shaposhnikov]

Add positive tests

[xazax.hun]

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

145 ↗

(On Diff #120908)

This will stop the analysis on this execution path. Is this desired? Usually, we stop the execution when there is no way to model the program state after the error, e.g.: after a division by zero. In this case the stack address escaped but it wasn't dereferenced (yet), so I think it might be safe to continue the analysis on this path. What do you think?

156 ↗

(On Diff #120908)

The variable should start with an uppercase letter.

[NoQ]

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

128 ↗

(On Diff #120908)

dispatch_once isn't really asynchronous; i think it's out of place here.

Also we model it in the body farm, so i guess any errors inside its block would be caught anyway.

136–139 ↗

(On Diff #120908)

If you like, feel free to add support for for (const auto &Item: Block->referenced_vars()) loops into BlockDataRegion :)

144 ↗

(On Diff #120908)

getMemorySpace() is never null. Every region resides in a memory space.

153 ↗

(On Diff #120908)

Not sure - the rest of the checker's code seems to be making use of the returned SourceRange by adding to the report, do we need that here?

test/Analysis/stack-async-leak.m

1 ↗

(On Diff #120908)

We don't add -analyzer-store=region nowadays; i don't think it's useful (though it doesn't hurt).

60 ↗

(On Diff #120908)

You may enjoy adding an extra note piece (as in D24278) to the offending statement within the block. It might be helpful if it's not obvious which block is being executed or which pointer points to the leaking variable.

Also it might be a good idea to think about a visitor that would track the block pointer to highlight where it was initialized with a concrete block code.

Though probably it's not super useful because we're most likely staying within a single function with this checker.

By the way, do we warn (or want to warn) on returning a block from a function, when this block captures a local variable?

[NoQ]

What i was trying to say is - Hey this check looks useful!~ Great idea.

[alexander-shaposhnikov]

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

144 ↗

(On Diff #120908)

i was looking at StackAddrEscapeChecker::checkPreStmt - should it be fixed there as well, or that one is different ?

145 ↗

(On Diff #120908)

I think you are right, would be better to make it non-fatal

[alexander-shaposhnikov]

Address the comments, add more tests

[alexander-shaposhnikov]

test/Analysis/stack-async-leak.m

60 ↗

(On Diff #120908)

regarding tracking - yeah, i agree, that would be good, although i would prefer to make progress incrementally and add this part later.

By the way, do we warn (or want to warn) on returning a block from a function, when this >block captures a local variable?

good question - yeah, now we do

[alexander-shaposhnikov]

more tests

[alexander-shaposhnikov]

cleanup

[dcoughlin]

I think this is a great idea, and I expect it to find some nasty bugs!

However, I'm worried about false positives in the following not-too-uncommon idiom:

dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
dispatch_async(some_queue, ^{

  // Do some work

  dispatch_semaphore_signal(semaphore);
});

// Do some other work concurrently with the asynchronous work

// Wait for the asynchronous work to finish
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);

What do you think about suppressing the diagnostic when the block captures a variable with type 'dispatch_semaphore_t'? This isn't a perfect suppression, but it will catch most of the cases that I have seen.

Also, does this checker diagnose when a block captures a C++ reference? If so, it would be great to add a test for that!

Some additional comments inline.

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

124 ↗

(On Diff #121100)

Could we break with tradition and add a oxygen comment describing what this method does?

133 ↗

(On Diff #121100)

Can you factor this logic out with the logic it duplicates in checkPreStmt()?

136 ↗

(On Diff #121100)

Stylistically we use isa<> for cases like these where the result of dyn_cast<> is not used.

146 ↗

(On Diff #121100)

Instead of "might leak ... " I would suggest " is captured by an asynchronously-executed block". In the context of the analyzer we use "leak" to mean "resource that is not freed" rather than in the sense of exposing information.

You'll need to make a different variant of the diagnostic text for when the block is returned rather than asynchronously executed.

185 ↗

(On Diff #121100)

Will this have a false positive if I create a block in a caller, pass it to a callee, and then the callee returns it? You may want to factor out and reuse the logic from below that compares the StackFrameContext for the current frame to the frame of the captured addresses in checkBlockCaptures().

test/Analysis/stack-async-leak.m

1 ↗

(On Diff #121100)

Can you add an additional run line testing the expected behavior when ARC is not enabled? You can pass a -DARC_ENABLED=1 or similar to distinguish between the two (for example, to test to make sure that a diagnostic is emitted under MRR but not ARC).

[alexander-shaposhnikov]

Refactor the checker, add more tests.

[alexander-shaposhnikov]

test/Analysis/stack-async-leak.m

1 ↗

(On Diff #121100)

i've tried this, it somehow gets more complicated to read / change the tests - decided to create a separate file.

[alexander-shaposhnikov]

Add more tests for blocks being passed around

[alexander-shaposhnikov]

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

68 ↗

(On Diff #121230)

c++ reference

[dcoughlin]

Thanks!

Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing.

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

121 ↗

(On Diff #121230)

I think here you will need to check whether Q is a TypedefType and compare the IdentifierInfo for its TypedefNameDecl to 'dispatch_semaphore_t' instead.

The reason is that NSObject<OS_dispatch_semaphore> is an implementation detail and in fact dispatch_semaphore_t is typedef'd to another type depending on whether the file is Objective-C(++) or not.

There is an example of the idiom we use to do IdentifierInfo comparisons in ObjCDeallocChecker. We cache the identifier in the checker class. This lets us get constant-time comparisons on identifiers (since they are interned strings).

Also: It would be good to add a comment explaining why we care whether a semaphore was captured.

316 ↗

(On Diff #121230)

We generally prefer to avoid combining style cleanups like these with functional changes except in the code that is required to be updated for the functional change. This makes it easier for future maintainer to use the commit history to understand what a commit did and why.

test/Analysis/stack-async-leak.m

1 ↗

(On Diff #121100)

That's fine!

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

144 ↗

(On Diff #121230)

Can you add a // no-warning comment to this line? We use that to indicate that we explicitly don't expect a warning there.

[alexander-shaposhnikov]

1. Add no-warning comments
2. Switch to using a cached IdentifierInfo (similar to how it's done in CheckObjCDealloc.cpp)
3. Rerun the all the tests - they are fine.

@dcoughlin - many thanks for the code review,
yeah, i will wait for the feedback from NoQ@ as well.

[alexander-shaposhnikov]

ping @NoQ

[alexander-shaposhnikov]

Fix

[NoQ]

Looks great, thanks! Minor inline stuff.

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

116 ↗

(On Diff #122194)

Because stack address escapes are not fatal errors, it is not necessarily true that any other stack frame context here would be an ancestor of the current context. It may be an escaped stack variable from a function that has exited long time ago. You might want to check the getParent() chain.

Hmm, or rather rename the function to "isNotInCurrentFrame" because that's how you seem to be using it.

173 ↗

(On Diff #122194)

Typo: capatured -> captured.

177 ↗

(On Diff #122194)

Hmm, why would such region even be in the list?

184 ↗

(On Diff #122194)

I think bugtype should describe the bug, so that the user knew what sort of bug it is by looking at the list. Maybe add "...is captured" here?

[alexander-shaposhnikov]

Address the comments

[alexander-shaposhnikov]

ping

[xazax.hun]

I found some nits, but overall I think this is getting close.

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

76 ↗

(On Diff #122507)

In case you already change these lines you could replace the type name on the left side with auto to avoid repeating types.

123 ↗

(On Diff #122507)

You can also use auto here to avoid mentioning the type twice.

192 ↗

(On Diff #122507)

How long usually these error messages are? Maybe 512 is a bit large buffer for this? Note that in case the error message is longer this is still not a hard error, it just will hit the slow path (allocating the buffer on the heap).

214 ↗

(On Diff #122507)

Maybe you wanted to mention "captured" here as well?

215 ↗

(On Diff #122507)

Maybe the error reporting part could be abstracted out to avoid code duplication with the function above?

[alexander-shaposhnikov]

lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

192 ↗

(On Diff #122507)

Note that in case the error message is longer this is still not a hard error, it just will hit the slow path

thanks, i'm aware of that.
512 was used in this file before my invasion,
the size obviously depends on what genName would generate (and inherently depends on the code style of the codebase)

[alexander-shaposhnikov]

adjust the messages, more uses of auto

[NoQ]

Thanks, looks great, please commit!~

cfe/trunk/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 using namespace ento;
 
 namespace {
-class StackAddrEscapeChecker : public Checker< check::PreStmt<ReturnStmt>,
-                                               check::EndFunction > {
+class StackAddrEscapeChecker
+    : public Checker<check::PreCall, check::PreStmt<ReturnStmt>,
+                     check::EndFunction> {
+  mutable IdentifierInfo *dispatch_semaphore_tII;
   mutable std::unique_ptr<BuiltinBug> BT_stackleak;
   mutable std::unique_ptr<BuiltinBug> BT_returnstack;
+  mutable std::unique_ptr<BuiltinBug> BT_capturedstackasync;
+  mutable std::unique_ptr<BuiltinBug> BT_capturedstackret;
 
 public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
   void checkEndFunction(CheckerContext &Ctx) const;
+
 private:
+  void checkReturnedBlockCaptures(const BlockDataRegion &B,
+                                  CheckerContext &C) const;
+  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();
   SourceManager &SM = Ctx.getSourceManager();
   SourceRange range;
   os << "Address of ";
 
   // Check if the region is a compound literal.
-  if (const CompoundLiteralRegion* CR = dyn_cast<CompoundLiteralRegion>(R)) {
+  if (const auto *CR = dyn_cast<CompoundLiteralRegion>(R)) {
     const CompoundLiteralExpr *CL = CR->getLiteralExpr();
     os << "stack memory associated with a compound literal "
           "declared on line "
-        << SM.getExpansionLineNumber(CL->getLocStart())
-        << " returned to caller";
+       << SM.getExpansionLineNumber(CL->getLocStart()) << " returned to caller";
     range = CL->getSourceRange();
-  }
-  else if (const AllocaRegion* AR = dyn_cast<AllocaRegion>(R)) {
+  } else if (const auto *AR = dyn_cast<AllocaRegion>(R)) {
     const Expr *ARE = AR->getExpr();
     SourceLocation L = ARE->getLocStart();
     range = ARE->getSourceRange();
     os << "stack memory allocated by call to alloca() on line "
        << SM.getExpansionLineNumber(L);
-  }
-  else if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) {
+  } else if (const auto *BR = dyn_cast<BlockDataRegion>(R)) {
     const BlockDecl *BD = BR->getCodeRegion()->getDecl();
     SourceLocation L = BD->getLocStart();
     range = BD->getSourceRange();
     os << "stack-allocated block declared on line "
        << SM.getExpansionLineNumber(L);
-  }
-  else if (const VarRegion *VR = dyn_cast<VarRegion>(R)) {
-    os << "stack memory associated with local variable '"
-       << VR->getString() << '\'';
+  } else if (const auto *VR = dyn_cast<VarRegion>(R)) {
+    os << "stack memory associated with local variable '" << VR->getString()
+       << '\'';
     range = VR->getDecl()->getSourceRange();
-  }
-  else if (const CXXTempObjectRegion *TOR = dyn_cast<CXXTempObjectRegion>(R)) {
+  } else if (const auto *TOR = dyn_cast<CXXTempObjectRegion>(R)) {
     QualType Ty = TOR->getValueType().getLocalUnqualifiedType();
     os << "stack memory associated with temporary object of type '";
     Ty.print(os, Ctx.getPrintingPolicy());
     os << "'";
     range = TOR->getExpr()->getSourceRange();
-  }
-  else {
+  } else {
     llvm_unreachable("Invalid region in ReturnStackAddressChecker.");
   }
 
   return range;
 }
 
-void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, const MemRegion *R,
-                                          const Expr *RetE) const {
-  ExplodedNode *N = C.generateErrorNode();
+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.getLocationContext()->getCurrentStackFrame();
+}
+
+bool StackAddrEscapeChecker::isSemaphoreCaptured(const BlockDecl &B) const {
+  if (!dispatch_semaphore_tII)
+    dispatch_semaphore_tII = &B.getASTContext().Idents.get("dispatch_semaphore_t");
+  for (const auto &C : B.captures()) {
+    const auto *T = C.getVariable()->getType()->getAs<TypedefType>();
+    if (T && T->getDecl()->getIdentifier() == dispatch_semaphore_tII)
+      return true; 
+  }
+  return false;
+}
+
+SmallVector<const MemRegion *, 4>
+StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
+                                                CheckerContext &C) {
+  SmallVector<const MemRegion *, 4> Regions;
+  BlockDataRegion::referenced_vars_iterator I = B.referenced_vars_begin();
+  BlockDataRegion::referenced_vars_iterator E = B.referenced_vars_end();
+  for (; I != E; ++I) {
+    SVal Val = C.getState()->getSVal(I.getCapturedRegion());
+    const MemRegion *Region = Val.getAsRegion();
+    if (Region && isa<StackSpaceRegion>(Region->getMemorySpace()))
+      Regions.push_back(Region);
+  }
+  return Regions;
+}
 
+void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
+                                            const MemRegion *R,
+                                            const Expr *RetE) const {
+  ExplodedNode *N = C.generateNonFatalErrorNode();
   if (!N)
     return;
-
   if (!BT_returnstack)
-    BT_returnstack.reset(
-        new BuiltinBug(this, "Return of address to stack-allocated memory"));
-
+    BT_returnstack = llvm::make_unique<BuiltinBug>(
+        this, "Return of address to stack-allocated memory");
   // Generate a report for this bug.
-  SmallString<512> buf;
+  SmallString<128> buf;
   llvm::raw_svector_ostream os(buf);
   SourceRange range = genName(os, R, C.getASTContext());
   os << " returned to caller";
   auto report = llvm::make_unique<BugReport>(*BT_returnstack, os.str(), N);
   report->addRange(RetE->getSourceRange());
   if (range.isValid())
     report->addRange(range);
-
   C.emitReport(std::move(report));
 }
 
+void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures(
+    const BlockDataRegion &B, CheckerContext &C) const {
+  // There is a not-too-uncommon idiom
+  // where a block passed to dispatch_async captures a semaphore
+  // and then the thread (which called dispatch_async) is blocked on waiting
+  // for the completion of the execution of the block 
+  // via dispatch_semaphore_wait. To avoid false-positives (for now) 
+  // we ignore all the blocks which have captured 
+  // a variable of the type "dispatch_semaphore_t".
+  if (isSemaphoreCaptured(*B.getDecl()))
+    return;
+  for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
+    // The block passed to dispatch_async may capture another block
+    // created on the stack. However, there is no leak in this situaton,
+    // no matter if ARC or no ARC is enabled:
+    // dispatch_async copies the passed "outer" block (via Block_copy)
+    // and if the block has captured another "inner" block,
+    // the "inner" block will be copied as well.
+    if (isa<BlockDataRegion>(Region))
+      continue;
+    ExplodedNode *N = C.generateNonFatalErrorNode();
+    if (!N)
+      continue;
+    if (!BT_capturedstackasync)
+      BT_capturedstackasync = llvm::make_unique<BuiltinBug>(
+          this, "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 an asynchronously-executed block";
+    auto Report =
+        llvm::make_unique<BugReport>(*BT_capturedstackasync, Out.str(), N);
+    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))
+      continue;
+    ExplodedNode *N = C.generateNonFatalErrorNode();
+    if (!N)
+      continue;
+    if (!BT_capturedstackret)
+      BT_capturedstackret = llvm::make_unique<BuiltinBug>(
+          this, "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 =
+        llvm::make_unique<BugReport>(*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 (!Call.isGlobalCFunction("dispatch_after") &&
+      !Call.isGlobalCFunction("dispatch_async"))
+    return;
+  for (unsigned Idx = 0, NumArgs = Call.getNumArgs(); Idx < NumArgs; ++Idx) {
+    if (const BlockDataRegion *B = dyn_cast_or_null<BlockDataRegion>(
+            Call.getArgSVal(Idx).getAsRegion()))
+      checkAsyncExecutedBlockCaptures(*B, C);
+  }
+}
+
 void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
                                           CheckerContext &C) const {
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
     return;
   RetE = RetE->IgnoreParens();
 
   const LocationContext *LCtx = C.getLocationContext();
   SVal V = C.getState()->getSVal(RetE, LCtx);
   const MemRegion *R = V.getAsRegion();
-
   if (!R)
     return;
 
-  const StackSpaceRegion *SS =
-    dyn_cast_or_null<StackSpaceRegion>(R->getMemorySpace());
-
-  if (!SS)
-    return;
-
-  // Return stack memory in an ancestor stack frame is fine.
-  const StackFrameContext *CurFrame = LCtx->getCurrentStackFrame();
-  const StackFrameContext *MemFrame = SS->getStackFrame();
-  if (MemFrame != CurFrame)
-    return;
+  if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
+    checkReturnedBlockCaptures(*B, C);
 
-  // Automatic reference counting automatically copies blocks.
-  if (C.getASTContext().getLangOpts().ObjCAutoRefCount &&
-      isa<BlockDataRegion>(R))
+  if (!isa<StackSpaceRegion>(R->getMemorySpace()) || 
+      isNotInCurrentFrame(R, C) || isArcManagedBlock(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())
     return;
 
   // The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied
   // so the stack address is not escaping here.
   if (auto *ICE = dyn_cast<ImplicitCastExpr>(RetE)) {
     if (isa<BlockDataRegion>(R) &&
         ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
       return;
     }
   }
 
   EmitStackError(C, R, RetE);
 }
 
 void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const {
-  ProgramStateRef state = Ctx.getState();
+  ProgramStateRef State = Ctx.getState();
 
   // Iterate over all bindings to global variables and see if it contains
   // a memory region in the stack space.
   class CallBack : public StoreManager::BindingsHandler {
   private:
     CheckerContext &Ctx;
     const StackFrameContext *CurSFC;
+
   public:
-    SmallVector<std::pair<const MemRegion*, const MemRegion*>, 10> V;
+    SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V;
 
-    CallBack(CheckerContext &CC) :
-      Ctx(CC),
-      CurSFC(CC.getLocationContext()->getCurrentStackFrame())
-    {}
-
-    bool HandleBinding(StoreManager &SMgr, Store store,
-                       const MemRegion *region, SVal val) override {
+    CallBack(CheckerContext &CC)
+        : Ctx(CC), CurSFC(CC.getLocationContext()->getCurrentStackFrame()) {}
 
-      if (!isa<GlobalsSpaceRegion>(region->getMemorySpace()))
-        return true;
-
-      const MemRegion *vR = val.getAsRegion();
-      if (!vR)
-        return true;
+    bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region,
+                       SVal Val) override {
 
-      // Under automated retain release, it is okay to assign a block
-      // directly to a global variable.
-      if (Ctx.getASTContext().getLangOpts().ObjCAutoRefCount &&
-          isa<BlockDataRegion>(vR))
+      if (!isa<GlobalsSpaceRegion>(Region->getMemorySpace()))
         return true;
-
-      if (const StackSpaceRegion *SSR =
-          dyn_cast<StackSpaceRegion>(vR->getMemorySpace())) {
-        // If the global variable holds a location in the current stack frame,
-        // record the binding to emit a warning.
-        if (SSR->getStackFrame() == CurSFC)
-          V.push_back(std::make_pair(region, vR));
-      }
-
+      const MemRegion *VR = Val.getAsRegion();
+      if (VR && isa<StackSpaceRegion>(VR->getMemorySpace()) &&
+          !isArcManagedBlock(VR, Ctx) && !isNotInCurrentFrame(VR, Ctx))
+        V.emplace_back(Region, VR);
       return true;
     }
   };
 
-  CallBack cb(Ctx);
-  state->getStateManager().getStoreManager().iterBindings(state->getStore(),cb);
+  CallBack Cb(Ctx);
+  State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+                                                          Cb);
 
-  if (cb.V.empty())
+  if (Cb.V.empty())
     return;
 
   // Generate an error node.
-  ExplodedNode *N = Ctx.generateNonFatalErrorNode(state);
+  ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
   if (!N)
     return;
 
   if (!BT_stackleak)
-    BT_stackleak.reset(
-        new BuiltinBug(this, "Stack address stored into global variable",
-                       "Stack address was saved into a global variable. "
-                       "This is dangerous because the address will become "
-                       "invalid after returning from the function"));
+    BT_stackleak = llvm::make_unique<BuiltinBug>(
+        this, "Stack address stored into global variable",
+        "invalid after returning from the function");
 
-  for (unsigned i = 0, e = cb.V.size(); i != e; ++i) {
+  for (const auto &P : Cb.V) {
     // Generate a report for this bug.
-    SmallString<512> buf;
-    llvm::raw_svector_ostream os(buf);
-    SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext());
-    os << " is still referred to by the ";
-    if (isa<StaticGlobalSpaceRegion>(cb.V[i].first->getMemorySpace()))
-      os << "static";
+    SmallString<128> Buf;
+    llvm::raw_svector_ostream Out(Buf);
+    SourceRange Range = genName(Out, P.second, Ctx.getASTContext());
+    Out << " is still referred to by the ";
+    if (isa<StaticGlobalSpaceRegion>(P.first->getMemorySpace()))
+      Out << "static";
     else
-      os << "global";
-    os << " variable '";
-    const VarRegion *VR = cast<VarRegion>(cb.V[i].first->getBaseRegion());
-    os << *VR->getDecl()
+      Out << "global";
+    Out << " variable '";
+    const VarRegion *VR = cast<VarRegion>(P.first->getBaseRegion());
+    Out << *VR->getDecl()
         << "' upon returning to the caller.  This will be a dangling reference";
-    auto report = llvm::make_unique<BugReport>(*BT_stackleak, os.str(), N);
-    if (range.isValid())
-      report->addRange(range);
+    auto Report = llvm::make_unique<BugReport>(*BT_stackleak, Out.str(), N);
+    if (Range.isValid())
+      Report->addRange(Range);
 
-    Ctx.emitReport(std::move(report));
+    Ctx.emitReport(std::move(Report));
   }
 }
 
-void ento::registerStackAddrEscapeChecker(CheckerManager &mgr) {
-  mgr.registerChecker<StackAddrEscapeChecker>();
+void ento::registerStackAddrEscapeChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<StackAddrEscapeChecker>();
 }

cfe/trunk/test/Analysis/stack-capture-leak-arc.mm

+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -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);
+
+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;
+
+  dispatch_async(queue, ^{
+    *p = 321;
+  });
+  // expected-warning@-3 {{Address of stack memory associated with local variable 'x' \
+is captured by an asynchronously-executed block}}
+}
+
+void test_block_expr_once_no_leak() {
+  int x = 123;
+  int *p = &x;
+  // synchronous, no warning
+  dispatch_once(predicate, ^{
+    *p = 321;
+  });
+}
+
+void test_block_expr_after() {
+  int x = 123;
+  int *p = &x;
+  dispatch_after(when, queue, ^{
+    *p = 321;
+  });
+  // expected-warning@-3 {{Address of stack memory associated with local variable 'x' \
+is captured by an asynchronously-executed block}}
+}
+
+void test_block_expr_async_no_leak() {
+  int x = 123;
+  int *p = &x;
+  // no leak
+  dispatch_async(queue, ^{
+    int y = x;
+    ++y;
+  });
+}
+
+void test_block_var_async() {
+  int x = 123;
+  int *p = &x;
+  void (^b)(void) = ^void(void) {
+    *p = 1; 
+  };
+  dispatch_async(queue, b);
+  // expected-warning@-1 {{Address of stack memory associated with local variable 'x' \
+is captured by an asynchronously-executed block}}
+}
+
+void test_block_with_ref_async() {
+  int x = 123;
+  int &r = x;
+  void (^b)(void) = ^void(void) {
+    r = 1; 
+  };
+  dispatch_async(queue, b);
+  // expected-warning@-1 {{Address of stack memory associated with local variable 'x' \
+is captured by an asynchronously-executed block}}
+}
+
+dispatch_block_t get_leaking_block() {
+  int leaked_x = 791;
+  int *p = &leaked_x;
+  return ^void(void) {
+    *p = 1; 
+  };
+  // expected-warning@-3 {{Address of stack memory associated with local variable 'leaked_x' \
+is captured by a returned block}}
+}
+
+void test_returned_from_func_block_async() {
+  dispatch_async(queue, get_leaking_block());
+  // expected-warning@-1 {{Address of stack memory associated with local variable 'leaked_x' \
+is captured by an asynchronously-executed block}}
+}
+
+// synchronous, no leak
+void test_block_var_once() {
+  int x = 123;
+  int *p = &x;
+  void (^b)(void) = ^void(void) {
+    *p = 1; 
+  };
+  dispatch_once(predicate, b); // no-warning
+}
+
+void test_block_var_after() {
+  int x = 123;
+  int *p = &x;
+  void (^b)(void) = ^void(void) {
+    *p = 1; 
+  };
+  dispatch_after(when, queue, b);
+  // expected-warning@-1 {{Address of stack memory associated with local variable 'x' \
+is captured by an asynchronously-executed block}}
+}
+
+void test_block_var_async_no_leak() {
+  int x = 123;
+  int *p = &x;
+  void (^b)(void) = ^void(void) {
+    int y = x;
+    ++y; 
+  };
+  dispatch_async(queue, b); // no-warning
+}
+
+void test_block_inside_block_async_no_leak() {
+  int x = 123;
+  int *p = &x;
+  void (^inner)(void) = ^void(void) {
+    int y = x;
+    ++y; 
+  };
+  void (^outer)(void) = ^void(void) {
+    int z = x;
+    ++z;
+    inner(); 
+  };
+  dispatch_async(queue, outer); // no-warning
+}
+
+dispatch_block_t accept_and_pass_back_block(dispatch_block_t block) {
+  block();
+  return block; // no-warning
+}
+
+void test_passing_continuation_no_leak() {
+  int x = 123;
+  int *p = &x;
+  void (^cont)(void) = ^void(void) {
+    *p = 128;
+  };
+  accept_and_pass_back_block(cont); // no-warning
+}
+
+@interface NSObject
+@end
+@protocol OS_dispatch_semaphore
+@end
+typedef NSObject<OS_dispatch_semaphore> *dispatch_semaphore_t;
+dispatch_semaphore_t dispatch_semaphore_create(long value);
+long dispatch_semaphore_wait(dispatch_semaphore_t dsema, dispatch_time_t timeout);
+long dispatch_semaphore_signal(dispatch_semaphore_t dsema);
+
+void test_no_leaks_on_semaphore_pattern() {
+  int x = 0;
+  int *p = &x;
+  dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
+  dispatch_async(queue, ^{
+    *p = 1;
+    // Some work.
+    dispatch_semaphore_signal(semaphore);
+  }); // no-warning
+
+  // Do some other work concurrently with the asynchronous work
+  // Wait for the asynchronous work to finish
+  dispatch_semaphore_wait(semaphore, 1000);
+}

cfe/trunk/test/Analysis/stack-capture-leak-no-arc.mm

+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -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 test_block_inside_block_async_no_leak() {
+  int x = 123;
+  int *p = &x;
+  void (^inner)(void) = ^void(void) {
+    int y = x;
+    ++y; 
+  };
+  // Block_copy(...) copies the captured block ("inner") too,
+  // there is no leak in this case.
+  dispatch_async(queue, ^void(void) {
+    int z = x;
+    ++z;
+    inner(); 
+  }); // no-warning
+}
+
+dispatch_block_t test_block_inside_block_async_leak() {
+  int x = 123;
+  void (^inner)(void) = ^void(void) {
+    int y = x;
+    ++y; 
+  };
+  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}}
+}
+