[racerd] equip interface call records with address

Summary:
Morally, INTERFACE_NOT_THREAD_SAFE is issued when an interface method is invoked from `ThreadSafe`-annotated code on an interface that is not known to be thread-safe or annotated so.

However, the ultimate purpose is to prevent races. Thus it should never be issued on an owned object or on objects we would not report races on for any reason (local variables, non-source variables, etc).

This diff equips interface call records with the abstract address they are invoked on, and uses the same rules for maintaining those records or not.

Reviewed By: skcho

Differential Revision: D20669259

fbshipit-source-id: 6c7841e6a

infer/src/concurrency/RacerD.ml

@@ -227,7 +227,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     let ret_access_exp = AccessExpression.base ret_base in
     let astate =
       if RacerDModels.should_flag_interface_call tenv actuals call_flags callee_pname then
-        Domain.add_unannotated_call_access extras callee_pname loc astate
+        Domain.add_unannotated_call_access extras callee_pname actuals loc astate
       else astate
     in
     let astate =
@@ -776,8 +776,8 @@ end = struct
           Location (AccessExpression.to_access_path exp)
       | ContainerRead {exp} | ContainerWrite {exp} ->
           Container (AccessExpression.to_access_path exp)
-      | InterfaceCall pn ->
+      | InterfaceCall {pname} ->
-          Call pn
+          Call pname
   end
 
   module M = Caml.Map.Make (Key)
@@ -843,11 +843,10 @@ let should_report_guardedby_violation classname ({snapshot; tenv; procname} : re
   (* restrict check to access paths of length one *)
   match
     RacerDDomain.Access.get_access_exp snapshot.elem.access
-    |> Option.map ~f:AccessExpression.to_accesses
+    |> AccessExpression.to_accesses
-    |> Option.map ~f:(fun (base, accesses) ->
+    |> fun (base, accesses) -> (base, List.filter accesses ~f:HilExp.Access.is_field_or_array_access)
-           (base, List.filter accesses ~f:HilExp.Access.is_field_or_array_access) )
   with
-  | Some (AccessExpression.Base (_, base_type), [HilExp.Access.FieldAccess field_name]) -> (
+  | AccessExpression.Base (_, base_type), [HilExp.Access.FieldAccess field_name] -> (
     match base_type.desc with
     | Tstruct base_name | Tptr ({desc= Tstruct base_name}, _) ->
         (* is the base class a subclass of the one containing the GuardedBy annotation? *)
@@ -946,7 +945,7 @@ let report_unsafe_accesses ~issue_log classname (aggregated_access_map : ReportM
   let report_unsafe_access accesses acc
       ({snapshot; threads; tenv; procname= pname} as reported_access) =
     match snapshot.elem.access with
-    | Access.InterfaceCall reported_pname
+    | Access.InterfaceCall {pname= reported_pname}
       when AccessSnapshot.is_unprotected snapshot
            && ThreadsDomain.is_any threads && is_marked_thread_safe pname tenv ->
         (* un-annotated interface call + no lock in method marked thread-safe. warn *)

infer/src/concurrency/RacerDDomain.ml

@@ -24,7 +24,7 @@ module Access = struct
     | Write of {exp: AccessExpression.t}
     | ContainerRead of {exp: AccessExpression.t; pname: Procname.t}
     | ContainerWrite of {exp: AccessExpression.t; pname: Procname.t}
-    | InterfaceCall of Procname.t
+    | InterfaceCall of {exp: AccessExpression.t; pname: Procname.t}
   [@@deriving compare]
 
   let make_field_access exp ~is_write = if is_write then Write {exp} else Read {exp}
@@ -33,7 +33,7 @@ module Access = struct
     if is_write then ContainerWrite {exp; pname} else ContainerRead {exp; pname}
 
 
-  let make_unannotated_call_access pname = InterfaceCall pname
+  let make_unannotated_call_access exp pname = InterfaceCall {exp; pname}
 
   let is_write = function
     | InterfaceCall _ | Read _ | ContainerRead _ ->
@@ -50,10 +50,8 @@ module Access = struct
 
 
   let get_access_exp = function
-    | Read {exp} | Write {exp} | ContainerWrite {exp} | ContainerRead {exp} ->
+    | Read {exp} | Write {exp} | ContainerWrite {exp} | ContainerRead {exp} | InterfaceCall {exp} ->
-        Some exp
+        exp
-    | InterfaceCall _ ->
-        None
 
 
   let should_keep formals access =
@@ -71,7 +69,7 @@ module Access = struct
           && (not (Pvar.is_static_local pvar))
           && (Pvar.is_global pvar || FormalMap.is_formal base formals)
     in
-    match get_access_exp access with None -> true | Some acc_exp -> check_access acc_exp
+    get_access_exp access |> check_access
 
 
   let map ~f access =
@@ -88,8 +86,9 @@ module Access = struct
     | ContainerRead ({exp} as record) ->
         let exp' = f exp in
         if phys_equal exp exp' then access else ContainerRead {record with exp= exp'}
-    | InterfaceCall _ as intfcall ->
+    | InterfaceCall ({exp} as record) ->
-        intfcall
+        let exp' = f exp in
+        if phys_equal exp exp' then access else InterfaceCall {record with exp= exp'}
 
 
   let pp fmt = function
@@ -101,8 +100,9 @@ module Access = struct
         F.fprintf fmt "Read of container %a via %a" AccessExpression.pp exp Procname.pp pname
     | ContainerWrite {exp; pname} ->
         F.fprintf fmt "Write to container %a via %a" AccessExpression.pp exp Procname.pp pname
-    | InterfaceCall pname ->
+    | InterfaceCall {exp; pname} ->
-        F.fprintf fmt "Call to un-annotated interface method %a" Procname.pp pname
+        F.fprintf fmt "Call to un-annotated interface method %a.%a" AccessExpression.pp exp
+          Procname.pp pname
 
 
   let mono_lang_pp = MF.wrap_monospaced pp_exp
@@ -116,7 +116,7 @@ module Access = struct
     | ContainerWrite {exp; pname} ->
         F.fprintf fmt "Write to container %a via call to %s" mono_lang_pp exp
           (MF.monospaced_to_string (Procname.get_method pname))
-    | InterfaceCall pname ->
+    | InterfaceCall {pname} ->
         F.fprintf fmt "Call to un-annotated interface method %a" Procname.pp pname
 end
 
@@ -286,9 +286,9 @@ module AccessSnapshot = struct
     make {access; lock; thread; ownership_precondition} loc |> filter formals
 
 
-  let make_unannotated_call_access formals pname lock ownership loc =
+  let make_unannotated_call_access formals exp pname lock ownership loc =
     let lock = LockDomain.is_locked lock in
-    let access = Access.make_unannotated_call_access pname in
+    let access = Access.make_unannotated_call_access exp pname in
     make_if_not_owned formals access lock ownership loc
 
 
@@ -531,9 +531,18 @@ let pp fmt {threads; locks; accesses; ownership; attribute_map} =
     ownership AttributeMapDomain.pp attribute_map
 
 
-let add_unannotated_call_access formals pname loc (astate : t) =
+let add_unannotated_call_access formals pname actuals loc (astate : t) =
+  match actuals with
+  | [] ->
+      astate
+  | receiver_hilexp :: _ -> (
+    match HilExp.get_access_exprs receiver_hilexp with
+    | [] | _ :: _ :: _ ->
+        (* if no access exps involved, or if more than one (should be impossible), ignore *)
+        astate
+    | [receiver] ->
         let snapshot =
-    AccessSnapshot.make_unannotated_call_access formals pname astate.locks astate.threads Unowned
+          AccessSnapshot.make_unannotated_call_access formals receiver pname astate.locks
-      loc
+            astate.threads Unowned loc
         in
-  {astate with accesses= AccessDomain.add_opt snapshot astate.accesses}
+        {astate with accesses= AccessDomain.add_opt snapshot astate.accesses} )

infer/src/concurrency/RacerDDomain.mli

@@ -19,12 +19,12 @@ module Access : sig
     | ContainerRead of {exp: AccessExpression.t; pname: Procname.t}  (** Read of container object *)
     | ContainerWrite of {exp: AccessExpression.t; pname: Procname.t}
         (** Write to container object *)
-    | InterfaceCall of Procname.t
+    | InterfaceCall of {exp: AccessExpression.t; pname: Procname.t}
         (** Call to method of interface not annotated with [@ThreadSafe] *)
 
   val pp : F.formatter -> t -> unit
 
-  val get_access_exp : t -> AccessExpression.t option
+  val get_access_exp : t -> AccessExpression.t
 end
 
 (** Overapproximation of number of time the lock has been acquired *)
@@ -191,7 +191,7 @@ type t =
 
 include AbstractDomain.WithBottom with type t := t
 
-val add_unannotated_call_access : FormalMap.t -> Procname.t -> Location.t -> t -> t
+val add_unannotated_call_access : FormalMap.t -> Procname.t -> HilExp.t list -> Location.t -> t -> t
 
 (** same as astate, but without [attribute_map] (since these involve locals) and with the addition
     of the ownership/attributes associated with the return value as well as the set of formals which

infer/tests/codetoanalyze/java/racerd/Dispatch.java

@@ -78,6 +78,15 @@ public class Dispatch {
       i.foo();
     }
   }
+
+  private void privateCallUnnanotatedInterfaceOk(UnannotatedInterface i) {
+    i.foo();
+  }
+
+  public void callOwnedUnnanotatedInterfaceOk() {
+    UnannotatedInterface owned = new UnannotadedImplementation();
+    privateCallUnnanotatedInterfaceOk(owned);
+  }
 }
 
 class Some {
@@ -102,3 +111,7 @@ class ThreadConfinedField {
     mNormal.foo();
   }
 }
+
+class UnannotadedImplementation implements UnannotatedInterface {
+  public void foo() {}
+}

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

@@ -49,7 +49,7 @@ codetoanalyze/java/racerd/Containers.java, codetoanalyze.java.checkers.Container
 codetoanalyze/java/racerd/DeepOwnership.java, DeepOwnership.globalNotOwnedBad():void, 16, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `DeepOwnership.global.next`]
 codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.Dispatch.callUnannotatedInterfaceBad(codetoanalyze.java.checkers.UnannotatedInterface):void, 49, INTERFACE_NOT_THREAD_SAFE, no_bucket, WARNING, [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, WARNING, [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, WARNING, [Call to un-annotated interface method void UnannotatedInterface.foo()]
+codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.ThreadConfinedField.interfaceCallOnNormalFieldBad():void, 111, INTERFACE_NOT_THREAD_SAFE, no_bucket, WARNING, [Call to un-annotated interface method void UnannotatedInterface.foo()]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByOther.accessBad():void, 127, GUARDEDBY_VIOLATION, no_bucket, WARNING, [access to `this.x`]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedWriteBad():void, 59, GUARDEDBY_VIOLATION, no_bucket, WARNING, [call to void GuardedByTests.privateUnlockedWriteOk(),access to `this.d`]
 codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.unlockedWriteBad():void, 35, GUARDEDBY_VIOLATION, no_bucket, WARNING, [access to `this.b`]