diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index b320d841a..463a7d349 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -363,6 +363,7 @@ OPTIONS BUFFER_OVERRUN_S2 (enabled by default), BUFFER_OVERRUN_U5 (disabled by default), Bad_footprint (enabled by default), + CAPTURED_STRONG_SELF (enabled by default), CHECKERS_ALLOCATES_MEMORY (enabled by default), CHECKERS_ANNOTATION_REACHABILITY_ERROR (enabled by default), CHECKERS_CALLS_EXPENSIVE_METHOD (enabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 1d9aefb55..cd983af7c 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -97,6 +97,7 @@ OPTIONS BUFFER_OVERRUN_S2 (enabled by default), BUFFER_OVERRUN_U5 (disabled by default), Bad_footprint (enabled by default), + CAPTURED_STRONG_SELF (enabled by default), CHECKERS_ALLOCATES_MEMORY (enabled by default), CHECKERS_ANNOTATION_REACHABILITY_ERROR (enabled by default), CHECKERS_CALLS_EXPENSIVE_METHOD (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index 1a451e050..b44f021a1 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -363,6 +363,7 @@ OPTIONS BUFFER_OVERRUN_S2 (enabled by default), BUFFER_OVERRUN_U5 (disabled by default), Bad_footprint (enabled by default), + CAPTURED_STRONG_SELF (enabled by default), CHECKERS_ALLOCATES_MEMORY (enabled by default), CHECKERS_ANNOTATION_REACHABILITY_ERROR (enabled by default), CHECKERS_CALLS_EXPENSIVE_METHOD (enabled by default), diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 742ce2d3c..64d918ed1 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -141,6 +141,8 @@ let buffer_overrun_u5 = register_from_string ~enabled:false "BUFFER_OVERRUN_U5" let cannot_star = register_from_string "Cannot_star" +let captured_strong_self = register_from_string "CAPTURED_STRONG_SELF" ~hum:"Captured strongSelf" + let checkers_allocates_memory = register_from_string "CHECKERS_ALLOCATES_MEMORY" ~hum:"Allocates Memory" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index ace92c273..b7a21035a 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -70,6 +70,8 @@ val buffer_overrun_u5 : t val cannot_star : t +val captured_strong_self : t + val checkers_allocates_memory : t (** Warning name when a performance critical method directly or indirectly calls a method allocating memory *) diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 84e0f1674..3926aca90 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -8,7 +8,12 @@ open! IStd module F = Format module DomainData = struct - type self_pointer_kind = SELF | UNCHECKED_STRONG_SELF | CHECKED_STRONG_SELF | WEAK_SELF + type self_pointer_kind = + | CAPTURED_STRONG_SELF + | CHECKED_STRONG_SELF + | SELF + | UNCHECKED_STRONG_SELF + | WEAK_SELF [@@deriving compare] let is_self kind = match kind with SELF -> true | _ -> false @@ -17,9 +22,13 @@ module DomainData = struct let is_unchecked_strong_self kind = match kind with UNCHECKED_STRONG_SELF -> true | _ -> false + let is_captured_strong_self kind = match kind with CAPTURED_STRONG_SELF -> true | _ -> false + let pp_self_pointer_kind fmt kind = let s = match kind with + | CAPTURED_STRONG_SELF -> + "CAPTURED_STRONG_SELF" | CHECKED_STRONG_SELF -> "CHECKED_STRONG_SELF" | SELF -> @@ -134,7 +143,7 @@ module TransferFunctions = struct let pp_session_name _node fmt = F.pp_print_string fmt "SelfCapturedInBlock" - let is_captured_strong_self attributes pvar = + let is_captured_self attributes pvar = let pvar_name = Pvar.get_name pvar in Pvar.is_self pvar && List.exists @@ -142,6 +151,17 @@ module TransferFunctions = struct attributes.ProcAttributes.captured + (* The variable is captured in the block, contains self in the name, is not self, and it's strong. *) + let is_captured_strong_self attributes pvar = + (not (Pvar.is_self pvar)) + && List.exists + ~f:(fun (captured, typ) -> + Typ.is_strong_pointer typ + && Mangled.equal captured (Pvar.get_name pvar) + && String.is_suffix ~suffix:"self" (String.lowercase (Mangled.to_string captured)) ) + attributes.ProcAttributes.captured + + let is_captured_weak_self attributes pvar = List.exists ~f:(fun (captured, typ) -> @@ -282,8 +302,10 @@ module TransferFunctions = struct match instr with | Load {id; e= Lvar pvar; loc; typ} -> let vars = - if is_captured_strong_self attributes pvar then + if is_captured_self attributes pvar then Vars.add id {pvar; typ; loc; kind= SELF} astate.vars + else if is_captured_strong_self attributes pvar then + Vars.add id {pvar; typ; loc; kind= CAPTURED_STRONG_SELF} astate.vars else if is_captured_weak_self attributes pvar then Vars.add id {pvar; typ; loc; kind= WEAK_SELF} astate.vars else @@ -381,6 +403,42 @@ let report_weakself_multiple_issues summary domain = () +let make_trace_captured_strong_self domain = + let trace_elems = + Vars.fold + (fun _ {pvar; loc; kind} trace_elems -> + match kind with + | CAPTURED_STRONG_SELF -> + let trace_elem_desc = F.asprintf "Using captured %a" (Pvar.pp Pp.text) pvar in + let trace_elem = Errlog.make_trace_element 0 loc trace_elem_desc [] in + trace_elem :: trace_elems + | _ -> + trace_elems ) + domain [] + in + List.sort trace_elems ~compare:(fun {Errlog.lt_loc= loc1} {Errlog.lt_loc= loc2} -> + Location.compare loc1 loc2 ) + + +let report_captured_strongself_issues summary domain = + let capturedStrongSelf_opt = + Vars.filter (fun _ {kind} -> DomainData.is_captured_strong_self kind) domain |> Vars.choose_opt + in + match capturedStrongSelf_opt with + | Some (_, {pvar; loc}) -> + let message = + F.asprintf + "The variable `%a`, used at `%a`, is a strong pointer to `self` captured in this block. \ + This could lead to retain cycles or unexpected behavior since to avoid retain cycles \ + one usually uses a local strong pointer or a captured weak pointer instead." + (Pvar.pp Pp.text) pvar Location.pp loc + in + let ltr = make_trace_captured_strong_self domain in + Reporting.log_error summary ~ltr ~loc IssueType.captured_strong_self message + | _ -> + () + + module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) let checker {Callbacks.exe_env; summary} = @@ -392,7 +450,8 @@ let checker {Callbacks.exe_env; summary} = match Analyzer.compute_post proc_data ~initial with | Some domain -> report_mix_self_weakself_issues summary domain.vars ; - report_weakself_multiple_issues summary domain.vars + report_weakself_multiple_issues summary domain.vars ; + report_captured_strongself_issues summary domain.vars | None -> () ) ; summary diff --git a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m index d7e8b2ef7..c82001e88 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m +++ b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m @@ -178,4 +178,20 @@ void m2(_Nullable SelfInBlockTest* obj) {} return 0; }; } + +- (void)capturedStrongSelf_bad { + __weak __typeof(self) weakSelf = self; + int (^my_block)() = ^() { + __strong typeof(self) strongSelf = weakSelf; + if (strongSelf) { + int (^my_block)() = ^() { + int x = strongSelf->x; // bug here + return 0; + }; + int x = strongSelf->x; + } + 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 c83f46791..da512043a 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -3,4 +3,6 @@ codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strong 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::wekSelfMultiple_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]