From f6341977fbf10c231de04fa6b637b2cc9376b0ed Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Mon, 17 Feb 2020 08:38:34 -0800 Subject: [PATCH] [Infer] Dedup reports of strongSelf Not checked Summary: When we discovered that a strongSelf var was not checked for null, we then report in each occurrence which is spammy. Now we report only the first occurrence. To achieve that, we store a `reported` flag in the domain that gets set to true after we report once, and we only report if it's false. Reviewed By: jvillard Differential Revision: D19877218 fbshipit-source-id: c44109ae9 --- infer/src/checkers/SelfInBlock.ml | 92 ++++++++++++------- .../objc/self-in-block/StrongSelf.m | 51 +++++++--- .../objc/self-in-block/issues.exp | 10 +- 3 files changed, 99 insertions(+), 54 deletions(-) diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 97bb8c002..20350dade 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -90,16 +90,20 @@ module PPPVar = struct end module CheckedForNull = struct - type t = {checked: bool; loc: Location.t} + type t = {checked: bool; loc: Location.t; reported: bool} - let pp fmt {checked} = + let pp fmt {checked; reported} = let s = match checked with true -> "Checked" | false -> "NotChecked" in - F.fprintf fmt "%s" s + let s' = match reported with true -> "Reported" | false -> "NotReported" in + F.fprintf fmt "%s, %s" s s' - let join {checked= elem1; loc= loc1} {checked= elem2; loc= loc2} = - let loc = match (elem1, elem2) with true, false -> loc2 | false, _ | true, true -> loc1 in - {checked= AbstractDomain.BooleanAnd.join elem1 elem2; loc} + let join {checked= elem1; loc= loc1; reported= reported1} + {checked= elem2; loc= loc2; reported= reported2} = + let loc, reported = + match (elem1, elem2) with true, false -> (loc2, reported2) | _ -> (loc1, reported1) + in + {checked= elem1 && elem2; loc; reported} let widen ~prev ~next ~num_iters:_ = join prev next @@ -165,13 +169,19 @@ module TransferFunctions = struct attributes.ProcAttributes.captured + let find_strong_var (domain : Domain.t) id = + match Vars.find_opt id domain.vars with + | Some elem when StrongEqualToWeakCapturedVars.mem elem.pvar domain.strongVars -> + Some (elem, elem.pvar, StrongEqualToWeakCapturedVars.find elem.pvar domain.strongVars) + | _ -> + None + + let exec_null_check_id (astate : Domain.t) id = - try - let elem = Vars.find id astate.vars in - if StrongEqualToWeakCapturedVars.mem elem.pvar astate.strongVars then - let strongVarElem = StrongEqualToWeakCapturedVars.find elem.pvar astate.strongVars in + match find_strong_var astate id with + | Some (elem, pvar, strongVarElem) -> let strongVars = - StrongEqualToWeakCapturedVars.add elem.pvar + StrongEqualToWeakCapturedVars.add pvar {strongVarElem with checked= true} astate.strongVars in @@ -181,8 +191,8 @@ module TransferFunctions = struct Vars.add id {elem with kind= CHECKED_STRONG_SELF} astate.vars in {Domain.vars; strongVars} - else astate - with Caml.Not_found -> astate + | None -> + astate let make_trace_unchecked_strongself (domain : Domain.t) = @@ -214,9 +224,9 @@ module TransferFunctions = struct let report_unchecked_strongself_issues summary (domain : Domain.t) var_use var = - try - let {DomainData.pvar; loc; kind} = Vars.find var domain.vars in - if DomainData.is_unchecked_strong_self kind then + match find_strong_var domain var with + | Some ({DomainData.pvar; loc; kind}, _, strongVarElem) + when DomainData.is_unchecked_strong_self kind && not strongVarElem.reported -> let message = F.asprintf "The variable `%a`, equal to a weak pointer to `self`, is %s without a check for null \ @@ -224,20 +234,27 @@ module TransferFunctions = struct (Pvar.pp Pp.text) pvar var_use Location.pp loc in let ltr = make_trace_unchecked_strongself domain in - Reporting.log_error summary ~ltr ~loc IssueType.strong_self_not_checked message - with Caml.Not_found -> () + Reporting.log_error summary ~ltr ~loc IssueType.strong_self_not_checked message ; + let strongVars = + StrongEqualToWeakCapturedVars.add pvar + {strongVarElem with reported= true} + domain.strongVars + in + {domain with strongVars} + | _ -> + domain let report_unchecked_strongself_issues_on_exps (domain : Domain.t) summary (instr : Sil.instr) = - let report_unchecked_strongself_issues_on_exp (exp : Exp.t) = + let report_unchecked_strongself_issues_on_exp strongVars (exp : Exp.t) = match exp with | Lfield (Var var, _, _) -> report_unchecked_strongself_issues summary domain "dereferenced" var | _ -> - () + strongVars in let exps = Sil.exps_of_instr instr in - List.iter ~f:report_unchecked_strongself_issues_on_exp exps + List.fold ~f:report_unchecked_strongself_issues_on_exp exps ~init:domain (* The translation of closures includes a load instruction for the captured variable, @@ -288,20 +305,22 @@ module TransferFunctions = struct (F.sprintf "passed to `%s`" (Procname.to_simplified_string pname)) var in - let rec report_on_non_nullable_arg ?annotations args = + let rec report_on_non_nullable_arg ?annotations domain args = match (annotations, args) with | Some (annot :: annot_rest), (arg, _) :: rest -> - ( match arg with - | Exp.Var var when not (Annotations.ia_is_nullable annot) -> - report_issue var - | _ -> - () ) ; - report_on_non_nullable_arg ~annotations:annot_rest rest + let domain = + match arg with + | Exp.Var var when not (Annotations.ia_is_nullable annot) -> + report_issue var + | _ -> + domain + in + report_on_non_nullable_arg ~annotations:annot_rest domain rest | None, (arg, _) :: rest -> - (match arg with Exp.Var var -> report_issue var | _ -> ()) ; - report_on_non_nullable_arg rest + let domain = match arg with Exp.Var var -> report_issue var | _ -> domain in + report_on_non_nullable_arg domain rest | _ -> - () + domain in let proc_desc_opt = Ondemand.get_proc_desc pname in let annotations = get_annotations proc_desc_opt in @@ -314,12 +333,12 @@ module TransferFunctions = struct match annotations with Some (_ :: rest) -> Some rest | _ -> annotations else annotations in - report_on_non_nullable_arg ?annotations args + report_on_non_nullable_arg ?annotations domain args let exec_instr (astate : Domain.t) {ProcData.summary} _cfg_node (instr : Sil.instr) = let attributes = Summary.get_attributes summary in - report_unchecked_strongself_issues_on_exps astate summary instr ; + let astate = report_unchecked_strongself_issues_on_exps astate summary instr in let astate = remove_ids_in_closures_from_domain astate instr in match instr with | Load {id; e= Lvar pvar; loc; typ} -> @@ -344,7 +363,10 @@ module TransferFunctions = struct try let {DomainData.pvar= binding_for_id} = Vars.find id astate.vars in if is_captured_weak_self attributes binding_for_id && Typ.is_strong_pointer pvar_typ - then StrongEqualToWeakCapturedVars.add pvar {checked= false; loc} astate.strongVars + then + StrongEqualToWeakCapturedVars.add pvar + {checked= false; loc; reported= false} + astate.strongVars else astate.strongVars with Caml.Not_found -> astate.strongVars in @@ -357,7 +379,7 @@ module TransferFunctions = struct | Prune (UnOp (LNot, BinOp (Binop.Eq, Var id, e), _), _, _, _) -> if Exp.is_null_literal e then exec_null_check_id astate id else astate | Call (_, Exp.Const (Const.Cfun callee_pn), args, _, _) -> - report_unchecked_strongself_issues_on_args astate summary callee_pn args ; + let astate = report_unchecked_strongself_issues_on_args astate summary callee_pn args in astate | _ -> astate diff --git a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m index df7bfa125..28a24dc47 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m +++ b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m @@ -98,23 +98,36 @@ void m2(_Nullable SelfInBlockTest* obj) {} } else { strongSelf->x; // bug here [strongSelf foo]; // no bug here - m(strongSelf); // bug here - m2(strongSelf); // no bug here because of _Nullable annotation - [strongSelf.user use_self_in_block_test:strongSelf]; // bug here - [strongSelf.user - use_self_in_block_test_nullable:1 - and:strongSelf]; // no bug here because of - // _Nullable annotation + m(strongSelf); // no bug here because of dedup } - [strongSelf foo]; - if (strongSelf != nil) { + return 0; + }; +} + +- (void)strongSelfCheck2_bad { + __weak __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + __strong __typeof(weakSelf) strongSelf = weakSelf; + if (strongSelf) { [strongSelf foo]; int x = strongSelf->x; + } else { + m(strongSelf); // bug here + int x = strongSelf->x; // no bug here because of dedup } return 0; }; } +- (void)strongSelfCheck6_good { + __weak __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + __strong __typeof(weakSelf) strongSelf = weakSelf; + m2(strongSelf); // no bug here because of _Nullable annotation + return 0; + }; +} + - (void)strongSelfCheck2_good { __weak __typeof(self) weakSelf = self; int (^my_block)(BOOL) = ^(BOOL isTapped) { @@ -140,13 +153,23 @@ void m2(_Nullable SelfInBlockTest* obj) {} }; } -- (void)strongSelfCheck4_good { +- (void)strongSelfCheck4_bad { __weak __typeof(self) weakSelf = self; int (^my_block)() = ^() { __strong __typeof(weakSelf) strongSelf = weakSelf; - if (!strongSelf) { - } else { - } + [strongSelf.user use_self_in_block_test:strongSelf]; // bug here + return 0; + }; +} + +- (void)strongSelfCheck5_good { + __weak __typeof(self) weakSelf = self; + int (^my_block)() = ^() { + __strong __typeof(weakSelf) strongSelf = weakSelf; + [strongSelf.user + use_self_in_block_test_nullable:1 + and:strongSelf]; // no bug here because of + // _Nullable annotation return 0; }; } @@ -162,7 +185,7 @@ void m2(_Nullable SelfInBlockTest* obj) {} }; } -- (void)strongSelfCheck6_good { +- (void)strongSelfCheck7_good { __weak __typeof(self) weakSelf = self; int (^my_block)() = ^() { __strong __typeof(weakSelf) strongSelf = weakSelf; diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp index 83bebfde8..ca77ecd63 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -1,9 +1,9 @@ codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_blockA::weak_in_noescape_block_bad:_1, 1, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 4, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self] -codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_7, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck2_bad_6, 6, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_9, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck4_bad_10, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 6, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null] -codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 8, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null] -codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 10, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null,Using &strongSelf not checked for null] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheckNotWeakSelf_good_2, 1, CAPTURED_STRONG_SELF, no_bucket, ERROR, [Using captured &weakSelf] -codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::weakSelfMultiple_bad_11, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] -codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockobjc_blockSelfInBlockTest::capturedStrongSelf_bad_12_13, 1, CAPTURED_STRONG_SELF, no_bucket, ERROR, [Using captured &strongSelf] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::weakSelfMultiple_bad_14, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockobjc_blockSelfInBlockTest::capturedStrongSelf_bad_15_16, 1, CAPTURED_STRONG_SELF, no_bucket, ERROR, [Using captured &strongSelf]