[PATCH] D39438: [analyzer] Diagnose stack leaks via block captures

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a reviewer: george.karpenkov.
dcoughlin added a comment.

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.



================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:124
 
+void StackAddrEscapeChecker::checkBlockCaptures(const BlockDataRegion &B,
+                                                CheckerContext &C) const {
----------------
Could we break with tradition and add a oxygen comment describing what this method does?


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:133
+      continue;
+    if (C.getASTContext().getLangOpts().ObjCAutoRefCount &&
+        isa<BlockDataRegion>(Region))
----------------
Can you factor this logic out with the logic it duplicates in checkPreStmt()?


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:136
+      continue;
+    if (dyn_cast<StackSpaceRegion>(Region->getMemorySpace())) {
+      ExplodedNode *N = C.generateNonFatalErrorNode();
----------------
Stylistically we use isa<> for cases like these where the result of dyn_cast<> is not used.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:146
+      SourceRange Range = genName(Out, Region, C.getASTContext());
+      Out << " might leak via the block capture";
+      auto Report =
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:185
+  if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
+    checkBlockCaptures(*B, C); 
+
----------------
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().


================
Comment at: test/Analysis/stack-async-leak.m:1
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -fobjc-arc -verify %s
+
----------------
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D39438