[Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks

[ahatanak]

Summary

ARC implicitly marks indirect parameters passed to a function as autoreleasing and passing a block that captures those parameters to another function sometimes causes problems that are hard to debug.

For example, in the code below, a block capturing fillMeIn is passed to enumerateObjectsUsingBlock, in which an autorelease pool is pushed and popped before and after the block is invoked:

void doStuff(NSString **fillMeIn) {
    [@[@"array"] enumerateObjectsUsingBlock:
      ^(id obj, NSUInteger idx, BOOL* stop) {
        *stop = YES;
        *fillMeIn = [@"wow" mutableCopy];
      }
     ];
}

The object assigned to *fillMeIn gets autoreleased inside the block, so it gets destructed when the autorelease pool is drained, and the program will crash if it tries to access the NSString returned in fillMeIn after doStuff returns.

To help the users figure out why the program is crashing, this patch adds a new warning "-Wblock-capture-autoreleasing" which warns about implicitly autoreleasing indirect parameters captured by blocks and suggests explicitly specifying ownership qualification.

[rjmccall]

include/clang/Basic/DiagnosticSemaKinds.td

5080

How about:

warning: block captures an autoreleasing out-parameter, which may result in use-after-free bugs
note: declare the parameter __autoreleasing explicitly to suppress this warning  (include a fixit on this one)
note: declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools

[ahatanak]

Improve warning messages.

[rjmccall]

Thanks. LGTM.

[Cy-4AH]

Hello!

What compiler do when call doStuff:

NSString * __strong fillMe;
NSString * __autoreleasing autorelesingFillMe = fillMe;
doStuff(&fillMe);
fillMe = autoreleasingFillMe;

Why it's not making the same thing in doStuff body?
Compiler generated code for yours example should be:

void doStuff(NSString **fillMeIn) {
    __block NSString *fillMeInResult;
    __block BOOL fillMeInResultHasChanged = NO;
    [@[@"array"] enumerateObjectsUsingBlock:
      ^(id obj, NSUInteger idx, BOOL* stop) {
        *stop = YES;
        fillMeInResult = [@"wow" mutableCopy];
        fillMeInResultHasChanged = YES;
      }
    ];
    if (fillMeInResultHasChanged) {
      *fillMeIn = fillMeInResult;
    }
}

Instead we got this warning and had to do the same stuff in every implementation

[rjmccall]

That's not an unreasonable idea, and we did consider it, and in fact it's still on the table as something we could do. However, it's a significantly more complex change, and we're not committed to doing it, and so we wanted to at least put this warning in place to help people avoid a relatively common bug.

include/clang/Basic/DiagnosticGroups.td

 def ARCNonPodMemAccess : DiagGroup<"arc-non-pod-memaccess">;
 def AutomaticReferenceCounting : DiagGroup<"arc",
                                            [ARCUnsafeRetainedAssign,
                                             ARCRetainCycles,
                                             ARCNonPodMemAccess]>;
 def ARCRepeatedUseOfWeakMaybe : DiagGroup<"arc-maybe-repeated-use-of-weak">;
 def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak",
                                      [ARCRepeatedUseOfWeakMaybe]>;
+def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">;
 def ObjCBridge : DiagGroup<"bridge-cast">;
 
 def DeallocInCategory:DiagGroup<"dealloc-in-category">;
 def SelectorTypeMismatch : DiagGroup<"selector-type-mismatch">;
 def Selector : DiagGroup<"selector", [SelectorTypeMismatch]>;
 def Protocol : DiagGroup<"protocol">;
 def AtProtocol : DiagGroup<"at-protocol">;
 def PropertyAccessDotSyntax: DiagGroup<"property-access-dot-syntax">;

include/clang/Basic/DiagnosticSemaKinds.td

def err_arc_assign_property_ownership : Error<
   "existing instance variable %1 for property %0 with %select{unsafe_unretained|assign}2 "
   "attribute must be __unsafe_unretained">;
 def err_arc_inconsistent_property_ownership : Error<
   "%select{|unsafe_unretained|strong|weak}1 property %0 may not also be "
   "declared %select{|__unsafe_unretained|__strong|__weak|__autoreleasing}2">;
 
 } // end "ARC and @properties" category
 
+def warn_block_capture_autoreleasing : Warning<
+  "block captures an autoreleasing out-parameter, which may result in "
+  "use-after-free bugs">,
+  InGroup<BlockCaptureAutoReleasing>, DefaultIgnore;
+def note_declare_parameter_autoreleasing : Note<
+  "declare the parameter __autoreleasing explicitly to suppress this warning">;
+def note_declare_parameter_strong : Note<
+  "declare the parameter __strong or capture a __block __strong variable to "
+  "keep values alive across autorelease pools">;
+
 def err_arc_atomic_ownership : Error<
   "cannot perform atomic operation on a pointer to type %0: type has "
   "non-trivial ownership">;
 
 let CategoryName = "ARC Casting Rules" in {
 
 def err_arc_bridge_cast_incompatible : Error<
   "incompatible types casting %0 to %1 with a %select{__bridge|"

lib/Sema/SemaExpr.cpp

if (CaptureType.getObjCLifetime() == Qualifiers::OCL_Autoreleasing) {
     if (BuildAndDiagnose) {
       S.Diag(Loc, diag::err_arc_autoreleasing_capture)
         << /*block*/ 0;
       S.Diag(Var->getLocation(), diag::note_previous_decl)
         << Var->getDeclName();
     }
     return false;
   }
+
+  // Warn about implicitly autoreleasing indirect parameters captured by blocks.
+  if (auto *PT = dyn_cast<PointerType>(CaptureType)) {
+    QualType PointeeTy = PT->getPointeeType();
+    if (isa<ObjCObjectPointerType>(PointeeTy.getCanonicalType()) &&
+        PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing &&
+        !isa<AttributedType>(PointeeTy)) {
+      if (BuildAndDiagnose) {
+        SourceLocation VarLoc = Var->getLocation();
+        S.Diag(Loc, diag::warn_block_capture_autoreleasing);
+        S.Diag(VarLoc, diag::note_declare_parameter_autoreleasing) <<
+            FixItHint::CreateInsertion(VarLoc, "__autoreleasing");;
+        S.Diag(VarLoc, diag::note_declare_parameter_strong);
+      }
+    }
+  }
+
   const bool HasBlocksAttr = Var->hasAttr<BlocksAttr>();
   if (HasBlocksAttr || CaptureType->isReferenceType() ||
       (S.getLangOpts().OpenMP && S.IsOpenMPCapturedDecl(Var))) {
     // Block capture by reference does not change the capture or
     // declaration reference types.
     ByRef = true;
   } else {
     // Block capture by copy introduces 'const'.

test/SemaObjC/arc.m

-// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class -Wblock-capture-autoreleasing %s
 
 typedef unsigned long NSUInteger;
 typedef const void * CFTypeRef;
 CFTypeRef CFBridgingRetain(id X);
 id CFBridgingRelease(CFTypeRef);
 @protocol NSCopying @end
 @interface NSDictionary
 + (id)dictionaryWithObjects:(const id [])objects forKeys:(const id <NSCopying> [])keys count:(NSUInteger)cnt;
#define TKAssertEqual(a, b) do{\
     }\
 }while(0)
 
 int garf() {
   id object;
   TKAssertEqual(object, nil);
   TKAssertEqual(object, (id)nil);
 }
+
+void block_capture_autoreleasing(A * __autoreleasing *a, A **b) { // expected-note {{declare the parameter __autoreleasing explicitly to suppress this warning}} expected-note {{declare the parameter __strong or capture a __block __strong variable to keep values alive across autorelease pools}}
+  ^{
+    (void)*a;
+    (void)*b; // expected-warning {{block captures an autoreleasing out-parameter, which may result in use-after-free bugs}}
+  }();
+}