[analyzer] ObjCAutoreleaseWriteChecker: Support explicit autoreleasepools.

[NoQ]

Summary

This ASTMatchers-based checker currently supports only a whitelist of block-enumeration methods which are known to internally clear an autorelease pool. Extend this checker to detect writes within the scope of explicit @autoreleasepool statements.

Patch by Paul Pelzl!
(who can't participate in the discussion for IRL reasons; i'll address comments)

[vsavchenko]

Otherwise, LTGM

clang/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

134–165

This looks a bit messy to me, but not critical.

[NoQ]

Break up the huge twine into an old-school stream.

Lowkey change bug category to a standard category. Technically this is indeed a retain count error so the category is appropriate and it shouldn't matter to the user that it is coming from a different checker. But i'm still a bit worried because RetainCountChecker is so iconic to its users that they may be surprised if a new syntactic checker suddenly shows up under this category. Maybe we should make a new category, something like "Memory error (ARC)".

[vsavchenko]

Awesome, thanks!

[ddkilzer]

Thanks for implementing this! For posterity, I wanted to note a couple cases that this checker doesn't catch.

1. Under ARC, a more general case of assigning to an __autoreleasing variable. (Not sure why anyone would do this, but it's possible to write.)

@implementation MyClass
- (BOOL)myError:(NSError * __strong *)error
{
    NSError __autoreleasing *localError;

    @autoreleasepool {
        localError = [[NSError alloc] init];
    }

    if (error) {
        *error = localError;
        return YES;
    }
    return NO;
}
@end

2. Under MRR, writing to an autoreleasing out parameter that outlives the autoreleasePool (similar to the issue that is now found under ARC):

@implementation MyClass
- (BOOL)myError:(NSError **)error
{
    @autoreleasepool {
        if (error) {
            *error = [[[NSError alloc] init] autorelease];
            return YES;
        }
    }
    return NO;
}
@end

clang/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

 // on exit from the function.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "llvm/ADT/Twine.h"
 
 using namespace clang;
 using namespace ento;
 using namespace ast_matchers;
 
 namespace {
 
 const char *ProblematicWriteBind = "problematicwrite";
 const char *CapturedBind = "capturedbind";
 const char *ParamBind = "parambind";
 const char *IsMethodBind = "ismethodbind";
+const char *IsARPBind = "isautoreleasepoolbind";
 
 class ObjCAutoreleaseWriteChecker : public Checker<check::ASTCodeBody> {
 public:
   void checkASTCodeBody(const Decl *D,
                         AnalysisManager &AM,
                         BugReporter &BR) const;
 private:
   std::vector<std::string> SelectorsWithAutoreleasingPool = {
if (!MarkedStmt) {
     assert(MarkedStmt);
     ActionMsg = "Capture of";
     IsCapture = true;
   }
 
   SourceRange Range = MarkedStmt->getSourceRange();
   PathDiagnosticLocation Location = PathDiagnosticLocation::createBegin(
       MarkedStmt, BR.getSourceManager(), ADC);
+
   bool IsMethod = Match.getNodeAs<ObjCMethodDecl>(IsMethodBind) != nullptr;
-  const char *Name = IsMethod ? "method" : "function";
+  const char *FunctionDescription = IsMethod ? "method" : "function";
+  bool IsARP = Match.getNodeAs<ObjCAutoreleasePoolStmt>(IsARPBind) != nullptr;
+
+  llvm::SmallString<128> BugNameBuf;
+  llvm::raw_svector_ostream BugName(BugNameBuf);
+  BugName << ActionMsg
+          << " autoreleasing out parameter inside autorelease pool";
+
+  llvm::SmallString<128> BugMessageBuf;
+  llvm::raw_svector_ostream BugMessage(BugMessageBuf);
+  BugMessage << ActionMsg << " autoreleasing out parameter ";
+  if (IsCapture)
+    BugMessage << "'" + PVD->getName() + "' ";
+
+  BugMessage << "inside ";
+  if (IsARP)
+    BugMessage << "locally-scoped autorelease pool;";
+  else
+    BugMessage << "autorelease pool that may exit before "
+               << FunctionDescription << " returns;";
+
+  BugMessage << " consider writing first to a strong local variable"
+                " declared outside ";
+  if (IsARP)
+    BugMessage << "of the autorelease pool";
+  else
+    BugMessage << "of the block";
 
-  BR.EmitBasicReport(
-      ADC->getDecl(), Checker,
-      /*Name=*/(llvm::Twine(ActionMsg)
-                + " autoreleasing out parameter inside autorelease pool").str(),
-      /*BugCategory=*/"Memory",
-      (llvm::Twine(ActionMsg) + " autoreleasing out parameter " +
-       (IsCapture ? "'" + PVD->getName() + "'" + " " : "") + "inside " +
-       "autorelease pool that may exit before " + Name + " returns; consider "
-       "writing first to a strong local variable declared outside of the block")
-          .str(),
-      Location,
+  BR.EmitBasicReport(ADC->getDecl(), Checker, BugName.str(),
+                     categories::MemoryRefCount, BugMessage.str(), Location,
                      Range);
 }
 
 void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D,
                                                   AnalysisManager &AM,
                                                   BugReporter &BR) const {
 
   auto DoublePointerParamM =
       parmVarDecl(hasType(hasCanonicalType(pointerType(
void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D,
   auto BlockPassedToMarkedFuncM = stmt(anyOf(
     callExpr(allOf(
       callsNames(FunctionsWithAutoreleasingPool), WritesOrCapturesInBlockM)),
     objcMessageExpr(allOf(
        hasAnySelector(toRefs(SelectorsWithAutoreleasingPool)),
        WritesOrCapturesInBlockM))
   ));
 
-  auto HasParamAndWritesInMarkedFuncM = allOf(
-      hasAnyParameter(DoublePointerParamM),
-      forEachDescendant(BlockPassedToMarkedFuncM));
+  // WritesIntoM happens inside an explicit @autoreleasepool.
+  auto WritesOrCapturesInPoolM =
+      autoreleasePoolStmt(
+          forEachDescendant(stmt(anyOf(WritesIntoM, CapturedInParamM))))
+          .bind(IsARPBind);
+
+  auto HasParamAndWritesInMarkedFuncM =
+      allOf(hasAnyParameter(DoublePointerParamM),
+            anyOf(forEachDescendant(BlockPassedToMarkedFuncM),
+                  forEachDescendant(WritesOrCapturesInPoolM)));
 
   auto MatcherM = decl(anyOf(
       objcMethodDecl(HasParamAndWritesInMarkedFuncM).bind(IsMethodBind),
       functionDecl(HasParamAndWritesInMarkedFuncM),
       blockDecl(HasParamAndWritesInMarkedFuncM)));
 
   auto Matches = match(MatcherM, *D, AM.getASTContext());
   for (BoundNodes Match : Matches)

clang/test/Analysis/autoreleasewritechecker_test.m

 #ifdef ARC
 @interface I : NSObject
 - (BOOL) writeToStrongErrorInBlock:(NSError *__strong *)error;
 - (BOOL) writeToErrorInBlock:(NSError *__autoreleasing *)error;
 - (BOOL) writeToLocalErrorInBlock:(NSError **)error;
 - (BOOL) writeToErrorInBlockMultipleTimes:(NSError *__autoreleasing *)error;
 - (BOOL) writeToError:(NSError *__autoreleasing *)error;
 - (BOOL) writeToErrorWithDispatchGroup:(NSError *__autoreleasing *)error;
+- (BOOL)writeToErrorInAutoreleasePool:(NSError *__autoreleasing *)error;
+- (BOOL)writeToStrongErrorInAutoreleasePool:(NSError *__strong *)error;
+- (BOOL)writeToLocalErrorInAutoreleasePool:(NSError *__autoreleasing *)error;
+- (BOOL)writeToErrorInAutoreleasePoolMultipleTimes:(NSError *__autoreleasing *)error;
 @end
 
 @implementation I
 
 - (BOOL) writeToErrorInBlock:(NSError *__autoreleasing *)error {
     dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
     dispatch_async(queue, ^{
         if (error) {
dispatch_async(queue, ^{
         dispatch_semaphore_signal(sem);
     });
     *error = [NSError errorWithDomain:1]; // no-warning
 
     dispatch_semaphore_wait(sem, 100);
     return 0;
 }
 
+- (BOOL)writeToErrorInAutoreleasePool:(NSError *__autoreleasing *)error {
+  @autoreleasepool {
+    if (error) {
+      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside locally-scoped autorelease pool; consider writing first to a strong local variable declared outside of the autorelease pool}}
+    }
+  }
+
+  return 0;
+}
+
+- (BOOL)writeToStrongErrorInAutoreleasePool:(NSError *__strong *)error {
+  @autoreleasepool {
+    if (error) {
+      *error = [NSError errorWithDomain:1]; // no-warning
+    }
+  }
+
+  return 0;
+}
+
+- (BOOL)writeToLocalErrorInAutoreleasePool:(NSError *__autoreleasing *)error {
+  NSError *localError;
+  @autoreleasepool {
+    localError = [NSError errorWithDomain:1]; // no-warning
+  }
+
+  if (error) {
+    *error = localError; // no-warning
+  }
+
+  return 0;
+}
+
+- (BOOL)writeToErrorInAutoreleasePoolMultipleTimes:(NSError *__autoreleasing *)error {
+  @autoreleasepool {
+    if (error) {
+      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside locally-scoped autorelease pool; consider writing first to a strong local variable declared outside of the autorelease pool}}
+    }
+  }
+  if (error) {
+    *error = [NSError errorWithDomain:1]; // no-warning
+  }
+  @autoreleasepool {
+    if (error) {
+      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside locally-scoped autorelease pool; consider writing first to a strong local variable declared outside of the autorelease pool}}
+      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside locally-scoped autorelease pool; consider writing first to a strong local variable declared outside of the autorelease pool}}
+    }
+  }
+
+  return 0;
+}
+
 - (BOOL) writeToError:(NSError *__autoreleasing *)error {
     *error = [NSError errorWithDomain:1]; // no-warning
     return 0;
 }
 @end
 
 BOOL writeToErrorInBlockFromCFunc(NSError *__autoreleasing* error) {
     dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
     dispatch_async(queue, ^{
         if (error) {
             *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside autorelease pool that may exit before function returns; consider writing first to a strong local variable declared outside of the block}}
         }
         dispatch_semaphore_signal(sem);
     });
 
     dispatch_semaphore_wait(sem, 100);
   return 0;
 }
 
+BOOL writeIntoErrorInAutoreleasePoolFromCFunc(NSError *__autoreleasing *error) {
+  @autoreleasepool {
+    if (error) {
+      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside locally-scoped autorelease pool; consider writing first to a strong local variable declared outside of the autorelease pool}}
+    }
+  }
+  return 0;
+}
+
 BOOL writeToErrorNoWarning(NSError *__autoreleasing* error) {
   *error = [NSError errorWithDomain:1]; // no-warning
   return 0;
 }
 
 BOOL writeToErrorWithIterator(NSError *__autoreleasing* error, NSArray *a, NSSet *s, NSDictionary *d, NSIndexSet *i) { [a enumerateObjectsUsingBlock:^{
     *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter inside autorelease pool that may exit before function returns; consider writing first to a strong local variable declared outside of the block}}
     }];