[racerd] restrict guarded by to one field/same or superclass

Reviewed By: jeremydubreil

Differential Revision: D14584698

fbshipit-source-id: ab0a7db11

infer/src/checkers/annotations.ml

@@ -251,3 +251,5 @@ let ia_is_mainthread ia = ia_ends_with ia mainthread
 let ia_is_thread_confined ia = ia_ends_with ia thread_confined
 
 let ia_is_worker_thread ia = ia_ends_with ia worker_thread
+
+let ia_is_guardedby ia = ia_ends_with ia guarded_by

infer/src/checkers/annotations.mli

@@ -84,6 +84,8 @@ val ia_is_expensive : Annot.Item.t -> bool
 
 val ia_is_functional : Annot.Item.t -> bool
 
+val ia_is_guardedby : Annot.Item.t -> bool
+
 val ia_is_propagates_nullable : Annot.Item.t -> bool
 
 val ia_is_ignore_allocations : Annot.Item.t -> bool

infer/src/concurrency/RacerD.ml

@@ -936,14 +936,25 @@ let should_report_on_proc tenv procdesc =
       false
 
 
-let should_report_guardedby_violation ({snapshot; tenv} : reported_access) =
+let should_report_guardedby_violation classname_str ({snapshot; tenv; procdesc} : reported_access)
+    =
   (not snapshot.lock)
-  && RacerDDomain.Access.get_access_path snapshot.access.elem
+  && Procdesc.get_proc_name procdesc |> Typ.Procname.is_java
-     |> Option.exists ~f:(fun path ->
+  &&
-            AccessPath.get_field_and_annotation path tenv
+  (* restrict check to access paths of length one *)
-            |> Option.exists ~f:(fun (_, annotlist) ->
+  match RacerDDomain.Access.get_access_path snapshot.access.elem with
-                   List.exists annotlist ~f:(fun (annot, _) ->
+  | Some ((_, base_type), [AccessPath.FieldAccess field_name]) -> (
-                       Annotations.annot_ends_with annot Annotations.guarded_by ) ) )
+      Typ.Struct.get_field_type_and_annotation ~lookup:(Tenv.lookup tenv) field_name base_type
+      |> Option.exists ~f:(fun (_, annotlist) -> Annotations.ia_is_guardedby annotlist)
+      &&
+      (* is the base class a subclass of the one containing the GuardedBy annotation? *)
+      match base_type.Typ.desc with
+      | Tstruct base_name | Tptr ({Typ.desc= Tstruct base_name}, _) ->
+          PatternMatch.is_subtype tenv base_name (Typ.Name.Java.from_string classname_str)
+      | _ ->
+          false )
+  | _ ->
+      false
 
 
 (** Report accesses that may race with each other.
@@ -974,7 +985,7 @@ let should_report_guardedby_violation ({snapshot; tenv} : reported_access) =
     currently not distinguishing different locks, and are treating "known to be confined to a
     thread" as if "known to be confined to UI thread".
 *)
-let report_unsafe_accesses (aggregated_access_map : ReportMap.t) =
+let report_unsafe_accesses classname (aggregated_access_map : ReportMap.t) =
   let open RacerDDomain in
   let open RacerDModels in
   let is_duplicate_report ({snapshot; procdesc} : reported_access)
@@ -1095,13 +1106,14 @@ let report_unsafe_accesses (aggregated_access_map : ReportMap.t) =
     (* Don't report on location if all accesses are on non-concurrent contexts *)
     if
       List.for_all reportable_accesses ~f:(fun ({threads} : reported_access) ->
+          (* FIXME this should be any thread or no thread *)
           ThreadsDomain.is_any threads |> not )
     then init
     else List.fold reportable_accesses ~init ~f:(report_unsafe_access reportable_accesses)
   in
   let report_guardedby_violations_on_location grouped_accesses init =
     List.fold grouped_accesses ~init ~f:(fun acc r ->
-        if should_report_guardedby_violation r && not (is_duplicate_report r acc) then (
+        if should_report_guardedby_violation classname r && not (is_duplicate_report r acc) then (
           report_thread_safety_violation ~make_description:make_guardedby_violation_description
             ~report_kind:GuardedByViolation r ;
           update_reported r acc )
@@ -1151,5 +1163,6 @@ let aggregate_by_class file_env =
    safety *)
 let file_analysis {Callbacks.procedures; source_file} =
   aggregate_by_class procedures
-  |> String.Map.iter ~f:(fun class_env -> report_unsafe_accesses (make_results_table class_env)) ;
+  |> String.Map.iteri ~f:(fun ~key:classname ~data:class_env ->
+         report_unsafe_accesses classname (make_results_table class_env) ) ;
   IssueLog.store Config.racerd_issues_dir_name source_file

infer/tests/codetoanalyze/java/racerd/GuardedByTests.java

@@ -89,4 +89,23 @@ public class GuardedByTests {
   synchronized int syncReadOk() {
     return g;
   }
+
+  GuardedByOther o;
+
+  void accessThroughMemberObjectOk() {
+    o.accessBad();
+  }
+
+  void accessIndirectOk(GuardedByOther o) {
+    o.accessBad();
+  }
+}
+
+class GuardedByOther {
+  @GuardedBy("bla")
+  int x;
+
+  void accessBad() {
+    x = 0;
+  }
 }

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

@@ -51,6 +51,7 @@ codetoanalyze/java/racerd/DeepOwnership.java, DeepOwnership.reassignBaseToGlobal
 codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.Dispatch.callUnannotatedInterfaceBad(codetoanalyze.java.checkers.UnannotatedInterface):void, 49, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [Call to un-annotated interface method void UnannotatedInterface.foo()]
 codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.Dispatch.callUnannotatedInterfaceIndirectBad(codetoanalyze.java.checkers.NotThreadSafe,codetoanalyze.java.checkers.UnannotatedInterface):void, 53, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [call to void NotThreadSafe.notThreadSafeOk(UnannotatedInterface),Call to un-annotated interface method void UnannotatedInterface.foo()]
 codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.ThreadConfinedField.interfaceCallOnNormalFieldBad():void, 102, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [Call to un-annotated interface method void UnannotatedInterface.foo()]
+codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByOther.accessBad():void, 109, GUARDEDBY_VIOLATION, no_bucket, ERROR, [access to `this.x`]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedReadBad():int, 58, GUARDEDBY_VIOLATION, no_bucket, ERROR, [call to int GuardedByTests.privateUnlockedReadOk(),access to `this.f`]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedReadBad():int, 58, THREAD_SAFETY_VIOLATION, no_bucket, ERROR, [<Read trace>,call to int GuardedByTests.privateUnlockedReadOk(),access to `this.f`,<Write trace>,access to `this.f`]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedWriteBad():void, 54, GUARDEDBY_VIOLATION, no_bucket, ERROR, [call to void GuardedByTests.privateUnlockedWriteOk(),access to `this.f`]