[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/autoreleaseracechecker_test.m

1

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

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 AutoreleaseRaceChecker : Checker<"AutoreleaseRace">,
+  HelpText<"Warn about potential races from autoreleasing objects in Objective-C">,
+  DescFile<"ObjCAutoreleaseRaceChecker.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"

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
+  ObjCAutoreleaseRaceChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   PaddingChecker.cpp

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseRaceChecker.cpp

+//===- ObjCAutoreleaseRaceChecker.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 ObjCAutoreleaseRaceChecker which warns against writes
+// into autoreleased objects from different queues which are likely to race
+// and cause crashes.
+// An example of a problematic write is a write to {@code error} in the example
+// below:
+//
+// - (BOOL) doSomethingWithError:(NSError *__autoreleasing *)error {
+//     dispatch_semaphore_t sem = dispatch_semaphore_create(0l);
+//     dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0l), ^{
+//         if (error) {
+//             *error = [NSError errorWithDomain:@"MyDomain" code:-1];
+//         }
+//         dispatch_semaphore_signal(sem);
+//     });
+//
+//     dispatch_semaphore_wait(sem, DISPATCH_TIME_FOREVER);
+//     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 <vector>
+#include <string>
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+
+const char *ProblematicWriteBind = "problematicwrite";
+
+class ObjCAutoreleaseRaceChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D,
+                        AnalysisManager &AM,
+                        BugReporter &BR) const;
+};
+
+}
+
+static auto callsName(const char *FunctionName)
+    -> decltype(callee(functionDecl())) {
+  return callee(functionDecl(hasName(FunctionName)));
+}
+
+static void emitDiagnostics(BoundNodes &Match,
+                            const Decl *D,
+                            BugReporter &BR,
+                            AnalysisManager &AM,
+                            const ObjCAutoreleaseRaceChecker *Checker) {
+  AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D);
+  const auto *SW = Match.getNodeAs<Expr>(ProblematicWriteBind);
+  assert(SW);
+  BR.EmitBasicReport(
+    ADC->getDecl(),
+    Checker,
+    /*Name=*/"Writing into auto-releasing variable from a different queue",
+    /*Category=*/"Memory",
+    "Writing into an auto-releasing variable from a different queue "
+    "to which it belongs to is very likely to race and crash",
+    PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC),
+    SW->getSourceRange()
+  );
+}
+
+void ObjCAutoreleaseRaceChecker::checkASTCodeBody(const Decl *D,
+                                                  AnalysisManager &AM,
+                                                  BugReporter &BR) const {
+  const char* ParamBind = "autoreleasingparam";
+
+  // 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)
+      ));
+
+  // FIXME: use a list of APIs which can dispatch in asynchronous manner.
+  auto CallsAsyncM = callExpr(allOf(callsName("dispatch_async"),
+                                    WritesInBlockM));
+
+  auto DoublePointerParamM =
+      parmVarDecl(hasType(pointerType(
+                      pointee(ignoringParens(objcObjectPointerType())))))
+          .bind(ParamBind);
+
+  auto HasParamAndWritesAsyncM = allOf(
+      hasAnyParameter(DoublePointerParamM),
+      forEachDescendant(CallsAsyncM));
+
+  auto MatcherM = decl(anyOf(
+      objcMethodDecl(HasParamAndWritesAsyncM),
+      functionDecl(HasParamAndWritesAsyncM)));
+
+  auto Matches = match(MatcherM, *D, AM.getASTContext());
+  for (BoundNodes Match : Matches)
+    emitDiagnostics(Match, D, BR, AM, this);
+}
+
+void ento::registerAutoreleaseRaceChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ObjCAutoreleaseRaceChecker>();
+}

test/Analysis/autoreleaseracechecker_test.m

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.AutoreleaseRace %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)();
+typedef NSError** MYERROR;
+
+typedef struct dispatch_queue_s *dispatch_queue_t;
+typedef void (^dispatch_block_t)(void);
+extern dispatch_queue_t queue;
+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);
+
+@interface I : NSObject
+- (BOOL) writeToErrorInBlock:(NSError *__autoreleasing *)error;
+- (BOOL) writeToErrorInBlockMultipleTimes:(NSError *__autoreleasing *)error;
+- (BOOL) writeToErrorInBlockNoAnnotation:(NSError **)error;
+- (BOOL) writeToError:(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 variable}}
+        }
+        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 variable}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+    dispatch_async(queue, ^{
+        if (error) {
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing variable}}
+            *error = [NSError errorWithDomain:1]; // expected-warning{{Writing into an auto-releasing variable}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+    *error = [NSError errorWithDomain:1]; // no-warning
+
+    dispatch_semaphore_wait(sem, 100);
+    return 0;
+}
+
+- (BOOL) writeToErrorInBlockNoAnnotation:(NSError **)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 variable}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    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 variable}}
+        }
+        dispatch_semaphore_signal(sem);
+    });
+
+    dispatch_semaphore_wait(sem, 100);
+  return 0;
+}
+
+BOOL writeToErrorNoWarning(NSError *__autoreleasing* error) {
+  *error = [NSError errorWithDomain:1]; // no-warning
+  return 0;
+}