From 9dfd1943e617d3a309685c97881fcd4a7813bde0 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Wed, 29 Jan 2020 02:46:01 -0800 Subject: [PATCH] [selfInBlock] Remove false positive from mix self weakSelf Summary: The translation of closures includes a load instruction for the captured variable, then we add that corresponding id to the closure. This doesn't correspond to an actual "use" of the captured variable in the source program though, and causes false positives. Here we remove the ids from the domain when that id is being added to a closure. Reviewed By: ngorogiannis Differential Revision: D19557352 fbshipit-source-id: 52b426011 --- infer/src/checkers/SelfInBlock.ml | 21 +++++++++++++++++++ .../objc/self-in-block/StrongSelf.m | 13 +++++++++++- .../objc/self-in-block/issues.exp | 2 +- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 3926aca90..00e96d3c5 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -246,6 +246,26 @@ module TransferFunctions = struct List.iter ~f:report_unchecked_strongself_issues_on_exp exps + (* The translation of closures includes a load instruction for the captured variable, + then we add that corresponding id to the closure. This doesn't correspond to an + actual "use" of the captured variable in the source program though, and causes false + positives. Here we remove the ids from the domain when that id is being added to a closure. *) + let remove_ids_in_closures_from_domain (domain : Domain.t) (instr : Sil.instr) = + let remove_id_in_closures_from_domain vars ((exp : Exp.t), _, _) = + match exp with Var id -> Vars.remove id vars | _ -> vars + in + let do_exp vars (exp : Exp.t) = + match exp with + | Closure {captured_vars} -> + List.fold ~init:vars ~f:remove_id_in_closures_from_domain captured_vars + | _ -> + vars + in + let exps = Sil.exps_of_instr instr in + let vars = List.fold ~init:domain.vars ~f:do_exp exps in + {domain with vars} + + let is_objc_instance proc_desc_opt = match proc_desc_opt with | Some proc_desc -> ( @@ -299,6 +319,7 @@ module TransferFunctions = struct 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 = remove_ids_in_closures_from_domain astate instr in match instr with | Load {id; e= Lvar pvar; loc; typ} -> let vars = diff --git a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m index c82001e88..df7bfa125 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m +++ b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m @@ -170,7 +170,7 @@ void m2(_Nullable SelfInBlockTest* obj) {} }; } -- (void)wekSelfMultiple_bad { +- (void)weakSelfMultiple_bad { __weak __typeof(self) weakSelf = self; int (^my_block)(BOOL) = ^(BOOL isTapped) { [weakSelf foo]; @@ -194,4 +194,15 @@ void m2(_Nullable SelfInBlockTest* obj) {} }; } +- (void)mixSelfWeakSelf_good:(NSArray*)resources { + __weak __typeof(self) weakSelf = self; + int (^my_block)() = ^() { + [self foo]; // no bug here + int (^my_block)() = ^() { + [weakSelf foo]; + return 0; + }; + return 0; + }; +} @end diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp index da512043a..9a77243d5 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -4,5 +4,5 @@ codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strong 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::wekSelfMultiple_bad_11, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &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]