From 24728dc093392b2e067b353b6be71d7336b6221e Mon Sep 17 00:00:00 2001 From: Dino Distefano Date: Fri, 31 May 2019 09:50:33 -0700 Subject: [PATCH] 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 | 17 +++++++ infer/src/clang/cPredicates.ml | 27 +++++++++++ infer/src/clang/cPredicates.mli | 7 +++ infer/src/clang/cTL.ml | 4 ++ .../objc/linters-for-test-only/issues.exp | 2 + .../codetoanalyze/objc/linters/issues.exp | 1 + .../codetoanalyze/objc/linters/optional.m | 48 +++++++++++++++++++ 7 files changed, 106 insertions(+) create mode 100644 infer/tests/codetoanalyze/objc/linters/optional.m diff --git a/infer/lib/linter_rules/linters.al b/infer/lib/linter_rules/linters.al index 8ea573e0c..cf77bea84 100644 --- a/infer/lib/linter_rules/linters.al +++ b/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"; + +}; diff --git a/infer/src/clang/cPredicates.ml b/infer/src/clang/cPredicates.ml index 7a66c5998..a5965f12c 100644 --- a/infer/src/clang/cPredicates.ml +++ b/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 -> ( diff --git a/infer/src/clang/cPredicates.mli b/infer/src/clang/cPredicates.mli index 3bbd7bf0b..799409820 100644 --- a/infer/src/clang/cPredicates.mli +++ b/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 diff --git a/infer/src/clang/cTL.ml b/infer/src/clang/cTL.ml index af3fbbc8e..01513b5c2 100644 --- a/infer/src/clang/cTL.ml +++ b/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 -> diff --git a/infer/tests/codetoanalyze/objc/linters-for-test-only/issues.exp b/infer/tests/codetoanalyze/objc/linters-for-test-only/issues.exp index 4e760e4eb..ba8383dae 100644 --- a/infer/tests/codetoanalyze/objc/linters-for-test-only/issues.exp +++ b/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, [] diff --git a/infer/tests/codetoanalyze/objc/linters/issues.exp b/infer/tests/codetoanalyze/objc/linters/issues.exp index ef8c0563c..ffa5a3532 100644 --- a/infer/tests/codetoanalyze/objc/linters/issues.exp +++ b/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, [] diff --git a/infer/tests/codetoanalyze/objc/linters/optional.m b/infer/tests/codetoanalyze/objc/linters/optional.m new file mode 100644 index 000000000..dcd3b5045 --- /dev/null +++ b/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 + +@protocol BarDelegate +@optional +- (void)optionalFunction; +@end + +@interface Bar : NSObject +@property(weak, nonatomic) id delegate; +- (void)action; +@end + +@implementation Bar +- (void)unsafeAction { + [self.delegate optionalFunction]; +} + +- (void)safeAction { + id delegate = self.delegate; + if ([delegate respondsToSelector:@selector(optionalFunction)]) { + [self.delegate optionalFunction]; + } +} +@end + +@interface Foo : NSObject +- (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; +}