From e1321883613e2e622c9f43afc844aa7b1c2e9b66 Mon Sep 17 00:00:00 2001
From: Sam Blackshear <shb@fb.com>
Date: Mon, 23 Jan 2017 15:39:27 -0800
Subject: [PATCH] [thread-safety] warn on unsafe accesses in overrides of
 methods annotated with @ThreadSafeMethod

Reviewed By: jeremydubreil

Differential Revision: D4439591

fbshipit-source-id: 268890b
---
 infer/src/checkers/ThreadSafety.ml            | 23 +++++++++++++++++--
 .../java/threadsafety/ThreadSafeExample.java  | 19 +++++++++++++++
 .../java/threadsafety/issues.exp              |  1 +
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml
index 48412c3f2..c09165ed2 100644
--- a/infer/src/checkers/ThreadSafety.ml
+++ b/infer/src/checkers/ThreadSafety.ml
@@ -589,8 +589,27 @@ let should_report_on_file file_env =
    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)
+  (* TODO (t15588153): clean this up *)
+  let is_thread_safe_method pdesc tenv =
+    let overrides_thread_safe_method pname tenv =
+      let check_method_attributes check pname =
+        match Specs.proc_resolve_attributes pname with
+        | None -> false
+        | Some attributes ->
+            let annotated_signature = Annotations.get_annotated_signature attributes in
+            let ret_annotation, _ = annotated_signature.Annotations.ret in
+            check ret_annotation in
+      let found = ref false in
+      PatternMatch.proc_iter_overridden_methods
+        (fun pn ->
+           found := !found || check_method_attributes Annotations.ia_is_thread_safe_method pn)
+        tenv
+        pname;
+      !found in
+    is_annotated Annotations.ia_is_thread_safe_method pdesc||
+    overrides_thread_safe_method (Procdesc.get_proc_name pdesc) tenv in
+  let should_report ((_, tenv, _, pdesc) as proc_env) =
+    (should_report_on_all_procs || is_thread_safe_method pdesc tenv)
     && should_report_on_proc proc_env in
   ResultsTableType.iter (* report errors for each method *)
     (fun proc_env (_, _, conditional_writes, unconditional_writes) ->
diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
index 16d08d97f..10886f137 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
+++ b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
@@ -153,4 +153,23 @@ class NonThreadSafeClass {
     this.field = new Object(); // should warn
   }
 
+  @ThreadSafeMethod
+  public void safeMethod() {
+  }
+
+}
+
+class NonThreadSafeSubclass extends NonThreadSafeClass {
+
+  @Override
+  // overrides method annotated with @ThreadSafeMethod, should warn
+  public void safeMethod() {
+    this.field = new Object();
+  }
+
+  // won't report this now, but should in the future. if a method annotated with @ThreadSafeMethod
+  // in class C touches field f, then all other accesses to f in C must also be thread-safe
+  public void FN_touchesSameFieldAsThreadSafeMethod() {
+    this.field = new Object();
+  }
 }
diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp
index 463442122..e0d5f75be 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp
+++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp
@@ -25,6 +25,7 @@ codetoanalyze/java/threadsafety/Ownership.java, void Ownership.writeToNotOwnedIn
 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 NonThreadSafeSubclass.safeMethod(), 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]