[thread-safety] add and support @ThreadSafeMethod annotation

Reviewed By: jeremydubreil

Differential Revision: D4411223

fbshipit-source-id: 55b0a6b

infer/annotations/com/facebook/infer/annotation/ThreadSafeMethod.java

@@ -0,0 +1,22 @@
+/*
+ * 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 com.facebook.infer.annotation;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/** like javax.annotation.concurrent.ThreadSafe, but for individual methods rather than classes */
+
+@Target(ElementType.METHOD)
+@Retention(RetentionPolicy.CLASS)
+public @interface ThreadSafeMethod {
+}

infer/src/checkers/ThreadSafety.ml

@@ -296,17 +296,19 @@ let is_thread_confined_method tenv pname =
   PatternMatch.check_current_class_attributes
     Annotations.ia_is_thread_confined tenv pname
 
+let is_annotated f_annot pdesc =
+  let annotated_signature = Annotations.get_annotated_signature (Procdesc.get_attributes pdesc) in
+  let ret_annotation, _ = annotated_signature.Annotations.ret in
+  f_annot ret_annotation
 
 (* 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 annotated with @UiThread or @OnEvent(SomeEvent.class) always run on the UI
      thread *)
-  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 || Annotations.ia_is_on_event ret_annotation in
-  is_annotated proc_desc
+  is_annotated
+    (fun annot -> Annotations.ia_is_ui_thread annot || Annotations.ia_is_on_event annot)
+    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 *)
@@ -421,17 +423,6 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) trace =
     report_one_path
     (PathDomain.get_reportable_sink_paths trace ~trace_of_pname)
 
-(* For now, just checks if there is one active element amongst the posts of the analyzed methods.
-   This indicates that the method races with itself. To be refined later. *)
-let process_results_table tab =
-  ResultsTableType.iter   (* report errors for each method *)
-    (fun proc_env (_, _, writes) ->
-       if should_report_on_proc proc_env then
-         report_thread_safety_violations proc_env writes
-       else ()
-    )
-    tab
-
 (* 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 *)
@@ -450,21 +441,21 @@ let should_report_on_file file_env =
   not (IList.exists current_class_marked_not_threadsafe file_env) &&
   IList.exists current_class_or_super_marked_threadsafe file_env
 
+(* For now, just checks if there is one active element amongst the posts of the analyzed methods.
+   This indicates that the method races with itself. To be refined later. *)
+let process_results_table file_env tab =
+  let should_report_on_all_procs = should_report_on_file file_env in
+  let should_report ((_, _, _, pdesc) as proc_env) =
+    (should_report_on_all_procs || is_annotated Annotations.ia_is_thread_safe_method pdesc)
+    && should_report_on_proc proc_env in
+  ResultsTableType.iter (* report errors for each method *)
+    (fun proc_env (_, _, writes) ->
+       if should_report proc_env then report_thread_safety_violations proc_env writes)
+    tab
+
 (*This is a "cluster checker" *)
 (*Gathers results by analyzing all the methods in a file, then post-processes
   the results to check (approximation of) thread safety *)
 (* file_env: (Idenv.t * Tenv.t * Procname.t * Procdesc.t) list *)
 let file_analysis _ _ get_procdesc file_env =
-  let tab = make_results_table get_procdesc file_env in
-  if should_report_on_file file_env then
-    process_results_table tab
-  else ()
-
-      (*
-    Todo:
-    0. Refactor abstract domain to use records rather than tuples
-    1. Track line numbers of accesses
-    2. Track protected writes and reads, too; if we have a write and a read where
-    one is non-protected then we have potential race (including protected write, unprotected read
-    3. Lotsa other stuff
-      *)
+  process_results_table file_env (make_results_table get_procdesc file_env)

infer/src/checkers/annotations.ml

@@ -121,6 +121,7 @@ let integrity_source = "IntegritySource"
 let integrity_sink = "IntegritySink"
 let guarded_by = "GuardedBy"
 let thread_safe = "ThreadSafe"
+let thread_safe_method = "ThreadSafeMethod"
 let not_thread_safe = "NotThreadSafe"
 let ui_thread = "UiThread"
 let any_thread = "AnyThread"
@@ -134,6 +135,9 @@ let ia_is_not_thread_safe ia =
 let ia_is_thread_safe ia =
   ia_ends_with ia thread_safe
 
+let ia_is_thread_safe_method ia =
+  ia_ends_with ia thread_safe_method
+
 let ia_is_nullable ia =
   ia_ends_with ia nullable
 

infer/src/checkers/annotations.mli

@@ -101,6 +101,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_thread_safe_method : Annot.Item.t -> bool
 val ia_is_ui_thread : Annot.Item.t -> bool
 val ia_is_thread_confined : Annot.Item.t -> bool
 

infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java

@@ -9,6 +9,8 @@
 
 package codetoanalyze.java.checkers;
 
+import com.facebook.infer.annotation.ThreadSafeMethod;
+
 @ThreadSafe
 public class ThreadSafeExample{
 
@@ -131,3 +133,14 @@ class YesThreadSafeExtendsNotThreadSafeExample extends NotThreadSafeExtendsThrea
   }
 
 }
+
+class NonThreadSafeClass {
+
+  Object field;
+
+  @ThreadSafeMethod
+  public void threadSafeMethod() {
+    this.field = new Object(); // should warn
+  }
+
+}

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

@@ -17,6 +17,7 @@ codetoanalyze/java/threadsafety/Ownership.java, void Ownership.ownInOneBranchBad
 codetoanalyze/java/threadsafety/Ownership.java, void Ownership.reassignToFormalBad(Obj), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Obj.g]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.newmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void NonThreadSafeClass.threadSafeMethod(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.NonThreadSafeClass.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callPublicMethodBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.oddBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.evenOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]