[SelfInBlock] Reporting strongSelf Not Checked only in field access

Summary: After receiving feedback about this, I'm changing the reporting of strongSelf Not Checked to only in cases where it can cause a crash. Here I'm adding reporting for field access, and removing general reporting. In a next diff, I'll also add reporting for passing strongSelf to methods in not explicitly nullable positions.

Reviewed By: skcho

Differential Revision: D19329842

fbshipit-source-id: 35beb2aa3

infer/src/checkers/SelfInBlock.ml

@@ -171,8 +171,64 @@ module TransferFunctions = struct
     with Not_found_s _ | Caml.Not_found -> astate
 
 
+  let make_trace_unchecked_strongself (domain : Domain.t) =
+    let trace_elems_strongVars =
+      StrongEqualToWeakCapturedVars.fold
+        (fun pvar {loc} trace_elems ->
+          let trace_elem_desc = F.asprintf "%a assigned here" (Pvar.pp Pp.text) pvar in
+          let trace_elem = Errlog.make_trace_element 0 loc trace_elem_desc [] in
+          trace_elem :: trace_elems )
+        domain.strongVars []
+    in
+    let trace_elems_vars =
+      Vars.fold
+        (fun _ {pvar; loc; kind} trace_elems ->
+          match kind with
+          | UNCHECKED_STRONG_SELF ->
+              let trace_elem_desc =
+                F.asprintf "Using %a not checked for null" (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.vars []
+    in
+    let trace_elems = List.append trace_elems_strongVars trace_elems_vars in
+    List.sort trace_elems ~compare:(fun {Errlog.lt_loc= loc1} {Errlog.lt_loc= loc2} ->
+        Location.compare loc1 loc2 )
+
+
+  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
+        let message =
+          F.asprintf
+            "The variable `%a`, equal to a weak pointer to `self`, is %s without a check for null \
+             at %a. This could cause a crash or unexpected behavior."
+            (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 Not_found_s _ | Caml.Not_found -> ()
+
+
+  let report_unchecked_strongself_issues_on_exps (domain : Domain.t) summary (instr : Sil.instr) =
+    let report_unchecked_strongself_issues_on_exp (exp : Exp.t) =
+      match exp with
+      | Lfield (Var var, _, _) ->
+          report_unchecked_strongself_issues summary domain "dereferenced" var
+      | _ ->
+          ()
+    in
+    let exps = Sil.exps_of_instr instr in
+    List.iter ~f:report_unchecked_strongself_issues_on_exp exps
+
+
   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 ;
     match instr with
     | Load {id; e= Lvar pvar; loc; typ} ->
         let vars =
@@ -272,53 +328,6 @@ let report_weakself_multiple_issues summary domain =
       ()
 
 
-let make_trace_unchecked_strongself (domain : Domain.t) =
-  let trace_elems_strongVars =
-    StrongEqualToWeakCapturedVars.fold
-      (fun pvar {loc} trace_elems ->
-        let trace_elem_desc = F.asprintf "%a assigned here" (Pvar.pp Pp.text) pvar in
-        let trace_elem = Errlog.make_trace_element 0 loc trace_elem_desc [] in
-        trace_elem :: trace_elems )
-      domain.strongVars []
-  in
-  let trace_elems_vars =
-    Vars.fold
-      (fun _ {pvar; loc; kind} trace_elems ->
-        match kind with
-        | UNCHECKED_STRONG_SELF ->
-            let trace_elem_desc =
-              F.asprintf "Using %a not checked for null" (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.vars []
-  in
-  let trace_elems = List.append trace_elems_strongVars trace_elems_vars in
-  List.sort trace_elems ~compare:(fun {Errlog.lt_loc= loc1} {Errlog.lt_loc= loc2} ->
-      Location.compare loc1 loc2 )
-
-
-let report_unchecked_strongself_issues summary (domain : Domain.t) =
-  let unchecked_strongSelf_opt =
-    Vars.filter (fun _ {kind} -> DomainData.is_unchecked_strong_self kind) domain.vars
-    |> Vars.choose_opt
-  in
-  match unchecked_strongSelf_opt with
-  | Some (_, {pvar; loc}) ->
-      let message =
-        F.asprintf
-          "The variable `%a`, equal to a weak pointer to `self`, is used without a check for null \
-           at %a. This could cause a crash or unexpected behavior."
-          (Pvar.pp Pp.text) pvar Location.pp loc
-      in
-      let ltr = make_trace_unchecked_strongself domain in
-      Reporting.log_error summary ~ltr ~loc IssueType.strong_self_not_checked message
-  | _ ->
-      ()
-
-
 module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions)
 
 let checker {Callbacks.exe_env; summary} =
@@ -330,7 +339,6 @@ 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_unchecked_strongself_issues summary domain ;
         report_weakself_multiple_issues summary domain.vars
     | None ->
         () ) ;

infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m

@@ -46,7 +46,7 @@
   };
 }
 
-- (void)strongSelfNoCheck_bad {
+- (void)strongSelfNoCheck_good {
   __weak __typeof(self) weakSelf = self;
   int (^my_block)(BOOL) = ^(BOOL isTapped) {
     __strong __typeof(weakSelf) strongSelf = weakSelf;
@@ -57,7 +57,7 @@
 
 // very unlikely pattern, but still complies with invariant:
 // any use of strongSelf is bad unless checked for null beforehand
-- (void)strongSelfNoCheck2_bad {
+- (void)strongSelfNoCheck2_good {
   __weak __typeof(self) weakSelf = self;
   int (^my_block)() = ^() {
     __strong __typeof(weakSelf) strongSelf = weakSelf;
@@ -74,7 +74,7 @@
       [strongSelf foo];
       int x = strongSelf->x;
     } else {
-      [strongSelf foo];
+      strongSelf->x;
     }
     [strongSelf foo];
     if (strongSelf != nil) {

infer/tests/codetoanalyze/objc/self-in-block/issues.exp

@@ -1,6 +1,4 @@
 codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 5, 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::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]
-codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheck2_bad_4, 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::strongSelfNoCheck_bad_3, 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::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::wekSelfMultiple_bad_9, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf]