From 7fd1149f85081a648bd442afd733c8ed28a9971c Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Fri, 22 Jul 2016 09:16:08 -0700 Subject: [PATCH] Add NPE check for weak variables captured in blocks Reviewed By: sblackshear Differential Revision: D3593746 fbshipit-source-id: bc5dea0 --- infer/src/backend/localise.ml | 19 +++++- infer/src/backend/localise.mli | 3 + infer/src/backend/rearrange.ml | 23 ++++++- .../objc/errors/npe/WeakCapturedVarsNPE.m | 47 ++++++++++++++ .../infer/WeakVariableInBlockNPETest.java | 65 +++++++++++++++++++ 5 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 infer/tests/codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m create mode 100644 infer/tests/endtoend/objc/infer/WeakVariableInBlockNPETest.java diff --git a/infer/src/backend/localise.ml b/infer/src/backend/localise.ml index a2f9739fc..d59089a2a 100644 --- a/infer/src/backend/localise.ml +++ b/infer/src/backend/localise.ml @@ -136,6 +136,7 @@ module Tags = struct let parameter_not_null_checked = "parameter_not_null_checked" (* describes a NPE that comes from parameter not nullable *) let field_not_null_checked = "field_not_null_checked" (* describes a NPE that comes from field not nullable *) let nullable_src = "nullable_src" (* @Nullable-annoted field/param/retval that causes a warning *) + let weak_captured_var_src = "weak_captured_var_src" (* Weak variable captured in a block that causes a warning *) let empty_vector_access = "empty_vector_access" let create () = ref [] let add tags tag value = tags := (tag, value) :: !tags @@ -307,6 +308,13 @@ let deref_str_nullable proc_name_opt nullable_obj_str = let problem_str = "" in _deref_str_null proc_name_opt problem_str tags +(** dereference strings for null dereference due to weak captured variable in block *) +let deref_str_weak_variable_in_block proc_name_opt nullable_obj_str = + let tags = Tags.create () in + Tags.add tags Tags.weak_captured_var_src nullable_obj_str; + let problem_str = "" in + _deref_str_null proc_name_opt problem_str tags + (** dereference strings for nonterminal nil arguments in c/objc variadic methods *) let deref_str_nil_argument_in_variadic_method pn total_args arg_number = let tags = Tags.create () in @@ -517,11 +525,16 @@ let dereference_string deref_str value_str access_opt loc = let problem_desc = let nullable_text = if !Config.curr_language = Config.Java then "@Nullable" else "__nullable" in let problem_str = - match Tags.get !tags Tags.nullable_src with - | Some nullable_src -> + match Tags.get !tags Tags.nullable_src, Tags.get !tags Tags.weak_captured_var_src with + | Some nullable_src, _ -> if nullable_src = value_str then "is annotated with " ^ nullable_text ^ " and is dereferenced without a null check" else "is indirectly marked " ^ nullable_text ^ " (source: " ^ nullable_src ^ ") and is dereferenced without a null check" - | None -> deref_str.problem_str in + | None, Some weak_var_str -> + if weak_var_str = value_str then + "is a weak pointer captured in the block and is dereferenced without a null check" + else "is equal to the variable " ^ weak_var_str ^ + ", a weak pointer captured in the block, and is dereferenced without a null check" + | None, None -> deref_str.problem_str in [(problem_str ^ " " ^ at_line tags loc)] in { no_desc with descriptions = value_desc:: access_desc @ problem_desc; tags = !tags } diff --git a/infer/src/backend/localise.mli b/infer/src/backend/localise.mli index e31e57560..f15f84df9 100644 --- a/infer/src/backend/localise.mli +++ b/infer/src/backend/localise.mli @@ -146,6 +146,9 @@ val deref_str_null : Procname.t option -> deref_str (** dereference strings for null dereference due to Nullable annotation *) val deref_str_nullable : Procname.t option -> string -> deref_str +(** dereference strings for null dereference due to weak captured variable in block *) +val deref_str_weak_variable_in_block : Procname.t option -> string -> deref_str + (** dereference strings for an undefined value coming from the given procedure *) val deref_str_undef : Procname.t * Location.t -> deref_str diff --git a/infer/src/backend/rearrange.ml b/infer/src/backend/rearrange.ml index d516ef2a0..cde56f2f9 100644 --- a/infer/src/backend/rearrange.ml +++ b/infer/src/backend/rearrange.ml @@ -1156,9 +1156,23 @@ let rec iter_rearrange end; res +let is_weak_captured_var pdesc pvar = + let pname = Cfg.Procdesc.get_proc_name pdesc in + match pname with + | Block _ -> + let is_weak_captured (var, typ) = + match typ with + | Typ.Tptr (_, Pk_objc_weak) -> + Mangled.equal (Pvar.get_name pvar) var + | _ -> false in + IList.exists is_weak_captured (Cfg.Procdesc.get_captured pdesc) + | _ -> false + + (** Check for dereference errors: dereferencing 0, a freed value, or an undefined value *) let check_dereference_error pdesc (prop : Prop.normal Prop.t) lexp loc = let nullable_obj_str = ref None in + let nullable_str_is_weak_captured_var = ref false in (* return true if deref_exp is only pointed to by fields/params with @Nullable annotations *) let is_only_pt_by_nullable_fld_or_param deref_exp = let ann_sig = Models.get_modelled_annotated_signature (Specs.pdesc_resolve_attributes pdesc) in @@ -1167,11 +1181,13 @@ let check_dereference_error pdesc (prop : Prop.normal Prop.t) lexp loc = match hpred with | Sil.Hpointsto (Sil.Lvar pvar, Sil.Eexp (Sil.Var _ as exp, _), _) when Sil.exp_equal exp deref_exp -> + let is_weak_captured_var = is_weak_captured_var pdesc pvar in let is_nullable = - if Annotations.param_is_nullable pvar ann_sig + if Annotations.param_is_nullable pvar ann_sig || is_weak_captured_var then begin nullable_obj_str := Some (Pvar.to_string pvar); + nullable_str_is_weak_captured_var := is_weak_captured_var; true end else @@ -1230,7 +1246,10 @@ let check_dereference_error pdesc (prop : Prop.normal Prop.t) lexp loc = let deref_str = if is_deref_of_nullable then match !nullable_obj_str with - | Some str -> Localise.deref_str_nullable None str + | Some str -> + if !nullable_str_is_weak_captured_var then + Localise.deref_str_weak_variable_in_block None str + else Localise.deref_str_nullable None str | None -> Localise.deref_str_nullable None "" else Localise.deref_str_null None in let err_desc = diff --git a/infer/tests/codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m b/infer/tests/codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m new file mode 100644 index 000000000..981714032 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m @@ -0,0 +1,47 @@ +/* + * 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. + */ + +#include + +@interface A : NSObject + +@end + +@implementation A { + int x; +} + +- (void)strongSelfNoCheckNotWeakSelf { + __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + __strong __typeof(weakSelf) strongSelf = weakSelf; + return strongSelf->x; + }; +} + +- (void)strongSelfNoCheck { + __weak __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + __strong __typeof(weakSelf) strongSelf = weakSelf; + return strongSelf->x; + }; +} + +- (void)strongSelfCheck { + __weak __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + __strong __typeof(weakSelf) strongSelf = weakSelf; + if (strongSelf) + return strongSelf->x; + else + return 0; + }; +} + +@end diff --git a/infer/tests/endtoend/objc/infer/WeakVariableInBlockNPETest.java b/infer/tests/endtoend/objc/infer/WeakVariableInBlockNPETest.java new file mode 100644 index 000000000..042cba608 --- /dev/null +++ b/infer/tests/endtoend/objc/infer/WeakVariableInBlockNPETest.java @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2013 - 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.infer; + +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 WeakVariableInBlockNPETest { + + public static final String FILE = + "infer/tests/codetoanalyze/objc/errors/npe/WeakCapturedVarsNPE.m"; + + private static ImmutableList inferCmdNPD; + + public static final String NULL_DEREFERENCE = "NULL_DEREFERENCE"; + + @ClassRule + public static DebuggableTemporaryFolder folderNPD = + new DebuggableTemporaryFolder(); + + @BeforeClass + public static void runInfer() throws InterruptedException, IOException { + inferCmdNPD = InferRunner.createiOSInferCommandWithMLBuckets( + folderNPD, + FILE, + "cf", + true); + } + + @Test + public void whenInferRunsOnWeakCapturedVariablesNPEThenNPEsFound() + throws InterruptedException, IOException, InferException { + InferResults inferResults = InferRunner.runInferC(inferCmdNPD); + String[] procedures = {"__objc_anonymous_block_A_strongSelfNoCheck______2"}; + assertThat( + "Results should not contain null dereference", + inferResults, + containsExactly( + NULL_DEREFERENCE, + FILE, + procedures + ) + ); + } +}