Check for the presence of removeObserver into class hierarchy

Reviewed By: jberdine

Differential Revision: D3669624

fbshipit-source-id: 559c66e

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;

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 *)

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

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 <Foundation/Foundation.h>
+
+// 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

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<String> 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})
+    );
+  }
+
+}