New ObjC checker for calls to @optional methods

Summary:
It is unsafe to call protocol methods defined optional. Before calling them we should check it
the implementation exists by calling

`if ([object respondsToSelector:selector(...)]) ...`

Without the above check we get run time crashes.

Reviewed By: jvillard

Differential Revision: D15554951

fbshipit-source-id: f0560971b

infer/lib/linter_rules/linters.al

@@ -273,3 +273,20 @@ DEFINE-CHECKER WRONG_SCOPE_FOR_DISPATCH_ONCE_T = {
 		SET severity = "WARNING";
 		SET mode = "ON";
 };
+
+
+DEFINE-CHECKER UNSAFE_CALL_TO_OPTIONAL_METHOD = {
+
+  SET report_when =
+   WHEN
+     is_call_to_optional_objc_method
+     AND
+     (NOT objc_method_call_within_responds_to_selector_block)
+  HOLDS-IN-NODE ObjCMessageExpr;
+
+  SET message = "This is a call to an `@optional` protocol method. Calling it without checking if its implemented can lead to crashes at run time.";
+	SET suggestion = "Please make sure to test the method is implemented by first calling `if ([object respondsToSelector:@selector(%decl_name%)]) ...` ";
+	SET severity = "ERROR";
+	SET mode = "ON";
+
+};

infer/src/clang/cPredicates.ml

@@ -856,6 +856,19 @@ let is_optional_objc_method an =
       false
 
 
+let is_call_to_optional_objc_method an =
+  let open Clang_ast_t in
+  match an with
+  | Ctl_parser_types.Stmt (ObjCMessageExpr (_, _, _, omei)) -> (
+    match CAst_utils.get_decl_opt omei.omei_decl_pointer with
+    | Some d ->
+        is_optional_objc_method (Ctl_parser_types.Decl d)
+    | _ ->
+        false )
+  | _ ->
+      false
+
+
 let is_in_block context =
   match context.CLintersContext.current_method with Some (BlockDecl _) -> true | _ -> false
 
@@ -1326,6 +1339,20 @@ let within_responds_to_selector_block (cxt : CLintersContext.context) an =
       false
 
 
+let objc_method_call_within_responds_to_selector_block (cxt : CLintersContext.context) an =
+  let open Clang_ast_t in
+  match an with
+  | Ctl_parser_types.Stmt (ObjCMessageExpr (_, _, _, mdi)) -> (
+    match cxt.if_context with
+    | Some if_context ->
+        let in_selector_block = if_context.within_responds_to_selector_block in
+        List.mem ~equal:String.equal in_selector_block mdi.omei_selector
+    | None ->
+        false )
+  | _ ->
+      false
+
+
 let within_available_class_block (cxt : CLintersContext.context) an =
   match (receiver_method_call an, cxt.if_context) with
   | Some receiver, Some if_context -> (

infer/src/clang/cPredicates.mli

@@ -93,6 +93,9 @@ val is_in_block : CLintersContext.context -> bool
 val is_optional_objc_method : Ctl_parser_types.ast_node -> bool
 (** true if the current node is an objc method declaration which is declared with @optional *)
 
+val is_call_to_optional_objc_method : Ctl_parser_types.ast_node -> bool
+(** true if the current node is a call to an objc method declaration which is declared with @optional *)
+
 val is_in_cxx_constructor : CLintersContext.context -> ALVar.alexp -> bool
 (** 'is_in_cxx_constructor context name' is true if the curent node is within a CXX constructor whose name contains 'name' *)
 
@@ -425,6 +428,10 @@ val get_selector : Ctl_parser_types.ast_node -> string option
 val within_responds_to_selector_block :
   CLintersContext.context -> Ctl_parser_types.ast_node -> bool
 
+val objc_method_call_within_responds_to_selector_block :
+  CLintersContext.context -> Ctl_parser_types.ast_node -> bool
+(** true if a ObjC method call is withing the scope of a responds_to_selector check *)
+
 val using_namespace : Ctl_parser_types.ast_node -> ALVar.alexp -> bool
 
 val receiver_class_method_call : Ctl_parser_types.ast_node -> Clang_ast_t.decl option

infer/src/clang/cTL.ml

@@ -1053,6 +1053,8 @@ let rec eval_Atomic pred_name_ args an lcxt =
       CPredicates.is_in_block lcxt
   | "is_optional_objc_method", [], an ->
       CPredicates.is_optional_objc_method an
+  | "is_call_to_optional_objc_method", [], an ->
+      CPredicates.is_call_to_optional_objc_method an
   | "is_in_cxx_constructor", [name], _ ->
       CPredicates.is_in_cxx_constructor lcxt name
   | "is_in_cxx_destructor", [name], _ ->
@@ -1173,6 +1175,8 @@ let rec eval_Atomic pred_name_ args an lcxt =
       CPredicates.method_return_type an typ
   | "within_responds_to_selector_block", [], an ->
       CPredicates.within_responds_to_selector_block lcxt an
+  | "objc_method_call_within_responds_to_selector_block", [], an ->
+      CPredicates.objc_method_call_within_responds_to_selector_block lcxt an
   | "using_namespace", [namespace], an ->
       CPredicates.using_namespace an namespace
   | "is_at_selector_with_name", [name], an ->

infer/tests/codetoanalyze/objc/linters-for-test-only/issues.exp

@@ -105,6 +105,7 @@ codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBase
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 68, TEST_ALL_METHODS, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 68, TEST_IS_RECEIVER_CLASS_NAMED, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 69, TEST_IS_RECEIVER_ID_TYPE, no_bucket, WARNING, []
+codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 69, UNSAFE_CALL_TO_OPTIONAL_METHOD, no_bucket, ERROR, []
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 70, TEST_IS_RECEIVER_CLASS_NAMED, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 71, TEST_ALL_METHODS, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/GenericTestClass.m, MyBaseClass::myBaseClassTestReceiver, 71, TEST_IS_RECEIVER_CLASS_TYPE, no_bucket, WARNING, []
@@ -248,6 +249,7 @@ codetoanalyze/objc/linters-for-test-only/namespace.mm, Linters_dummy_method, 15,
 codetoanalyze/objc/linters-for-test-only/ns_assume_nonnull.m, Blah::someMethod, 14, TEST_INSTANCE_TYPE, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/ns_assume_nonnull.m, Blah::someMethod, 20, TEST_INSTANCE_TYPE, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/ns_assume_nonnull.m, Blah::someMethod, 20, TEST_IS_METHOD_EXPOSED, no_bucket, WARNING, []
+codetoanalyze/objc/linters-for-test-only/optional.m, Bar::unsafeAction, 25, UNSAFE_CALL_TO_OPTIONAL_METHOD, no_bucket, ERROR, []
 codetoanalyze/objc/linters-for-test-only/optional.m, BarDelegate::optionalFunction, 16, TEST_IS_OPTIONAL_METHOD, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/optional.m, Linters_dummy_method, 20, STRONG_DELEGATE_WARNING, no_bucket, WARNING, []
 codetoanalyze/objc/linters-for-test-only/protocols.m, Foo::newWithA, 22, TEST_INSTANCE_TYPE, no_bucket, WARNING, []

infer/tests/codetoanalyze/objc/linters/issues.exp

@@ -43,6 +43,7 @@ codetoanalyze/objc/linters/nsnumber.m, bad1, 11, BAD_POINTER_COMPARISON, no_buck
 codetoanalyze/objc/linters/nsnumber.m, bad2, 16, BAD_POINTER_COMPARISON, no_bucket, WARNING, []
 codetoanalyze/objc/linters/nsnumber.m, bad3, 21, BAD_POINTER_COMPARISON, no_bucket, WARNING, []
 codetoanalyze/objc/linters/nsnumber.m, bad4, 41, BAD_POINTER_COMPARISON, no_bucket, WARNING, []
+codetoanalyze/objc/linters/optional.m, Bar::unsafeAction, 21, UNSAFE_CALL_TO_OPTIONAL_METHOD, no_bucket, ERROR, []
 codetoanalyze/objc/linters/registered_observer/Person.m, Linters_dummy_method, 78, REGISTERED_OBSERVER_BEING_DEALLOCATED, no_bucket, WARNING, []
 codetoanalyze/objc/linters/registered_observer/Person.m, Linters_dummy_method, 95, REGISTERED_OBSERVER_BEING_DEALLOCATED, no_bucket, WARNING, []
 codetoanalyze/objc/linters/registered_observer/ViewController3.m, Linters_dummy_method, 12, REGISTERED_OBSERVER_BEING_DEALLOCATED, no_bucket, WARNING, []

infer/tests/codetoanalyze/objc/linters/optional.m

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2019-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+#import <Foundation/NSObject.h>
+
+@protocol BarDelegate<NSObject>
+@optional
+- (void)optionalFunction;
+@end
+
+@interface Bar : NSObject
+@property(weak, nonatomic) id<BarDelegate> delegate;
+- (void)action;
+@end
+
+@implementation Bar
+- (void)unsafeAction {
+  [self.delegate optionalFunction];
+}
+
+- (void)safeAction {
+  id<BarDelegate> delegate = self.delegate;
+  if ([delegate respondsToSelector:@selector(optionalFunction)]) {
+    [self.delegate optionalFunction];
+  }
+}
+@end
+
+@interface Foo : NSObject<BarDelegate>
+- (void)doSomething;
+@end
+
+@implementation Foo
+- (void)doSomething {
+  Bar* x = [Bar new];
+  x.delegate = self;
+  [x unsafeAction];
+}
+@end
+
+int main(int argc, const char* argv[]) {
+  Foo* y = [Foo new];
+  [y doSomething];
+  return 0;
+}