[Sema][ObjC] Warn about 'performSelector' calls with selectors that return record types

[arphaman]

Summary

The performSelector family of methods from Foundation use objc_msgSend to dispatch the selector invocations to objects. However, method calls to methods that return record types might have to use the objc_msgSend_stret as the return value won't find into the register [1]. This is also supported by this sentence from performSelector documentation: "The method should not have a significant return value and should take a single argument of type id, or no arguments" [2]. This patch adds a new warning that warns when a selector which corresponds to a method that returns a record type is passed into performSelector.

[1] http://www.tomdalling.com/blog/cocoa/why-performselector-is-more-dangerous-than-i-thought/
[2] https://developer.apple.com/reference/objectivec/nsobject/1416176-performselector?language=objc

[ahatanak]

Do we still issue a warning even when the struct can be returned in a register? For example, x86 can return a small struct (for example, a struct with one int field) in a single register, in which case it's fine to pass it to performSelector via @selector.

If we should warn only when the method has to return via sret, then it looks like we have to delay issuing the warning until we know where the return value goes (IRGen?).

[arphaman]

In D30174#681801, @ahatanak wrote:

Do we still issue a warning even when the struct can be returned in a register? For example, x86 can return a small struct (for example, a struct with one int field) in a single register, in which case it's fine to pass it to performSelector via @selector.

Yes, we do. Primarily for the following reason: even if some target may return a struct in a register, another target isn't guaranteed to do the same thing. It's better to always warn about this rather than accept some small structs. Furthermore, the official documentation states that "For methods that return anything other than an object, use NSInvocation." [1], so methods that return records are completely prohibited by the docs.

If we should warn only when the method has to return via sret, then it looks like we have to delay issuing the warning until we know where the return value goes (IRGen?).

[1] https://developer.apple.com/reference/objectivec/1418956-nsobject/1418867-performselector?language=objc

[ahatanak]

In D30174#681843, @arphaman wrote:

Yes, we do. Primarily for the following reason: even if some target may return a struct in a register, another target isn't guaranteed to do the same thing. It's better to always warn about this rather than accept some small structs. Furthermore, the official documentation states that "For methods that return anything other than an object, use NSInvocation." [1], so methods that return records are completely prohibited by the docs.

OK, I think you are right. It doesn't seem like a good idea to warn when compiling for one target and not warn when compiling for another target.

If the official documentation says the method should return an object, why not prohibit all methods that don't return a pointer to an object (methods like returnsInt in the test case)? Doing so will also take care of methods that don't return struct but still return via sret (for example, I think some targets return vectors via sret).

Also, the documentation says that methods passed to performSelectorInBackground and performSelectorOnMainThread should not have a significant return value. Does it mean that the methods can have any return type but the return value will be ignored (I noticed that those two methods return "void" unlike performSelector which returns "id")?

[arphaman]

In D30174#681890, @ahatanak wrote:

In D30174#681843, @arphaman wrote:

Yes, we do. Primarily for the following reason: even if some target may return a struct in a register, another target isn't guaranteed to do the same thing. It's better to always warn about this rather than accept some small structs. Furthermore, the official documentation states that "For methods that return anything other than an object, use NSInvocation." [1], so methods that return records are completely prohibited by the docs.

OK, I think you are right. It doesn't seem like a good idea to warn when compiling for one target and not warn when compiling for another target.

If the official documentation says the method should return an object, why not prohibit all methods that don't return a pointer to an object (methods like returnsInt in the test case)? Doing so will also take care of methods that don't return struct but still return via sret (for example, I think some targets return vectors via sret).

I think it would catch too many "false positives" if we prohibit all primitive types. This might make the warning less effective as well, since users might get annoyed and completely disable the warning if they think that it's too strict. Maybe we can have a stricter version of the warning that's not on by default? I agree that we can warn on vectors as well though.

Also, the documentation says that methods passed to performSelectorInBackground and performSelectorOnMainThread should not have a significant return value. Does it mean that the methods can have any return type but the return value will be ignored (I noticed that those two methods return "void" unlike performSelector which returns "id")?

No, they still use objc_msgSend, so the memory corruption problem still applies to them as they can write to the object thinking that it's the pointer to the return value. The fact that performSelectorInBackground and performSelectorOnMainThread don't return anything doesn't really make a difference.

[ahatanak]

I'm not sure how common it is to pass a function that doesn't return an object or void, I think it's OK to allow returning primitive types if you think being too strict would catch too many false positives.

lib/AST/DeclObjC.cpp

987 ↗

(On Diff #89132)

It seems like this code would set "family" to OMF_None for some of the performSelector functions. For example:

https://developer.apple.com/reference/objectivec/nsobject/1411637-performselectoronmainthread?language=objc
https://developer.apple.com/reference/objectivec/nsobject/1417922-performselector?language=objc

Do those functions belong to the performSelector family of methods?

lib/Sema/SemaExprObjC.cpp

2280 ↗

(On Diff #89132)

Do you need to check if OPT->getInterfaceDecl() returns null here? What happens if OPT is id?

2499 ↗

(On Diff #89132)

I'm not sure why checkFoundationAPI has to be called inside the else statement. Was there a reason you didn't or couldn't move it outside?

[arphaman]

lib/AST/DeclObjC.cpp

987 ↗

(On Diff #89132)

Good catch! Yes, we should be more inclusive here.

lib/Sema/SemaExprObjC.cpp

2280 ↗

(On Diff #89132)

You're right, my mistake.

2499 ↗

(On Diff #89132)

We should check for 'super' calls as wells, you're right.

[arphaman]

The updated diff:

• Warns for vector types.
• Addresses Akira's comments.

[ahatanak]

Looks good, thanks!

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_objc_object_catch : Error<
   "cannot catch an Objective-C object by value">;
 def err_incomplete_type_objc_at_encode : Error<
   "'@encode' of incomplete type %0">;
 def warn_objc_circular_container : Warning<
   "adding '%0' to '%1' might cause circular dependency in container">,
   InGroup<DiagGroup<"objc-circular-container">>;
 def note_objc_circular_container_declared_here : Note<"'%0' declared here">;
+def warn_objc_unsafe_perform_selector : Warning<
+  "%0 is incompatible with selectors that return a "
+  "%select{struct|union|vector}1 type">,
+  InGroup<DiagGroup<"objc-unsafe-perform-selector">>;
+def note_objc_unsafe_perform_selector_method_declared_here :  Note<
+  "method %0 that returns %1 declared here">;
 
 def warn_setter_getter_impl_required : Warning<
   "property %0 requires method %1 to be defined - "
   "use @synthesize, @dynamic or provide a method implementation "
   "in this class implementation">,
   InGroup<ObjCPropertyImpl>;
 def warn_setter_getter_impl_required_in_category : Warning<
   "property %0 requires method %1 to be defined - "

cfe/trunk/lib/AST/DeclObjC.cpp

case OMF_self:
     break;
       
   case OMF_initialize:
     if (isInstanceMethod() || !getReturnType()->isVoidType())
       family = OMF_None;
     break;
       
   case OMF_performSelector:
-    if (!isInstanceMethod() || !getReturnType()->isObjCIdType())
+    if (!isInstanceMethod() ||
+        !(getReturnType()->isObjCIdType() || getReturnType()->isVoidType()))
       family = OMF_None;
     else {
       unsigned noParams = param_size();
-      if (noParams < 1 || noParams > 3)
+      if (noParams < 1 || noParams > 5)
         family = OMF_None;
       else {
         ObjCMethodDecl::param_type_iterator it = param_type_begin();
         QualType ArgT = (*it);
         if (!ArgT->isObjCSelType()) {
           family = OMF_None;
           break;
         }
-        while (--noParams) {
-          it++;
-          ArgT = (*it);
-          if (!ArgT->isObjCIdType()) {
+        // The first type should generally always be 'id' or 'Thread *', the
+        // other types can vary.
+        if (noParams > 1) {
+          ArgT = *(it + 1);
+          if (!ArgT->isObjCObjectPointerType()) {
             family = OMF_None;
             break;
           }
         }
       }
     }
     break;
       

cfe/trunk/lib/Basic/IdentifierTable.cpp

if (sel.isUnarySelector()) {
     if (name == "dealloc") return OMF_dealloc;
     if (name == "finalize") return OMF_finalize;
     if (name == "release") return OMF_release;
     if (name == "retain") return OMF_retain;
     if (name == "retainCount") return OMF_retainCount;
     if (name == "self") return OMF_self;
     if (name == "initialize") return OMF_initialize;
   }
- 
-  if (name == "performSelector") return OMF_performSelector;
+  if (name == "performSelector" || name == "performSelectorInBackground" ||
+      name == "performSelectorOnMainThread")
+    return OMF_performSelector;
 
   // The other method families may begin with a prefix of underscores.
   while (!name.empty() && name.front() == '_')
     name = name.substr(1);
 
   if (name.empty()) return OMF_None;
   switch (name.front()) {
   case 'a':

cfe/trunk/lib/Sema/SemaExprObjC.cpp

static void applyCocoaAPICheck(Sema &S, const ObjCMessageExpr *Msg,
   }
 }
 
 static void checkCocoaAPI(Sema &S, const ObjCMessageExpr *Msg) {
   applyCocoaAPICheck(S, Msg, diag::warn_objc_redundant_literal_use,
                      edit::rewriteObjCRedundantCallWithLiteral);
 }
 
+static void checkFoundationAPI(Sema &S, SourceLocation Loc,
+                               const ObjCMethodDecl *Method,
+                               ArrayRef<Expr *> Args, QualType ReceiverType,
+                               bool IsClassObjectCall) {
+  // Check if this is a performSelector method that uses a selector that returns
+  // a record or a vector type.
+  if (Method->getMethodFamily() != OMF_performSelector || Args.empty())
+    return;
+  const auto *SE = dyn_cast<ObjCSelectorExpr>(Args[0]->IgnoreParens());
+  if (!SE)
+    return;
+  ObjCMethodDecl *ImpliedMethod;
+  if (!IsClassObjectCall) {
+    const auto *OPT = ReceiverType->getAs<ObjCObjectPointerType>();
+    if (!OPT || !OPT->getInterfaceDecl())
+      return;
+    ImpliedMethod =
+        OPT->getInterfaceDecl()->lookupInstanceMethod(SE->getSelector());
+    if (!ImpliedMethod)
+      ImpliedMethod =
+          OPT->getInterfaceDecl()->lookupPrivateMethod(SE->getSelector());
+  } else {
+    const auto *IT = ReceiverType->getAs<ObjCInterfaceType>();
+    if (!IT)
+      return;
+    ImpliedMethod = IT->getDecl()->lookupClassMethod(SE->getSelector());
+    if (!ImpliedMethod)
+      ImpliedMethod =
+          IT->getDecl()->lookupPrivateClassMethod(SE->getSelector());
+  }
+  if (!ImpliedMethod)
+    return;
+  QualType Ret = ImpliedMethod->getReturnType();
+  if (Ret->isRecordType() || Ret->isVectorType() || Ret->isExtVectorType()) {
+    QualType Ret = ImpliedMethod->getReturnType();
+    S.Diag(Loc, diag::warn_objc_unsafe_perform_selector)
+        << Method->getSelector()
+        << (!Ret->isRecordType()
+                ? /*Vector*/ 2
+                : Ret->isUnionType() ? /*Union*/ 1 : /*Struct*/ 0);
+    S.Diag(ImpliedMethod->getLocStart(),
+           diag::note_objc_unsafe_perform_selector_method_declared_here)
+        << ImpliedMethod->getSelector() << Ret;
+  }
+}
+
 /// \brief Diagnose use of %s directive in an NSString which is being passed
 /// as formatting string to formatting method.
 static void
 DiagnoseCStringFormatDirectiveInObjCAPI(Sema &S,
                                         ObjCMethodDecl *Method,
                                         Selector Sel,
                                         Expr **Args, unsigned NumArgs) {
   unsigned Idx = 0;
ExprResult Sema::BuildClassMessage(TypeSourceInfo *ReceiverTypeInfo,
   else {
     Result = ObjCMessageExpr::Create(Context, ReturnType, VK, LBracLoc, 
                                      ReceiverTypeInfo, Sel, SelectorLocs,
                                      Method, makeArrayRef(Args, NumArgs),
                                      RBracLoc, isImplicit);
     if (!isImplicit)
       checkCocoaAPI(*this, Result);
   }
+  if (Method)
+    checkFoundationAPI(*this, SelLoc, Method, makeArrayRef(Args, NumArgs),
+                       ReceiverType, /*IsClassObjectCall=*/true);
   return MaybeBindToTemporary(Result);
 }
 
 // ActOnClassMessage - used for both unary and keyword messages.
 // ArgExprs is optional - if it is present, the number of expressions
 // is obtained from Sel.getNumArgs().
 ExprResult Sema::ActOnClassMessage(Scope *S, 
                                    ParsedType Receiver,
ExprResult Sema::BuildInstanceMessage(Expr *Receiver,
   else {
     Result = ObjCMessageExpr::Create(Context, ReturnType, VK, LBracLoc,
                                      Receiver, Sel, SelectorLocs, Method,
                                      makeArrayRef(Args, NumArgs), RBracLoc,
                                      isImplicit);
     if (!isImplicit)
       checkCocoaAPI(*this, Result);
   }
+  if (Method) {
+    bool IsClassObjectCall = ClassMessage;
+    // 'self' message receivers in class methods should be treated as message
+    // sends to the class object in order for the semantic checks to be
+    // performed correctly. Messages to 'super' already count as class messages,
+    // so they don't need to be handled here.
+    if (Receiver && isSelfExpr(Receiver)) {
+      if (const auto *OPT = ReceiverType->getAs<ObjCObjectPointerType>()) {
+        if (OPT->getObjectType()->isObjCClass()) {
+          if (const auto *CurMeth = getCurMethodDecl()) {
+            IsClassObjectCall = true;
+            ReceiverType =
+                Context.getObjCInterfaceType(CurMeth->getClassInterface());
+          }
+        }
+      }
+    }
+    checkFoundationAPI(*this, SelLoc, Method, makeArrayRef(Args, NumArgs),
+                       ReceiverType, IsClassObjectCall);
+  }
 
   if (getLangOpts().ObjCAutoRefCount) {
     // In ARC, annotate delegate init calls.
     if (Result->getMethodFamily() == OMF_init &&
         (SuperLoc.isValid() || isSelfExpr(Receiver))) {
       // Only consider init calls *directly* in init implementations,
       // not within blocks.
       ObjCMethodDecl *method = dyn_cast<ObjCMethodDecl>(CurContext);

cfe/trunk/test/SemaObjC/unsafe-perform-selector.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fsyntax-only -fobjc-arc -verify %s
+// rdar://12056271
+
+@class Thread;
+
+__attribute__((objc_root_class))
+@interface NSObject
+
+- (id)performSelector:(SEL)sel;
+- (void)performSelectorInBackground:(SEL)sel withObject:(id)arg;
+- (void)performSelectorOnMainThread:(SEL)sel;
+
+- (void)performSelectorOnMainThread:(SEL)aSelector
+                           onThread:(Thread *)thread
+                         withObject:(id)arg
+                      waitUntilDone:(int)wait
+                              modes:(id *)array;
+
+@end
+
+typedef struct { int x; int y; int width; int height; } Rectangle;
+
+struct Struct { Rectangle r; };
+
+typedef union { int x; float f; } Union;
+
+@interface Base : NSObject
+
+- (struct Struct)returnsStruct2; // expected-note {{method 'returnsStruct2' that returns 'struct Struct' declared here}}
+- (Union)returnsId;
+
+@end
+
+@protocol IP
+
+- (Union)returnsUnion; // expected-note 2 {{method 'returnsUnion' that returns 'Union' declared here}}
+
+@end
+
+typedef __attribute__((__ext_vector_type__(3))) float float3;
+typedef int int4 __attribute__ ((vector_size (16)));
+
+@interface I : Base<IP>
+
+- (Rectangle)returnsStruct; // expected-note 4 {{method 'returnsStruct' that returns 'Rectangle' declared here}}
+- (id)returnsId; // shadows base 'returnsId'
+- (int)returnsInt;
+- (I *)returnPtr;
+- (float3)returnsExtVector; // expected-note {{method 'returnsExtVector' that returns 'float3' (vector of 3 'float' values) declared here}}
+- (int4)returnsVector; // expected-note {{method 'returnsVector' that returns 'int4' (vector of 4 'int' values) declared here}}
+
++ (Rectangle)returnsStructClass; // expected-note 2 {{method 'returnsStructClass' that returns 'Rectangle' declared here}}
++ (void)returnsUnion; // Not really
+
+@end
+
+void foo(I *i) {
+  [i performSelector: @selector(returnsStruct)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+  [i performSelectorInBackground: @selector(returnsStruct) withObject:0]; // expected-warning {{'performSelectorInBackground:withObject:' is incompatible with selectors that return a struct type}}
+  [i performSelector: ((@selector(returnsUnion)))]; // expected-warning {{'performSelector:' is incompatible with selectors that return a union type}}
+  [i performSelectorOnMainThread: @selector(returnsStruct2)]; // expected-warning {{'performSelectorOnMainThread:' is incompatible with selectors that return a struct type}}
+  [I performSelector: (@selector(returnsStructClass))]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+
+  [i performSelector: @selector(returnsId)];
+  [i performSelector: @selector(returnsInt)];
+  [i performSelector: @selector(returnsPtr)];
+  [I performSelector: @selector(returnsUnion)]; // No warning expected
+
+  id obj = i;
+  [obj performSelector: @selector(returnsId)];
+  [obj performSelector: @selector(returnsStruct)];
+}
+
+@interface SubClass: I
+
+@end
+
+@interface SubClass ()
+- (struct Struct)returnsSubStructExt; // expected-note {{method 'returnsSubStructExt' that returns 'struct Struct' declared here}} expected-note {{method 'returnsSubStructExt' declared here}}
+@end
+
+@implementation SubClass // expected-warning {{method definition for 'returnsSubStructExt' not found}}
+
+- (struct Struct)returnsSubStructImpl { // expected-note {{method 'returnsSubStructImpl' that returns 'struct Struct' declared here}}
+  struct Struct Result;
+  return Result;
+}
+
+- (void)checkPrivateCalls {
+  [self performSelector: @selector(returnsSubStructExt)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+  [self performSelector: @selector(returnsSubStructImpl)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+}
+
+- (void)checkSuperCalls {
+  [super performSelector: @selector(returnsStruct)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+  [super performSelectorInBackground: @selector(returnsUnion) withObject: self]; // expected-warning {{'performSelectorInBackground:withObject:' is incompatible with selectors that return a union type}}
+  [super performSelector: @selector(returnsId)];
+}
+
++ (struct Struct)returnsSubStructClassImpl { // expected-note {{method 'returnsSubStructClassImpl' that returns 'struct Struct' declared here}}
+  struct Struct Result;
+  return Result;
+}
+
++ (void)checkClassPrivateCalls {
+  [self performSelector: @selector(returnsSubStructClassImpl)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+}
+
++ (void)checkClassSuperCalls {
+  [super performSelector: @selector(returnsStructClass)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a struct type}}
+  [super performSelector: @selector(returnsUnion)]; // No warning expected
+}
+
+@end
+
+@implementation I (LongPerformSelectors)
+
+- (void)checkLongCallsFromCategory {
+  [self performSelectorOnMainThread: @selector(returnsStruct) onThread:0 withObject:self waitUntilDone:1 modes:0]; // expected-warning {{'performSelectorOnMainThread:onThread:withObject:waitUntilDone:modes:' is incompatible with selectors that return a struct type}}
+}
+
+- (void)checkVectorReturn {
+  [self performSelector: @selector(returnsExtVector)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a vector type}}
+  [self performSelector: @selector(returnsVector)]; // expected-warning {{'performSelector:' is incompatible with selectors that return a vector type}}
+}
+
+@end