[selfInBlock] Add a new check when strongSelf is captured in a (sub)block.

Summary:
Adding a new check as part of the SelfInBlock checker, for cases when a strong pointer to self, that is not self, is captured in the block.
This will cover the following wrong scenarios:

1. strongSelf is a local variable in a block as it should be, and then it's used in a sub-block, in which case it's a captured variable.
2. weakSelf is defined without the weak attribute by mistake.

Reviewed By: ngorogiannis

Differential Revision: D19538036

fbshipit-source-id: 151871745

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),

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),

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),

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"
 

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 *)

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

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

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]