[analyzer] Extend the ObjCAutoreleaseWriteChecker to warn on captures as well

[george.karpenkov]

Summary

A common pattern is that the code in the block does not write into the variable explicitly, but instead passes it to a helper function which performs the write.

[NoQ]

Sounds pretty aggressive. Are we sure that it's never a good idea to pass the pointer around? I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

The code looks reasonable.

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

118–132

/dance

*
 *

150–156

Hmm, do we need an equalsBoundNode here? Why don't we simply inline the matcher?

169

Would this be better with ignoringParenImpCasts?

Or we could as well stop on parentheses as a weird suppression mechanism for the users.

[george.karpenkov]

Sounds pretty aggressive

Actually, it's much less aggressive than the Clang warning, firing on *every* capture of autoreleasing variable in a block [unless you explicitly add an __autoreleasing lifetime specifier suppressing it]

Are we sure that it's never a good idea to pass the pointer around?

It's an almost guaranteed use-after-free if an autoreleasing pointer is written into inside the autoreleasing block (almost because maybe the pointer is never read from again).
It's OK to read it -- but then the user should pass *pointer, as that is sufficient. Passing just pointer almost guarantees a writing intent.

I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

It is captured in a block: the warning fires when we use a wrapper to write to the autoreleasing variable *already inside the block running in the autoreleasing pool*.

[NoQ]

In D46772#1098469, @george.karpenkov wrote:

I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

It is captured in a block: the warning fires when we use a wrapper to write to the autoreleasing variable *already inside the block running in the autoreleasing pool*.

Aha, ok, yup, sounds reasonable!

[george.karpenkov]

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

150–156

Avoids mega-expressions, and we need the binding anyway.

[NoQ]

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

150–156

I mean, declRefExpr(to(DoublePointerParamM)) sounds pretty short.

And even if we need the binding, having equalsBoundNode() specifically is making things weirder because i'm starting to suspect that we're matching a pattern with backreferences while reading the code, even though in fact we don't.

[NoQ]

Thx!

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

 //===- ObjCAutoreleaseWriteChecker.cpp ----------------------------*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines ObjCAutoreleaseWriteChecker which warns against writes
-// into autoreleased out parameters which are likely to cause crashes.
+// into autoreleased out parameters which cause crashes.
 // An example of a problematic write is a write to {@code error} in the example
 // below:
 //
 // - (BOOL) mymethod:(NSError *__autoreleasing *)error list:(NSArray*) list {
 //     [list enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) {
 //       NSString *myString = obj;
 //       if ([myString isEqualToString:@"error"] && error)
 //         *error = [NSError errorWithDomain:@"MyDomain" code:-1];
 //     }];
 //     return false;
 // }
 //
-// Such code is very likely to crash due to the other queue autorelease pool
-// begin able to free the error object.
+// Such code will crash on read from `*error` due to the autorelease pool
+// in `enumerateObjectsUsingBlock` implementation freeing the error object
+// on exit from the function.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.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 *MethodBind = "methodbind";
+const char *IsMethodBind = "ismethodbind";
 
 class ObjCAutoreleaseWriteChecker : public Checker<check::ASTCodeBody> {
 public:
   void checkASTCodeBody(const Decl *D,
                         AnalysisManager &AM,
                         BugReporter &BR) const;
 private:
   std::vector<std::string> SelectorsWithAutoreleasingPool = {

 }
 
 static void emitDiagnostics(BoundNodes &Match, const Decl *D, BugReporter &BR,
                             AnalysisManager &AM,
                             const ObjCAutoreleaseWriteChecker *Checker) {
   AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D);
 
   const auto *PVD = Match.getNodeAs<ParmVarDecl>(ParamBind);
-  assert(PVD);
   QualType Ty = PVD->getType();
   if (Ty->getPointeeType().getObjCLifetime() != Qualifiers::OCL_Autoreleasing)
     return;
-  const auto *SW = Match.getNodeAs<Expr>(ProblematicWriteBind);
-  bool IsMethod = Match.getNodeAs<ObjCMethodDecl>(MethodBind) != nullptr;
+  const char *WarningMsg = "Write to";
+  const auto *MarkedStmt = Match.getNodeAs<Expr>(ProblematicWriteBind);
+
+  // Prefer to warn on write, but if not available, warn on capture.
+  if (!MarkedStmt) {
+    MarkedStmt = Match.getNodeAs<Expr>(CapturedBind);
+    assert(MarkedStmt);
+    WarningMsg = "Capture of";
+  }
+
+  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";
-  assert(SW);
   BR.EmitBasicReport(
       ADC->getDecl(), Checker,
-      /*Name=*/"Write to autoreleasing out parameter inside autorelease pool",
+      /*Name=*/(llvm::Twine(WarningMsg)
+                + " autoreleasing out parameter inside autorelease pool").str(),
       /*Category=*/"Memory",
-      (llvm::Twine("Write to autoreleasing out parameter inside ") +
+      (llvm::Twine(WarningMsg) + " autoreleasing out parameter inside " +
        "autorelease pool that may exit before " + Name + " returns; consider "
        "writing first to a strong local variable declared outside of the block")
           .str(),
-      PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC),
-      SW->getSourceRange());
+      Location,
+      Range);
 }
 
 void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D,
                                                   AnalysisManager &AM,
                                                   BugReporter &BR) const {
 
+  auto DoublePointerParamM =
+      parmVarDecl(hasType(hasCanonicalType(pointerType(
+                      pointee(hasCanonicalType(objcObjectPointerType()))))))
+          .bind(ParamBind);
+
+  auto ReferencedParamM =
+      declRefExpr(to(parmVarDecl(equalsBoundNode(ParamBind))));
+
   // Write into a binded object, e.g. *ParamBind = X.
   auto WritesIntoM = binaryOperator(
     hasLHS(unaryOperator(
         hasOperatorName("*"),
         hasUnaryOperand(
-          ignoringParenImpCasts(
-            declRefExpr(to(parmVarDecl(equalsBoundNode(ParamBind))))))
+          ignoringParenImpCasts(ReferencedParamM))
     )),
     hasOperatorName("=")
   ).bind(ProblematicWriteBind);
 
+  auto ArgumentCaptureM = hasAnyArgument(
+    ignoringParenImpCasts(ReferencedParamM));
+  auto CapturedInParamM = stmt(anyOf(
+      callExpr(ArgumentCaptureM),
+      objcMessageExpr(ArgumentCaptureM))).bind(CapturedBind);
+
   // WritesIntoM happens inside a block passed as an argument.
-  auto WritesInBlockM = hasAnyArgument(allOf(
+  auto WritesOrCapturesInBlockM = hasAnyArgument(allOf(
       hasType(hasCanonicalType(blockPointerType())),
-      forEachDescendant(WritesIntoM)
-      ));
+      forEachDescendant(
+        stmt(anyOf(WritesIntoM, CapturedInParamM))
+      )));
 
-  auto CallsAsyncM = stmt(anyOf(
+  auto BlockPassedToMarkedFuncM = stmt(anyOf(
     callExpr(allOf(
-      callsNames(FunctionsWithAutoreleasingPool), WritesInBlockM)),
+      callsNames(FunctionsWithAutoreleasingPool), WritesOrCapturesInBlockM)),
     objcMessageExpr(allOf(
        hasAnySelector(toRefs(SelectorsWithAutoreleasingPool)),
-       WritesInBlockM))
+       WritesOrCapturesInBlockM))
   ));
 
-  auto DoublePointerParamM =
-      parmVarDecl(hasType(hasCanonicalType(pointerType(
-                      pointee(hasCanonicalType(objcObjectPointerType()))))))
-          .bind(ParamBind);
-
-  auto HasParamAndWritesAsyncM = allOf(
+  auto HasParamAndWritesInMarkedFuncM = allOf(
       hasAnyParameter(DoublePointerParamM),
-      forEachDescendant(CallsAsyncM));
+      forEachDescendant(BlockPassedToMarkedFuncM));
 
   auto MatcherM = decl(anyOf(
-      objcMethodDecl(HasParamAndWritesAsyncM).bind(MethodBind),
-      functionDecl(HasParamAndWritesAsyncM)));
+      objcMethodDecl(HasParamAndWritesInMarkedFuncM).bind(IsMethodBind),
+      functionDecl(HasParamAndWritesInMarkedFuncM)));
 
   auto Matches = match(MatcherM, *D, AM.getASTContext());
   for (BoundNodes Match : Matches)
     emitDiagnostics(Match, D, BR, AM, this);
 }
 
 void ento::registerAutoreleaseWriteChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<ObjCAutoreleaseWriteChecker>();
 }

test/Analysis/autoreleasewritechecker_test.m

[i indexesPassingTest:^{
     }];
   [i indexWithOptions: NSEnumerationReverse passingTest:^(NSUInteger idx, BOOL *stop) {
     *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}}
     return YES;
     }];
   return 0;
 }
 
+void writeIntoError(NSError **error) {
+  *error = [NSError errorWithDomain:1];
+}
+
+extern void readError(NSError *error);
+
 void writeToErrorWithIteratorNonnull(NSError *__autoreleasing* _Nonnull error, NSDictionary *a) {
   [a enumerateKeysAndObjectsUsingBlock:^{
      *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter}}
   }];
 }
+
+
+void escapeErrorFromIterator(NSError *__autoreleasing* _Nonnull error, NSDictionary *a) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+     writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}}
+  }];
+}
+
+void noWarningOnRead(NSError *__autoreleasing* error, NSDictionary *a) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+     NSError* local = *error; // no-warning
+  }];
+}
+
+void noWarningOnEscapeRead(NSError *__autoreleasing* error, NSDictionary *a) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+     readError(*error); // no-warning
+  }];
+}
+
+@interface ErrorCapture
+- (void) captureErrorOut:(NSError**) error;
+- (void) captureError:(NSError*) error;
+@end
+
+void escapeErrorFromIteratorMethod(NSError *__autoreleasing* _Nonnull error,
+                                   NSDictionary *a,
+                                   ErrorCapture *capturer) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+      [capturer captureErrorOut:error]; // expected-warning{{Capture of autoreleasing out parameter}}
+  }];
+}
+
+void noWarningOnEscapeReadMethod(NSError *__autoreleasing* error,
+                                 NSDictionary *a,
+                                 ErrorCapture *capturer) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+    [capturer captureError:*error]; // no-warning
+  }];
+}
+
+void multipleErrors(NSError *__autoreleasing* error, NSDictionary *a) {
+  [a enumerateKeysAndObjectsUsingBlock:^{
+     writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}}
+     *error = [NSError errorWithDomain:1]; // expected-warning{{Write to autoreleasing out parameter}}
+     writeIntoError(error); // expected-warning{{Capture of autoreleasing out parameter}}
+  }];
+}
+
 #endif