[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–119 ↗

(On Diff #146403)

/dance

*
 *

150–156 ↗

(On Diff #146403)

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

169 ↗

(On Diff #146403)

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 ↗

(On Diff #146403)

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

[NoQ]

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

150–156 ↗

(On Diff #146403)

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!

cfe/trunk/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(DoublePointerParamM)));
+
   // 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>();
 }

cfe/trunk/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