From 76f80f114ce03e0a9d4199a2e614f2bdc978f6f8 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 27 Mar 2020 06:45:42 -0700 Subject: [PATCH] [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 | 15 +++--- infer/src/concurrency/RacerDDomain.ml | 49 +++++++++++-------- infer/src/concurrency/RacerDDomain.mli | 6 +-- .../codetoanalyze/java/racerd/Dispatch.java | 13 +++++ .../codetoanalyze/java/racerd/issues.exp | 2 +- 5 files changed, 53 insertions(+), 32 deletions(-) diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 747874d6e..91a390a9e 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/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 -> - Call pn + | InterfaceCall {pname} -> + 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 - |> Option.map ~f:(fun (base, accesses) -> - (base, List.filter accesses ~f:HilExp.Access.is_field_or_array_access) ) + |> AccessExpression.to_accesses + |> fun (base, accesses) -> (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 *) diff --git a/infer/src/concurrency/RacerDDomain.ml b/infer/src/concurrency/RacerDDomain.ml index 6b4672301..a7175e8c0 100644 --- a/infer/src/concurrency/RacerDDomain.ml +++ b/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} -> - Some exp - | InterfaceCall _ -> - None + | Read {exp} | Write {exp} | ContainerWrite {exp} | ContainerRead {exp} | InterfaceCall {exp} -> + exp 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 -> - intfcall + | InterfaceCall ({exp} as record) -> + 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 -> - F.fprintf fmt "Call to un-annotated interface method %a" Procname.pp pname + | InterfaceCall {exp; 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 snapshot = - AccessSnapshot.make_unannotated_call_access formals pname astate.locks astate.threads Unowned - loc - in - {astate with accesses= AccessDomain.add_opt snapshot astate.accesses} +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 receiver pname astate.locks + astate.threads Unowned loc + in + {astate with accesses= AccessDomain.add_opt snapshot astate.accesses} ) diff --git a/infer/src/concurrency/RacerDDomain.mli b/infer/src/concurrency/RacerDDomain.mli index dcd19b3e1..03b6c8496 100644 --- a/infer/src/concurrency/RacerDDomain.mli +++ b/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 diff --git a/infer/tests/codetoanalyze/java/racerd/Dispatch.java b/infer/tests/codetoanalyze/java/racerd/Dispatch.java index f7e3710ac..24b045ef7 100644 --- a/infer/tests/codetoanalyze/java/racerd/Dispatch.java +++ b/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() {} +} diff --git a/infer/tests/codetoanalyze/java/racerd/issues.exp b/infer/tests/codetoanalyze/java/racerd/issues.exp index c1ba2ffc4..632cc7f31 100644 --- a/infer/tests/codetoanalyze/java/racerd/issues.exp +++ b/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`]