From c868f51b2d65855fd552d0e7d4ec790f687a4769 Mon Sep 17 00:00:00 2001 From: Dino Distefano Date: Fri, 26 Feb 2016 09:50:22 -0800 Subject: [PATCH] Introducing checker for observer of notification centers Summary:public An observer object that registered to a notification center needs to be unregistered before it is deallocated. If not, the notification center may send a notification to a gost object. This diff introduce a checker for this problem. Reviewed By: dulmarod Differential Revision: D2949692 fb-gh-sync-id: 1653cec shipit-source-id: 1653cec --- infer/lib/python/inferlib/issues.py | 1 + infer/src/backend/abs.ml | 17 ++++ infer/src/backend/exceptions.ml | 4 + infer/src/backend/exceptions.mli | 1 + infer/src/backend/localise.ml | 11 +++ infer/src/backend/localise.mli | 3 + infer/src/backend/prop.ml | 3 + infer/src/backend/prop.mli | 3 + .../registered_observer/ViewController.m | 88 +++++++++++++++++++ .../endtoend/objc/RegisteredObserver.java | 66 ++++++++++++++ infer/tests/utils/InferResults.java | 1 + infer/tests/utils/InferRunner.java | 14 +++ 12 files changed, 212 insertions(+) create mode 100644 infer/tests/codetoanalyze/objc/errors/registered_observer/ViewController.m create mode 100644 infer/tests/endtoend/objc/RegisteredObserver.java diff --git a/infer/lib/python/inferlib/issues.py b/infer/lib/python/inferlib/issues.py index aa8926437..cac2ba43c 100644 --- a/infer/lib/python/inferlib/issues.py +++ b/infer/lib/python/inferlib/issues.py @@ -48,6 +48,7 @@ ISSUE_TYPES = [ 'PREMATURE_NIL_TERMINATION_ARGUMENT', 'DIRECT_ATOMIC_PROPERTY_ACCESS', 'CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK', + 'REGISTERED_OBSERVER_BEING_DEALLOCATED', ] NULL_STYLE_ISSUE_TYPES = [ diff --git a/infer/src/backend/abs.ml b/infer/src/backend/abs.ml index 53f84dddd..4a56381db 100644 --- a/infer/src/backend/abs.ml +++ b/infer/src/backend/abs.ml @@ -1016,6 +1016,22 @@ let cycle_has_weak_or_unretained_or_assign_field cycle = else do_cycle c' in do_cycle cycle +let check_observer_is_unsubscribed_deallocation prop e = + let pvar_opt = match Prop.get_resource_attribute prop e with + | Some (Sil.Aresource ({Sil.ra_vpath = Some (Sil.Dpvar pvar) })) -> Some pvar + | _ -> None in + let loc = State.get_loc () in + match Prop.get_observer_attribute prop e with + | Some Sil.Aobserver -> + (match pvar_opt with + | Some pvar -> + L.d_strln (" ERROR: Object " ^ (Sil.pvar_to_string pvar) ^ + " is being deallocated while still registered in a notification center"); + let desc = Localise.desc_registered_observer_being_deallocated pvar loc in + raise (Exceptions.Registered_observer_being_deallocated (desc, __POS__)) + | _ -> ()) + | _ -> () + let check_junk ?original_prop pname tenv prop = let fav_sub_sigmafp = Sil.fav_new () in Sil.sub_fav_add fav_sub_sigmafp (Prop.get_sub prop); @@ -1070,6 +1086,7 @@ let check_junk ?original_prop pname tenv prop = (* find the alloc attribute of one of the roots of hpred, if it exists *) let res = ref None in let do_entry e = + check_observer_is_unsubscribed_deallocation prop e; match Prop.get_resource_attribute prop e with | Some (Sil.Aresource ({ Sil.ra_kind = Sil.Racquire }) as a) -> L.d_str "ATTRIBUTE: "; Sil.d_exp (Sil.Const (Sil.Cattribute a)); L.d_ln (); diff --git a/infer/src/backend/exceptions.ml b/infer/src/backend/exceptions.ml index 9a5d7a935..011886a17 100644 --- a/infer/src/backend/exceptions.ml +++ b/infer/src/backend/exceptions.ml @@ -67,6 +67,7 @@ exception Pointer_size_mismatch of Localise.error_desc * ml_loc exception Precondition_not_found of Localise.error_desc * ml_loc exception Precondition_not_met of Localise.error_desc * ml_loc exception Retain_cycle of Prop.normal Prop.t * Sil.hpred * Localise.error_desc * ml_loc +exception Registered_observer_being_deallocated of Localise.error_desc * ml_loc exception Return_expression_required of Localise.error_desc * ml_loc exception Return_statement_missing of Localise.error_desc * ml_loc exception Return_value_ignored of Localise.error_desc * ml_loc @@ -234,6 +235,9 @@ let recognize_exception exn = | Retain_cycle (_, _, desc, ml_loc) -> (Localise.retain_cycle, desc, Some ml_loc, Exn_user, High, None, Prover) + | Registered_observer_being_deallocated (desc, ml_loc) -> + (Localise.registered_observer_being_deallocated, + desc, Some ml_loc, Exn_user, High, Some Kerror, Nocat) | Return_expression_required (desc, ml_loc) -> (Localise.return_expression_required, desc, Some ml_loc, Exn_user, Medium, None, Nocat) diff --git a/infer/src/backend/exceptions.mli b/infer/src/backend/exceptions.mli index a51b5e8e5..448cdf25a 100644 --- a/infer/src/backend/exceptions.mli +++ b/infer/src/backend/exceptions.mli @@ -66,6 +66,7 @@ exception Pointer_size_mismatch of Localise.error_desc * ml_loc exception Precondition_not_found of Localise.error_desc * ml_loc exception Precondition_not_met of Localise.error_desc * ml_loc exception Retain_cycle of Prop.normal Prop.t * Sil.hpred * Localise.error_desc * ml_loc +exception Registered_observer_being_deallocated of Localise.error_desc * ml_loc exception Return_expression_required of Localise.error_desc * ml_loc exception Return_statement_missing of Localise.error_desc * ml_loc exception Return_value_ignored of Localise.error_desc * ml_loc diff --git a/infer/src/backend/localise.ml b/infer/src/backend/localise.ml index 99639ff34..76afb9326 100644 --- a/infer/src/backend/localise.ml +++ b/infer/src/backend/localise.ml @@ -53,6 +53,7 @@ let pointer_size_mismatch = "POINTER_SIZE_MISMATCH" let precondition_not_found = "PRECONDITION_NOT_FOUND" let precondition_not_met = "PRECONDITION_NOT_MET" let premature_nil_termination = "PREMATURE_NIL_TERMINATION_ARGUMENT" +let registered_observer_being_deallocated = "REGISTERED_OBSERVER_BEING_DEALLOCATED" let resource_leak = "RESOURCE_LEAK" let retain_cycle = "RETAIN_CYCLE" let return_value_ignored = "RETURN_VALUE_IGNORED" @@ -734,6 +735,16 @@ let desc_retain_cycle prop cycle loc cycle_dotty = !str_cycle (at_line tags loc) in { no_desc with descriptions = [desc]; tags = !tags; dotty = cycle_dotty } +let desc_registered_observer_being_deallocated pvar loc = + let tags = Tags.create () in + let obj_str = Sil.pvar_to_string pvar in + { no_desc with descriptions = ["Object " ^ obj_str ^ + " has not been unregistered from a notification " ^ + "center and is being deallocated " ^ at_line tags loc ^ + ". Being still registered as observer of the notification " ^ + "center, the deallocated object " + ^ obj_str ^ " may be notified in the future." ]; tags = !tags } + let desc_return_statement_missing loc = let tags = Tags.create () in { no_desc with descriptions = ["Return statement missing " ^ at_line tags loc]; tags = !tags } diff --git a/infer/src/backend/localise.mli b/infer/src/backend/localise.mli index becc7c99e..f11ee3f96 100644 --- a/infer/src/backend/localise.mli +++ b/infer/src/backend/localise.mli @@ -51,6 +51,7 @@ val pointer_size_mismatch : t val precondition_not_found : t val precondition_not_met : t val premature_nil_termination : t +val registered_observer_being_deallocated : t val retain_cycle : t val resource_leak : t val return_value_ignored : t @@ -227,6 +228,8 @@ val desc_retain_cycle : Prop.normal Prop.t -> ((Sil.strexp * Sil.typ) * Ident.fieldname * Sil.strexp) list -> Location.t -> string option -> error_desc +val desc_registered_observer_being_deallocated : Sil.pvar -> Location.t -> error_desc + val desc_return_statement_missing : Location.t -> error_desc val desc_return_value_ignored : Procname.t -> Location.t -> error_desc diff --git a/infer/src/backend/prop.ml b/infer/src/backend/prop.ml index 247563889..eed8ae059 100644 --- a/infer/src/backend/prop.ml +++ b/infer/src/backend/prop.ml @@ -1788,6 +1788,9 @@ let get_objc_null_attribute prop exp = let get_div0_attribute prop exp = get_attribute prop exp Sil.ACdiv0 +let get_observer_attribute prop exp = + get_attribute prop exp Sil.ACobserver + let has_dangling_uninit_attribute prop exp = let la = get_exp_attributes prop exp in IList.exists (fun a -> Sil.attribute_equal a (Sil.Adangling (Sil.DAuninit))) la diff --git a/infer/src/backend/prop.mli b/infer/src/backend/prop.mli index 54c9bd1b3..6a6c90948 100644 --- a/infer/src/backend/prop.mli +++ b/infer/src/backend/prop.mli @@ -289,6 +289,9 @@ val get_autorelease_attribute : 'a t -> exp -> attribute option (** Get the div0 attribute associated to the expression, if any *) val get_div0_attribute : 'a t -> exp -> attribute option +(** Get the observer attribute associated to the expression, if any *) +val get_observer_attribute : 'a t -> exp -> attribute option + (** Get the objc null attribute associated to the expression, if any *) val get_objc_null_attribute : 'a t -> exp -> attribute option diff --git a/infer/tests/codetoanalyze/objc/errors/registered_observer/ViewController.m b/infer/tests/codetoanalyze/objc/errors/registered_observer/ViewController.m new file mode 100644 index 000000000..231a06317 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/errors/registered_observer/ViewController.m @@ -0,0 +1,88 @@ +/* + * 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 +@interface ViewController : NSViewController +@end + +@implementation ViewController + +- (instancetype)init { + + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(fired) + name:@"some_notification" + object:nil]; + return self; +} + +- (void)fired { + NSLog(@"This codepath fired"); +} + +- (void)invalidate1 { + [[NSNotificationCenter defaultCenter] removeObserver:self]; +} + +- (void)invalidate2 { + [[NSNotificationCenter defaultCenter] removeObserver:self + name:@"some_notification" + object:nil]; +} +@end + +int fooError() { + ViewController* vc = [[ViewController alloc] init]; + ViewController* vc2; + vc2 = vc; + [vc fired]; + return 0; +} + +int fooOK1() { + ViewController* vc = [[ViewController alloc] init]; + ViewController* vc2; + vc2 = vc; + [vc fired]; + [vc invalidate1]; + return 0; +} + +int fooOK2() { + ViewController* vc = [[ViewController alloc] init]; + ViewController* vc2; + vc2 = vc; + [vc fired]; + [vc invalidate2]; + return 0; +} + +int barError() { + ViewController* vc = [[ViewController alloc] init]; + [vc fired]; + + vc = [[ViewController alloc] init]; + return 0; +} + +int barOK1() { + ViewController* vc = [[ViewController alloc] init]; + [vc invalidate1]; + vc = [[ViewController alloc] init]; + [vc invalidate1]; + return 0; +} + +int barOK2() { + ViewController* vc = [[ViewController alloc] init]; + [vc invalidate2]; + vc = [[ViewController alloc] init]; + [vc invalidate1]; + return 0; +} diff --git a/infer/tests/endtoend/objc/RegisteredObserver.java b/infer/tests/endtoend/objc/RegisteredObserver.java new file mode 100644 index 000000000..330195d6c --- /dev/null +++ b/infer/tests/endtoend/objc/RegisteredObserver.java @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2015 - 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; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsExactly.containsExactly; + +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 RegisteredObserver { + + public static final String VCFile = + "infer/tests/codetoanalyze/objc/errors/registered_observer/ViewController.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); + String[] methods = { + "fooError", "barError" + }; + assertThat( + "Results should contain " + REGISTERED_OBSERVER, + inferResults, + containsExactly( + REGISTERED_OBSERVER, + VCFile, + methods + ) + ); + } + +} diff --git a/infer/tests/utils/InferResults.java b/infer/tests/utils/InferResults.java index 065f13039..b48e94df3 100644 --- a/infer/tests/utils/InferResults.java +++ b/infer/tests/utils/InferResults.java @@ -61,6 +61,7 @@ public class InferResults { errorType.equals("STRONG_DELEGATE_WARNING") || errorType.equals("DIRECT_ATOMIC_PROPERTY_ACCESS") || errorType.equals("CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK") || + errorType.equals("REGISTERED_OBSERVER_BEING_DEALLOCATED") || errorType.equals("IMMUTABLE_CAST") || errorType.equals("PARAMETER_NOT_NULL_CHECKED") || errorType.equals("DANGLING_POINTER_DEREFERENCE") || diff --git a/infer/tests/utils/InferRunner.java b/infer/tests/utils/InferRunner.java index 4595428e0..344123e86 100644 --- a/infer/tests/utils/InferRunner.java +++ b/infer/tests/utils/InferRunner.java @@ -393,6 +393,20 @@ public class InferRunner { false); } + public static ImmutableList createObjCInferCommandSimple( + TemporaryFolder folder, + String sourceFile, + String ml_bucket) throws IOException, InterruptedException { + return createClangInferCommand( + folder, + sourceFile, + Language.ObjC, + true, + null, + ml_bucket, + false); + } + public static ImmutableList createObjCInferCommandWithMLBuckets( TemporaryFolder folder, String sourceFile,