diff --git a/infer/src/clang/cFrontend_checkers.ml b/infer/src/clang/cFrontend_checkers.ml index 32f17b6a0..5d35088ea 100644 --- a/infer/src/clang/cFrontend_checkers.ml +++ b/infer/src/clang/cFrontend_checkers.ml @@ -310,21 +310,67 @@ let bad_pointer_comparison_warning _ stmt_info stmts = None -(* exist m1: m1.body|- EF call_method(addObserver:) => exists m2 : m2.body |- EF call_method(removeObserver:) *) -let checker_NSNotificationCenter _ decl_info decls = +(* exist m1: m1.body |- EF call_method(addObserver:) => + exists m2 : m2.body |- EF call_method(removeObserver:) *) +let checker_NSNotificationCenter _ decl_info impl_decl_info decls = let exists_method_calling_addObserver = - IList.exists (dec_body_eventually (call_method_on_nth is_self 1) "addObserver:selector:name:object:") decls in + IList.exists + (dec_body_eventually (call_method_on_nth is_self 1) + "addObserver:selector:name:object:") decls in let exists_method_calling_addObserverForName = - IList.exists (dec_body_eventually (call_method_on_nth is_self 1) "addObserverForName:object:queue:usingBlock:") decls in + IList.exists + (dec_body_eventually (call_method_on_nth is_self 1) + "addObserverForName:object:queue:usingBlock:") decls in let eventually_addObserver = exists_method_calling_addObserver || exists_method_calling_addObserverForName in - let exists_method_calling_removeObserver = - IList.exists (dec_body_eventually (call_method_on_nth is_self 1) "removeObserver:") decls in - let exists_method_calling_removeObserverName = - IList.exists (dec_body_eventually (call_method_on_nth is_self 1) "removeObserver:name:object:") decls in - let eventually_removeObserver = exists_method_calling_removeObserver - || exists_method_calling_removeObserverName in - let condition = eventually_addObserver && (not eventually_removeObserver) in + + let eventually_removeObserver decls = + let exists_method_calling_removeObserver = + IList.exists (dec_body_eventually (call_method_on_nth is_self 1) "removeObserver:") decls in + let exists_method_calling_removeObserverName = + IList.exists + (dec_body_eventually (call_method_on_nth is_self 1) + "removeObserver:name:object:") decls in + exists_method_calling_removeObserver || exists_method_calling_removeObserverName in + + let get_super impl_decl_info = + let objc_interface_decl_current = + CFrontend_utils.Ast_utils.get_decl_opt_with_decl_ref + impl_decl_info.Clang_ast_t.oidi_class_interface in + let objc_interface_decl_super = + match objc_interface_decl_current with + | Some Clang_ast_t.ObjCInterfaceDecl(_, _, _, _, interface_decl_info) -> + CFrontend_utils.Ast_utils.get_decl_opt_with_decl_ref interface_decl_info.otdi_super + | _ -> None in + let objc_implementation_decl_super = + match objc_interface_decl_super with + | Some ObjCInterfaceDecl(_, _, _, _, interface_decl_info) -> + CFrontend_utils.Ast_utils.get_decl_opt_with_decl_ref + interface_decl_info.otdi_implementation + | _ -> None in + match objc_implementation_decl_super with + | Some ObjCImplementationDecl(_, _, decl_list, _, impl_decl_info) -> + Some (decl_list, impl_decl_info) + | _ -> None in + + let rec exists_on_hierarchy f super = + match super with + | Some (decl_list, impl_decl_info) -> + f decl_list || exists_on_hierarchy f (get_super impl_decl_info) + | None -> false in + + let eventually_removeObserver_in_whole_hierarchy decls impl_decl_info = + exists_on_hierarchy eventually_removeObserver (Some (decls, impl_decl_info)) in + + (* if registration happens among the given decls, then search for removeObserver across the *) + (* whole hierarchy of classes *) + let condition = + eventually_addObserver && + match impl_decl_info with + | Some idi -> + not (eventually_removeObserver_in_whole_hierarchy decls idi) + | None -> not (eventually_removeObserver decls) in + if condition then Some { CIssue.issue = CIssue.Registered_observer_being_deallocated; diff --git a/infer/src/clang/cFrontend_checkers.mli b/infer/src/clang/cFrontend_checkers.mli index e596dc9dd..7ba5d27d5 100644 --- a/infer/src/clang/cFrontend_checkers.mli +++ b/infer/src/clang/cFrontend_checkers.mli @@ -41,7 +41,8 @@ val bad_pointer_comparison_warning : but not removed before deallocation *) val checker_NSNotificationCenter : CLintersContext.context -> - Clang_ast_t.decl_info -> Clang_ast_t.decl list -> CIssue.issue_desc option + Clang_ast_t.decl_info -> Clang_ast_t.obj_c_implementation_decl_info option -> + Clang_ast_t.decl list -> CIssue.issue_desc option (* GLOBAL_VARIABLE_INITIALIZED_WITH_FUNCTION_OR_METHOD_CALL warning: a global variable initialization should not *) (* contain calls to functions or methods as these can be expensive an delay the starting time *) diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index c9e967db8..2bec5ec25 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -37,8 +37,8 @@ let checkers_for_capture_vars stmt_info captured_vars checker context = let ns_notification_checker_list = [CFrontend_checkers.checker_NSNotificationCenter] (* Invocation of checker belonging to ns_notification_center_list *) -let checkers_for_ns decl_info decls checker context = - checker context decl_info decls +let checkers_for_ns decl_info impl_decl_info decls checker context = + checker context decl_info impl_decl_info decls (* List of checkers on global variables *) let global_var_checker_list = [CFrontend_checkers.global_var_init_with_calls_warning] @@ -172,7 +172,10 @@ let run_frontend_checkers_on_decl context cfg cg dec = match dec with | ObjCImplementationDecl (decl_info, _, decl_list, _, _) | ObjCProtocolDecl (decl_info, _, decl_list, _, _) -> - let call_ns_checker = checkers_for_ns decl_info decl_list in + let idi = match dec with + | ObjCImplementationDecl (_, _, _, _, impl_decl_info) -> Some impl_decl_info + | _ -> None in + let call_ns_checker = checkers_for_ns decl_info idi decl_list in let key = Ast_utils.generate_key_decl dec in invoke_set_of_checkers call_ns_checker context cfg cg None key ns_notification_checker_list; context diff --git a/infer/tests/codetoanalyze/objc/linters/registered_observer/Person.m b/infer/tests/codetoanalyze/objc/linters/registered_observer/Person.m new file mode 100644 index 000000000..12ba5757c --- /dev/null +++ b/infer/tests/codetoanalyze/objc/linters/registered_observer/Person.m @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2016 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +#import + +// OkPerson is the only class in the whole hierarchy that invokes removeObserver + +@interface OkPerson : NSObject + +@property(strong) NSNotificationCenter* nc; + +@end + +@implementation OkPerson + +- (void)boo { + [self.nc removeObserver:self]; +} + +@end + +// --- + +@interface OkPerson2 : OkPerson + +@end + +@implementation OkPerson2 + +- (void)foo:(NSMutableDictionary*)dict { + self.nc = [NSNotificationCenter defaultCenter]; + [self.nc addObserver:self selector:@selector(foo:) name:nil object:nil]; +} + +@end + +// --- + +@interface OkPerson3 : OkPerson2 + +@end + +@implementation OkPerson3 + +- (void)foo:(NSMutableDictionary*)dict { + self.nc = [NSNotificationCenter defaultCenter]; + [self.nc addObserver:self selector:@selector(foo:) name:nil object:nil]; +} + +@end + +// --- + +// No one in the whole hierarchy of BadPerson invokes removeObserver + +@interface BadPerson : NSObject + +@property(strong) NSNotificationCenter* nc; + +@end + +@implementation BadPerson + +@end + +// --- + +@interface BadPerson2 : BadPerson + +@property(strong) NSNotificationCenter* nc; + +@end + +@implementation BadPerson2 + +- (void)foo:(NSMutableDictionary*)dict { + self.nc = [NSNotificationCenter defaultCenter]; + [self.nc addObserver:self selector:@selector(foo:) name:nil object:nil]; +} + +@end + +// --- + +@interface BadPerson3 : BadPerson2 + +@property(strong) NSNotificationCenter* nc; + +@end + +@implementation BadPerson3 + +- (void)fooRegister:(NSMutableDictionary*)dict { + self.nc = [NSNotificationCenter defaultCenter]; + [self.nc addObserver:self selector:@selector(foo:) name:nil object:nil]; +} + +@end diff --git a/infer/tests/endtoend/objc/linters/RegisteredObserver4.java b/infer/tests/endtoend/objc/linters/RegisteredObserver4.java new file mode 100644 index 000000000..cbd96e708 --- /dev/null +++ b/infer/tests/endtoend/objc/linters/RegisteredObserver4.java @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2016 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package endtoend.objc.linters; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsLineNumbers.containsOnlyLines; + +import com.google.common.collect.ImmutableList; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import java.io.IOException; + +import utils.DebuggableTemporaryFolder; +import utils.InferException; +import utils.InferResults; +import utils.InferRunner; + +public class RegisteredObserver4 { + + public static final String VCFile = + "infer/tests/codetoanalyze/objc/linters/registered_observer/Person.m"; + + private static ImmutableList inferCmd; + + public static final String REGISTERED_OBSERVER = "REGISTERED_OBSERVER_BEING_DEALLOCATED"; + + @ClassRule + public static DebuggableTemporaryFolder folder = new DebuggableTemporaryFolder(); + + @BeforeClass + public static void runInfer() throws InterruptedException, IOException { + inferCmd = InferRunner.createObjCInferCommandSimple( + folder, + VCFile, + "cf"); + } + + @Test + public void RegisteredObserverShouldBeFound() + throws InterruptedException, IOException, InferException { + InferResults inferResults = InferRunner.runInferObjC(inferCmd); + assertThat( + "Results should contain " + REGISTERED_OBSERVER, + inferResults, + containsOnlyLines(new int[] {80, 97}) + ); + } + +}