[thread-safety] report more warnings by inferring when state is accessed by methods marked `@ThreadSafe` and other methods.

Summary: If two public methods touch the same state and only one is marked `ThreadSafe`, it's reasonable to report unsafe accesses on both of them.

Reviewed By: peterogithub

Differential Revision: D4785038

fbshipit-source-id: 5a80da4

infer/src/checkers/ThreadSafety.ml

@@ -805,14 +805,6 @@ let is_thread_safe_method pdesc tenv =
     tenv
     (Procdesc.get_proc_name pdesc)
 
-(* return true if we should report on unprotected accesses during the procedure *)
-let should_report_on_proc proc_desc tenv =
-  let proc_name = Procdesc.get_proc_name proc_desc in
-  is_thread_safe_method proc_desc tenv ||
-  (not (Typ.Procname.java_is_autogen_method proc_name) &&
-   Procdesc.get_access proc_desc <> PredSymb.Private &&
-   not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting))
-
 let empty_post =
   let initial_known_on_ui_thread = false
   and has_lock = false
@@ -1035,8 +1027,17 @@ let empty_reported =
   let reported_reads = Typ.Procname.Set.empty in
   { reported_sites; reported_reads; reported_writes; }
 
+(* return true if procedure is at an abstraction boundary or reporting has been explicitly
+   requested via @ThreadSafe *)
+let should_report_on_proc proc_desc tenv =
+  let proc_name = Procdesc.get_proc_name proc_desc in
+  is_thread_safe_method proc_desc tenv ||
+  (not (Typ.Procname.java_is_autogen_method proc_name) &&
+   Procdesc.get_access proc_desc <> PredSymb.Private &&
+   not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting))
+
 (* report accesses that may race with each other *)
-let report_unsafe_accesses ~should_report aggregated_access_map =
+let report_unsafe_accesses ~is_file_threadsafe aggregated_access_map =
   let open ThreadSafetyDomain in
   let is_duplicate_report access pname { reported_sites; reported_writes; reported_reads; } =
     CallSite.Set.mem (TraceElem.call_site access) reported_sites ||
@@ -1054,7 +1055,8 @@ let report_unsafe_accesses ~should_report aggregated_access_map =
     | Access.Read ->
         let reported_reads = Typ.Procname.Set.add pname reported.reported_reads in
         { reported with reported_reads; reported_sites; } in
-  let report_unsafe_access (access, pre, threaded, tenv, pdesc) accesses reported_acc =
+  let report_unsafe_access
+      ~should_report (access, pre, threaded, tenv, pdesc) accesses reported_acc =
     let pname = Procdesc.get_proc_name pdesc in
     if is_duplicate_report access pname reported_acc
     then
@@ -1130,34 +1132,26 @@ let report_unsafe_accesses ~should_report aggregated_access_map =
        let reported =
          { reported_acc with reported_writes = Typ.Procname.Set.empty;
                              reported_reads = Typ.Procname.Set.empty; } in
+       let accessed_by_threadsafe_method =
+         List.exists
+           ~f:(fun (_, _, _, tenv, pdesc) -> is_thread_safe_method pdesc tenv)
+           grouped_accesses in
+       (* report if
+          - the method/class of the access is thread-safe (or an override or superclass is), or
+          - any access is in a field marked thread-safe (or an override) *)
+       let should_report pdesc tenv =
+         ((is_file_threadsafe || accessed_by_threadsafe_method) && should_report_on_proc pdesc tenv)
+         || is_thread_safe_method pdesc tenv in
        let reportable_accesses =
          List.filter ~f:(fun (_, _, _, tenv, pdesc) -> should_report pdesc tenv) grouped_accesses in
        List.fold
-         ~f:(fun acc access -> report_unsafe_access access reportable_accesses acc)
+         ~f:(fun acc access -> report_unsafe_access ~should_report access reportable_accesses acc)
          reportable_accesses
          ~init:reported)
     aggregated_access_map
     empty_reported
   |> ignore
 
-(* Currently we analyze if there is an @ThreadSafe annotation on at least one of
-   the classes in a file. This might be tightened in future or even broadened in future
-   based on other criteria *)
-let should_report_on_file file_env =
-  let current_class_or_super_marked_threadsafe =
-    fun (_, tenv, pname, _) ->
-      match get_current_class_and_threadsafe_superclasses tenv pname with
-      | Some (_, thread_safe_annotated_classes) ->
-          not (List.is_empty thread_safe_annotated_classes)
-      | _ -> false
-  in
-  let current_class_marked_not_threadsafe =
-    fun (_, tenv, pname, _) ->
-      PatternMatch.check_current_class_attributes Annotations.ia_is_not_thread_safe tenv pname
-  in
-  not (List.exists ~f:current_class_marked_not_threadsafe file_env) &&
-  List.exists ~f:current_class_or_super_marked_threadsafe file_env
-
 (* create a map from [abstraction of a memory loc] -> accesses that may touch that memory loc. for
    now, our abstraction is an access path like x.f.g whose concretization is the set of memory cells
    that x.f.g may point to during execution *)
@@ -1213,11 +1207,21 @@ let make_results_table file_env =
    confined to a thread" as if "known to be confined to UI thread".
 *)
 let process_results_table file_env tab =
-  let should_report_on_all_procs = should_report_on_file file_env in
+  (* Currently we analyze if there is an @ThreadSafe annotation on at least one of the classes in a
-  let should_report pdesc tenv =
+     file. This might be tightened in future or even broadened in future based on other criteria *)
-    (should_report_on_all_procs && should_report_on_proc pdesc tenv) ||
+  let is_file_threadsafe =
-    is_thread_safe_method pdesc tenv in
+    let current_class_or_super_marked_threadsafe (_, tenv, pname, _) =
-  report_unsafe_accesses ~should_report tab
+      match get_current_class_and_threadsafe_superclasses tenv pname with
+      | Some (_, thread_safe_annotated_classes) ->
+          not (List.is_empty thread_safe_annotated_classes)
+      | _ ->
+          false in
+    let current_class_marked_not_threadsafe (_, tenv, pname, _) =
+      PatternMatch.check_current_class_attributes Annotations.ia_is_not_thread_safe tenv pname in
+    not (List.exists ~f:current_class_marked_not_threadsafe file_env) &&
+    List.exists ~f:current_class_or_super_marked_threadsafe file_env in
+
+  report_unsafe_accesses ~is_file_threadsafe tab
 
 (* Gathers results by analyzing all the methods in a file, then post-processes the results to check
    an (approximation of) thread safety *)

infer/tests/codetoanalyze/java/threadsafety/ThreadSafeMethods.java

@@ -48,20 +48,21 @@ class ThreadSafeMethods {
 
   // won't report this now, but should in the future. if a method annotated with @ThreadSafe
   // in class C touches field f, then all other accesses to f in C must also be thread-safe
-  public void FN_writeSameFieldAsThreadSafeMethod1Bad() {
+  public void writeSameFieldAsThreadSafeMethod1Bad() {
     this.field1 = new Object();
   }
 
   // reads a field that is written in a method marked thread-safe
-  public Object FN_readSameFieldAsThreadSafeMethod1Bad() {
+  public Object readSameFieldAsThreadSafeMethod1Bad() {
     return this.field1;
   }
 
+  // TODO: should we report this or not?
   public synchronized void safelyWriteSameFieldAsThreadSafeMethod1Ok() {
     this.field1 = new Object();
   }
 
-  public synchronized Object safelyReadSameFieldAsThreadSafeMethod1Ok() {
+  public synchronized Object readSameFieldAsThreadSafeMethod1Ok() {
     return this.field1;
   }
 
@@ -71,12 +72,12 @@ class ThreadSafeMethods {
   }
 
   // unprotected write to a field that is written safely in a method marked thread-safe
-  public void FN_writeSameFieldAsThreadSafeMethod2Bad() {
+  public void writeSameFieldAsThreadSafeMethod2Bad() {
     this.field4 = new Object();
   }
 
   // unprotected read of a field that is written safely in a method marked thread-safe
-  public Object FN_readSameFieldAsThreadSafeMethod2Bad() {
+  public Object readSameFieldAsThreadSafeMethod2Bad() {
     return this.field4;
   }
 
@@ -85,13 +86,17 @@ class ThreadSafeMethods {
     return this.field5;
   }
 
+  private void privateAccessOk() {
+    this.field5 = new Object();
+  }
+
   // unprotected write to a field that is read safely in a method marked thread-safe
-  public void FN_writeSameFieldAsThreadSafeMethod3Bad() {
+  public void writeSameFieldAsThreadSafeMethod3Bad() {
     this.field5 = new Object();
   }
 
   // unprotected read of a field that is read safely in a method marked thread-safe
-  public Object FN_readSameFieldAsThreadSafeMethod3Bad() {
+  public Object readSameFieldAsThreadSafeMethod3Bad() {
     return this.field5;
   }
 
@@ -114,11 +119,11 @@ class ThreadSafeMethodsSubclass extends ThreadSafeMethods {
     return this.field1;
   }
 
-  public void FN_writeThreadSafeFieldOfOverrideBad() {
+  public void writeThreadSafeFieldOfOverrideBad() {
     this.subclassField = new Object();
   }
 
-  public Object FN_readThreadSafeFieldOfOverrideBad() {
+  public Object readThreadSafeFieldOfOverrideBad() {
     return this.subclassField;
   }
 

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

@@ -79,8 +79,16 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.o
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void YesThreadSafeExtendsNotThreadSafeExample.subsubmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.YesThreadSafeExtendsNotThreadSafeExample.subsubfield]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod1Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field1]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod2Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field4]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod3Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field5]
 codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.threadSafeMethodReadBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field2]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethodsSubclass.readThreadSafeFieldOfOverrideBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethodsSubclass.subclassField]
 codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.threadSafeMethodWriteBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field1]
 codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.threadSafePrivateMethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field2]
 codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.threadSafeVisibleForTestingMethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field3]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.writeSameFieldAsThreadSafeMethod1Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field1]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.writeSameFieldAsThreadSafeMethod2Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field4]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.writeSameFieldAsThreadSafeMethod3Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethods.field5]
 codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethodsSubclass.safeMethodOverride(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethodsSubclass.subclassField]
+codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethodsSubclass.writeThreadSafeFieldOfOverrideBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeMethodsSubclass.subclassField]