[analyzer] Path-insensitive checker for writes into an auto-releasing pointer from the wrong pool

[george.karpenkov]

Summary

rdar://25301111

[dcoughlin]

Looks good with updated diagnostic text and a test for non-ARC behavior.

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp

11 ↗

(On Diff #139696)

I would say "warns about writes to autoreleasing out parameters that are likely to cause crashes".

16 ↗

(On Diff #139696)

I would use the -enumerateObjectsUsingBlock: as the canonical example. The queue/threading aspect is incidental to the problem here.

102 ↗

(On Diff #139696)

The queue aspect is incidental here. I suggest:

"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"

and

"write to autoreleasing out parameter inside autorelease pool that may exit before method returns; consider writing first to a strong local variable declared outside of the block"

for the method case.

test/Analysis/autoreleasewritechecker_test.m

1 ↗

(On Diff #139696)

Can you add a test that makes sure we don't warn on a write to an out parameter when ARC is disabled.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 def ObjCDeallocChecker : Checker<"Dealloc">,
   HelpText<"Warn about Objective-C classes that lack a correct implementation of -dealloc">,
   DescFile<"CheckObjCDealloc.cpp">;
 
 def ObjCSuperDeallocChecker : Checker<"SuperDealloc">,
   HelpText<"Warn about improper use of '[super dealloc]' in Objective-C">,
   DescFile<"ObjCSuperDeallocChecker.cpp">;
 
+def AutoreleaseWriteChecker : Checker<"AutoreleaseWrite">,
+  HelpText<"Warn about potentially crashing writes to autoreleasing objects from different autoreleasing pools in Objective-C">,
+  DescFile<"ObjCAutoreleaseWriteChecker.cpp">;
 } // end "osx.cocoa"
 
 let ParentPackage = Performance in {
 
 def GCDAntipattern : Checker<"GCDAntipattern">,
   HelpText<"Check for performance anti-patterns when using Grand Central Dispatch">,
   DescFile<"GCDAntipatternChecker.cpp">;
 } // end "optin.performance"

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NonnullGlobalConstantsChecker.cpp
   NullabilityChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
+  ObjCAutoreleaseWriteChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   PaddingChecker.cpp

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.
+// 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.
+//
+//===----------------------------------------------------------------------===//
+
+#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 *ParamBind = "parambind";
+const char *MethodBind = "methodbind";
+
+class ObjCAutoreleaseWriteChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D,
+                        AnalysisManager &AM,
+                        BugReporter &BR) const;
+private:
+  std::vector<std::string> SelectorsWithAutoreleasingPool = {
+      "enumerateObjectsUsingBlock:",
+      "enumerateKeysAndObjectsUsingBlock:",
+      "enumerateKeysAndObjectsWithOptions:usingBlock:",
+      "enumerateObjectsWithOptions:usingBlock:",
+      "enumerateObjectsAtIndexes:options:usingBlock:",
+      "enumerateIndexesWithOptions:usingBlock:",
+      "enumerateIndexesUsingBlock:",
+      "enumerateIndexesInRange:options:usingBlock:",
+      "enumerateRangesUsingBlock:",
+      "enumerateRangesWithOptions:usingBlock:",
+      "enumerateRangesInRange:options:usingBlock:"
+      "objectWithOptions:passingTest:",
+  };
+
+  std::vector<std::string> FunctionsWithAutoreleasingPool = {
+      "dispatch_async", "dispatch_group_async", "dispatch_barrier_async"};
+};
+}
+
+static inline std::vector<llvm::StringRef> toRefs(std::vector<std::string> V) {
+  return std::vector<llvm::StringRef>(V.begin(), V.end());
+}
+
+static auto callsNames(std::vector<std::string> FunctionNames)
+    -> decltype(callee(functionDecl())) {
+  return callee(functionDecl(hasAnyName(toRefs(FunctionNames))));
+}
+
+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 *Name = IsMethod ? "method" : "function";
+  assert(SW);
+  BR.EmitBasicReport(
+      ADC->getDecl(), Checker,
+      /*Name=*/"Writing into auto-releasing variable from a different queue",
+      /*Category=*/"Memory",
+      (llvm::Twine("Writing into an auto-releasing 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());
+}
+
+void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D,
+                                                  AnalysisManager &AM,
+                                                  BugReporter &BR) const {
+
+  // Write into a binded object, e.g. *ParamBind = X.
+  auto WritesIntoM = binaryOperator(
+    hasLHS(unaryOperator(
+        hasOperatorName("*"),
+        hasUnaryOperand(
+          ignoringParenImpCasts(
+            declRefExpr(to(parmVarDecl(equalsBoundNode(ParamBind))))))
+    )),
+    hasOperatorName("=")
+  ).bind(ProblematicWriteBind);
+
+  // WritesIntoM happens inside a block passed as an argument.
+  auto WritesInBlockM = hasAnyArgument(allOf(
+      hasType(hasCanonicalType(blockPointerType())),
+      forEachDescendant(WritesIntoM)
+      ));
+
+  auto CallsAsyncM = stmt(anyOf(
+    callExpr(allOf(
+      callsNames(FunctionsWithAutoreleasingPool), WritesInBlockM)),
+    objcMessageExpr(allOf(
+       hasAnySelector(toRefs(SelectorsWithAutoreleasingPool)),
+       WritesInBlockM))
+  ));
+
+  auto DoublePointerParamM =
+      parmVarDecl(hasType(pointerType(
+                      pointee(hasCanonicalType(objcObjectPointerType())))))
+          .bind(ParamBind);
+
+  auto HasParamAndWritesAsyncM = allOf(
+      hasAnyParameter(DoublePointerParamM),
+      forEachDescendant(CallsAsyncM));
+
+  auto MatcherM = decl(anyOf(
+      objcMethodDecl(HasParamAndWritesAsyncM).bind(MethodBind),
+      functionDecl(HasParamAndWritesAsyncM)));
+
+  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

+// RUN: %clang_analyze_cc1 -DARC -fobjc-arc -analyzer-checker=core,osx.cocoa.AutoreleaseWrite %s -fblocks -verify
+
+
+typedef signed char BOOL;
+@protocol NSObject  - (BOOL)isEqual:(id)object; @end
+@interface NSObject <NSObject> {}
++(id)alloc;
+-(id)init;
+-(id)autorelease;
+-(id)copy;
+-(id)retain;
+@end
+typedef int NSZone;
+typedef int NSCoder;
+@protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
+@protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
+@interface NSError : NSObject <NSCopying, NSCoding> {}
++ (id)errorWithDomain:(int)domain;
+@end
+
+typedef int dispatch_semaphore_t;
+typedef void (^block_t)();
+
+@interface NSArray
+- (void) enumerateObjectsUsingBlock:(block_t)block;
+@end
+
+typedef int group_t;
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef void (^dispatch_block_t)(void);
+extern dispatch_queue_t queue;
+
+void dispatch_group_async(dispatch_queue_t queue,
+                          group_t group,
+                          dispatch_block_t block);
+void dispatch_async(dispatch_queue_t queue, dispatch_block_t block);
+dispatch_semaphore_t dispatch_semaphore_create(int);
+
+void dispatch_semaphore_wait(dispatch_semaphore_t, int);
+void dispatch_semaphore_signal(dispatch_semaphore_t);
+
+// No warnings without ARC.
+#ifdef NOARC
+
+// expected-no-diagnostics
+BOOL writeToErrorWithIterator(NSError ** error, NSArray *a) {
+  [a enumerateObjectsUsingBlock:^{
+    *error = [NSError errorWithDomain:1]; // no-warning
+    }];
+  return 0;
+}
+#endif
+
+#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;
+@end
+
+@implementation I
+
+- (BOOL) writeToErrorInBlock:(NSError *__autoreleasing *)error {
+    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+    dispatch_async(queue, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing out parameter inside autorelease pool that may exit before method returns}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+    return 0;
+}
+
+- (BOOL) writeToErrorWithDispatchGroup:(NSError *__autoreleasing *)error {
+    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+    dispatch_group_async(queue, 0, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing out}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+    return 0;
+}
+
+- (BOOL) writeToLocalErrorInBlock:(NSError *__autoreleasing *)error {
+    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+    dispatch_async(queue, ^{
+        NSError* error2;
+        NSError*__strong* error3 = &error2;
+        if (error) {
+            *error3 = [NSError errorWithDomain:1]; // no-warning
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+    return 0;
+}
+
+- (BOOL) writeToStrongErrorInBlock:(NSError *__strong *)error {
+    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+    dispatch_async(queue, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:2]; // no-warning
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+    return 0;
+}
+
+- (BOOL) writeToErrorInBlockMultipleTimes:(NSError *__autoreleasing *)error {
+    dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+    dispatch_async(queue, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing out}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+    dispatch_async(queue, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing out}}
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing out}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+    *error = [NSError errorWithDomain:1]; // no-warning
+
+    dispatch_semaphore_wait(sem, 100);
+    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{{Writing into an auto-releasing out}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+  return 0;
+}
+
+BOOL writeToErrorNoWarning(NSError *__autoreleasing* error) {
+  *error = [NSError errorWithDomain:1]; // no-warning
+  return 0;
+}
+
+BOOL writeToErrorWithIterator(NSError *__autoreleasing* error, NSArray *a) {
+  [a enumerateObjectsUsingBlock:^{
+    *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing 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 0;
+}
+#endif