[thread-safety] don't warn on methods annotated with UiThread

Summary: These methods should only be called from other methods that also run on the UI thread, and they should not be starting new threads.

Reviewed By: peterogithub

Differential Revision: D4383133

fbshipit-source-id: 6cb2e40

infer/src/checkers/ThreadSafety.ml

@@ -23,38 +23,6 @@ module Summary = Summary.Make (struct
       payload.Specs.threadsafety
   end)
 
-(* we want to consider Builder classes and other safe immutablility-ensuring patterns as
-   thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder`
-   is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with
-   @Builder and verify that annotated classes satisfy the expected invariants. *)
-let is_builder_class class_name =
-  String.is_suffix ~suffix:"Builder" class_name
-
-(* similarly, we assume that immutable classes safely encapsulate their state *)
-let is_immutable_collection_class class_name tenv =
-  let immutable_collections = [
-    "com.google.common.collect.ImmutableCollection";
-    "com.google.common.collect.ImmutableMap";
-    "com.google.common.collect.ImmutableTable";
-  ] in
-  PatternMatch.supertype_exists
-    tenv (fun typename _ -> IList.mem (=) (Typename.name typename) immutable_collections) class_name
-
-let is_call_to_builder_class_method = function
-  | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname)
-  | _ -> false
-
-let is_call_to_immutable_collection_method tenv = function
-  | Procname.Java java_pname ->
-      is_immutable_collection_class (Procname.java_get_class_type_name java_pname) tenv
-  | _ ->
-      false
-
-let is_initializer tenv proc_name =
-  Procname.is_constructor proc_name ||
-  Procname.is_class_initializer proc_name ||
-  FbThreadSafety.is_custom_init tenv proc_name
-
 module TransferFunctions (CFG : ProcCfg.S) = struct
   module CFG = CFG
   module Domain = ThreadSafetyDomain
@@ -123,7 +91,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
 
   let exec_instr
       ({ ThreadSafetyDomain.locks; reads; writes; id_map; owned; } as astate)
-      { ProcData.pdesc; tenv; } _ =
+      { ProcData.pdesc; } _ =
 
     let is_allocation pn =
       Procname.equal pn BuiltinDecl.__new ||
@@ -157,11 +125,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
                     let astate' =
                       (* TODO (14842325): report on constructors that aren't threadsafe
                          (e.g., constructors that access static fields) *)
-                      if is_unprotected locks' &&
+                      if is_unprotected locks'
-                         not (is_initializer tenv pn) &&
-                         not (FbThreadSafety.is_logging_method pn) &&
-                         not (is_call_to_builder_class_method pn) &&
-                         not (is_call_to_immutable_collection_method tenv pn)
                       then
                         let call_site = CallSite.make pn loc in
                         let reads' =
@@ -249,13 +213,69 @@ module ResultsTableType = Caml.Map.Make (struct
     let compare (_, _, pn1, _) (_,_,pn2,_) =  Procname.compare pn1 pn2
   end)
 
-let should_report_on_proc (_,tenv,proc_name,proc_desc) =
+(* we want to consider Builder classes and other safe immutablility-ensuring patterns as
+   thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder`
+   is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with
+   @Builder and verify that annotated classes satisfy the expected invariants. *)
+let is_builder_class class_name =
+  String.is_suffix ~suffix:"Builder" class_name
+
+(* similarly, we assume that immutable classes safely encapsulate their state *)
+let is_immutable_collection_class class_name tenv =
+  let immutable_collections = [
+    "com.google.common.collect.ImmutableCollection";
+    "com.google.common.collect.ImmutableMap";
+    "com.google.common.collect.ImmutableTable";
+  ] in
+  PatternMatch.supertype_exists
+    tenv (fun typename _ -> IList.mem (=) (Typename.name typename) immutable_collections) class_name
+
+let is_call_to_builder_class_method = function
+  | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname)
+  | _ -> false
+
+let is_call_to_immutable_collection_method tenv = function
+  | Procname.Java java_pname ->
+      is_immutable_collection_class (Procname.java_get_class_type_name java_pname) tenv
+  | _ ->
+      false
+
+let is_initializer tenv proc_name =
+  Procname.is_constructor proc_name ||
+  Procname.is_class_initializer proc_name ||
+  FbThreadSafety.is_custom_init tenv proc_name
+
+(* we don't want to warn on methods that run on the UI thread because they should always be
+   single-threaded *)
+let runs_on_ui_thread proc_desc =
+  (* assume that methods named onClick always run on the UI thread *)
+  let is_on_click pdesc = match Procdesc.get_proc_name pdesc with
+    | Procname.Java java_pname -> Procname.java_get_method java_pname = "onClick"
+    | _ -> false in
+  let is_annotated pdesc =
+    let annotated_signature = Annotations.get_annotated_signature (Procdesc.get_attributes pdesc) in
+    let ret_annotation, _ = annotated_signature.Annotations.ret in
+    Annotations.ia_is_ui_thread ret_annotation in
+  is_on_click proc_desc || is_annotated proc_desc
+
+(* return true if we should compute a summary for the procedure. if this returns false, we won't
+   analyze the procedure or report any warnings on it *)
+(* note: in the future, we will want to analyze the procedures in all of these cases in order to
+   find more bugs. this is just a temporary measure to avoid obvious false positives *)
+let should_analyze_proc pdesc tenv =
+  let pn = Procdesc.get_proc_name pdesc in
+  not (is_initializer tenv pn) &&
+  not (FbThreadSafety.is_logging_method pn) &&
+  not (is_call_to_builder_class_method pn) &&
+  not (is_call_to_immutable_collection_method tenv pn) &&
+  not (runs_on_ui_thread pdesc)
+
+(* return true if we should report on unprotected accesses during the procedure *)
+let should_report_on_proc (_, tenv, proc_name, proc_desc) =
   not (is_initializer tenv proc_name) &&
   not (Procname.java_is_autogen_method proc_name) &&
   Procdesc.get_access proc_desc <> PredSymb.Private
 
-let dummy_cg = Cg.create None
-
 (* creates a map from proc_envs to postconditions *)
 let make_results_table get_proc_desc file_env =
   (* make a Map sending each element e of list l to (f e) *)
@@ -265,12 +285,19 @@ let make_results_table get_proc_desc file_env =
   in
   let compute_post_for_procedure = (* takes proc_env as arg *)
     fun (idenv, tenv, proc_name, proc_desc) ->
+      let empty = false, ThreadSafetyDomain.PathDomain.empty, ThreadSafetyDomain.PathDomain.empty in
       (* convert the abstract state to a summary by dropping the id map *)
       let compute_post ({ ProcData.pdesc; tenv; } as proc_data) =
-        if not (Procdesc.did_preanalysis pdesc) then Preanal.do_liveness pdesc tenv;
+        if should_analyze_proc pdesc tenv
-        match Analyzer.compute_post proc_data ~initial:ThreadSafetyDomain.empty with
+        then
-        | Some { locks; reads; writes; } -> Some (locks, reads, writes)
+          begin
-        | None -> None in
+            if not (Procdesc.did_preanalysis pdesc) then Preanal.do_liveness pdesc tenv;
+            match Analyzer.compute_post proc_data ~initial:ThreadSafetyDomain.empty with
+            | Some { locks; reads; writes; } -> Some (locks, reads, writes)
+            | None -> None
+          end
+        else
+          Some empty in
       let callback_arg =
         {Callbacks.get_proc_desc; get_procs_in_file = (fun _ -> []);
          idenv; tenv; proc_name; proc_desc} in
@@ -280,8 +307,7 @@ let make_results_table get_proc_desc file_env =
           ~make_extras:ProcData.make_empty_extras
           callback_arg with
       | Some post -> post
-      | None ->
+      | None -> empty
-          false, ThreadSafetyDomain.PathDomain.empty, ThreadSafetyDomain.PathDomain.empty
   in
   map_post_computation_over_procs compute_post_for_procedure file_env
 

infer/src/checkers/annotations.ml

@@ -115,6 +115,7 @@ let integrity_sink = "IntegritySink"
 let guarded_by = "GuardedBy"
 let thread_safe = "ThreadSafe"
 let not_thread_safe = "NotThreadSafe"
+let ui_thread = "UiThread"
 
 let ia_is_not_thread_safe ia =
   ia_ends_with ia not_thread_safe
@@ -214,6 +215,9 @@ let ia_is_integrity_sink ia =
 let ia_is_guarded_by ia =
   ia_ends_with ia guarded_by
 
+let ia_is_ui_thread ia =
+  ia_ends_with ia ui_thread
+
 type annotation =
   | Nullable
   | Present

infer/src/checkers/annotations.mli

@@ -96,6 +96,7 @@ val ia_is_integrity_sink : Annot.Item.t -> bool
 val ia_is_guarded_by : Annot.Item.t -> bool
 val ia_is_not_thread_safe : Annot.Item.t -> bool
 val ia_is_thread_safe : Annot.Item.t -> bool
+val ia_is_ui_thread : Annot.Item.t -> bool
 
 val ia_iter : (Annot.t -> unit) -> Annot.Item.t -> unit
 

infer/tests/codetoanalyze/java/threadsafety/Annotations.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2017 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package codetoanalyze.java.checkers;
+
+import android.support.annotation.UiThread;
+
+/** tests for classes and method annotations that are meaningful w.r.t thread-safety */
+
+@ThreadSafe
+class Annotations {
+  Object f;
+
+  @UiThread
+  public void setF(Object newF) {
+    this.f = newF; // shouldn't report here
+  }
+
+  public void callSetFOnMethodOk(Annotations obj) {
+    obj.setF(new Object()); // or here
+  }
+
+  public void mutateOffUiThreadBad() {
+    this.f = new Object();
+  }
+
+  // we model onClick as running on the UI thread, should not warn
+  public void onClick() {
+    this.f = new Object();
+  }
+
+}

infer/tests/codetoanalyze/java/threadsafety/issues.exp

@@ -1,6 +1,6 @@
+codetoanalyze/java/threadsafety/Annotations.java, void Annotations.mutateOffUiThreadBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.f]
 codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g]
 codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g]
-codetoanalyze/java/threadsafety/Builders.java, void TopLevelBuilder.setG(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.TopLevelBuilder.g]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]