[racerd] record dynamic types used in constructors for synchronized containers

Summary:
One source of false positives on container races is when the container member field is initialised to a concurrent version in a constructor, but the static type of the field doesn't reflect the thread safety of it.

This solution
- tracks flows from constructors of safe data structures to abstract addresses;
- initialises the initial attribute state when analysing a non-constructor method to that achieved by all constructors/class-initializers.
- checks for that attribute when recording container accesses.

Reviewed By: jvillard

Differential Revision: D21089428

fbshipit-source-id: 02a88f6e8
master
Nikos Gorogiannis 5 years ago committed by Facebook GitHub Bot
parent 94b75585e5
commit 73dda893ed

@ -13,16 +13,17 @@ val get_proc_desc : Procname.t -> Procdesc.t option
(** Find a proc desc for the procedure, perhaps loading it from disk. *)
val analyze_proc_desc : caller_summary:Summary.t -> Procdesc.t -> Summary.t option
(** [analyze_proc_desc ~caller_summary callee_pdesc] performs an on-demand analysis of callee_pdesc
triggered during the analysis of caller_summary *)
(** [analyze_proc_desc ~caller_summary callee_pdesc] performs an on-demand analysis of
[callee_pdesc] triggered during the analysis of [caller_summary] *)
val analyze_proc_name : caller_summary:Summary.t -> Procname.t -> Summary.t option
(** [analyze_proc_name ~caller_summary callee_pname] performs an on-demand analysis of callee_pname
triggered during the analysis of caller_summary *)
(** [analyze_proc_name ~caller_summary callee_pname] performs an on-demand analysis of
[callee_pname] triggered during the analysis of [caller_summary] *)
val analyze_proc_name_no_caller : Procname.t -> Summary.t option
(** [analyze_proc_name_no_caller callee_pname] performs an on-demand analysis of callee_pname
triggered by the top-level of a cluster checker *)
(** [analyze_proc_name_no_caller callee_pname] performs an on-demand analysis of [callee_pname]
triggered by the top-level of a file-level checker. This must not be used in any other context,
as this will break incremental analysis. *)
val set_exe_env : Exe_env.t -> unit
(** Set the execution enviroment used during on-demand analysis. *)

@ -0,0 +1,30 @@
(*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*)
open! IStd
let get_java_class_initializer_summary_of caller_summary =
let procname = Summary.get_proc_name caller_summary in
match procname with
| Procname.Java _ ->
Procname.get_class_type_name procname
|> Option.map ~f:(fun tname -> Procname.(Java (Java.get_class_initializer tname)))
|> Option.bind ~f:(Ondemand.analyze_proc_name ~caller_summary)
| _ ->
None
let get_java_constructor_summaries_of tenv caller_summary =
let procname = Summary.get_proc_name caller_summary in
Procname.get_class_type_name procname
(* retrieve its definition *)
|> Option.bind ~f:(Tenv.lookup tenv)
(* get the list of methods in the class *)
|> Option.value_map ~default:[] ~f:(fun (tstruct : Struct.t) -> tstruct.methods)
(* keep only the constructors *)
|> List.filter ~f:Procname.(function Java jname -> Java.is_constructor jname | _ -> false)
(* get the summaries of the constructors *)
|> List.filter_map ~f:(Ondemand.analyze_proc_name ~caller_summary)

@ -0,0 +1,11 @@
(*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*)
open! IStd
val get_java_class_initializer_summary_of : Summary.t -> Summary.t option
val get_java_constructor_summaries_of : Tenv.t -> Summary.t -> Summary.t list

@ -25,6 +25,15 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
type extras = FormalMap.t
let rec get_access_exp = function
| HilExp.AccessExpression access_expr ->
Some access_expr
| HilExp.Cast (_, e) | HilExp.Exception e ->
get_access_exp e
| _ ->
None
let add_access formals loc ~is_write_access locks threads ownership
(proc_data : extras ProcData.t) access_domain exp =
let open Domain in
@ -54,7 +63,10 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
let make_container_access formals ret_base callee_pname ~is_write receiver_ap callee_loc tenv
(astate : Domain.t) =
let open Domain in
if RacerDModels.is_synchronized_container callee_pname receiver_ap tenv then None
if
AttributeMapDomain.is_synchronized astate.attribute_map receiver_ap
|| RacerDModels.is_synchronized_container callee_pname receiver_ap tenv
then None
else
let ownership_pre = OwnershipDomain.get_owned receiver_ap astate.ownership in
let callee_access =
@ -81,14 +93,6 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
let open Domain in
if AccessDomain.is_empty accesses then accesses
else
let rec get_access_exp = function
| HilExp.AccessExpression access_expr ->
Some access_expr
| HilExp.Cast (_, e) | HilExp.Exception e ->
get_access_exp e
| _ ->
None
in
let formal_map = FormalMap.make pdesc in
let expand_exp exp =
match FormalMap.get_formal_index (AccessExpression.get_base exp) formal_map with
@ -145,7 +149,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
AccessDomain.fold update_callee_access callee_accesses caller_astate.accesses
let call_without_summary callee_pname ret_base call_flags actuals astate =
let call_without_summary tenv callee_pname ret_base call_flags actuals astate =
let open RacerDModels in
let open RacerDDomain in
let should_assume_returns_ownership callee_pname (call_flags : CallFlags.t) actuals =
@ -187,6 +191,13 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
astate.ownership
in
{astate with ownership}
else if RacerDModels.is_synchronized_container_constructor tenv callee_pname actuals then
List.hd actuals |> Option.bind ~f:get_access_exp
|> Option.value_map ~default:astate ~f:(fun receiver ->
let attribute_map =
AttributeMapDomain.add receiver Synchronized astate.attribute_map
in
{astate with attribute_map} )
else astate
@ -305,7 +316,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
in
{locks; threads; accesses; ownership; attribute_map}
| None ->
call_without_summary callee_pname ret_base call_flags actuals astate )
call_without_summary tenv callee_pname ret_base call_flags actuals astate )
in
let add_if_annotated predicate attribute attribute_map =
if PatternMatch.override_exists predicate tenv callee_pname then
@ -375,7 +386,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
if bool_value then ThreadsDomain.AnyThreadButSelf else ThreadsDomain.AnyThread
in
{acc with threads}
| Attribute.(Functional | Nothing) ->
| Attribute.(Functional | Nothing | Synchronized) ->
acc
in
let accesses =
@ -389,7 +400,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
|> Option.value_map ~default:astate ~f:(fun bool_value ->
(* prune (prune_exp) can only evaluate to true if the choice is [bool_value].
add the constraint that the choice must be [bool_value] to the state *)
AttributeMapDomain.find access_expr astate.attribute_map
AttributeMapDomain.get access_expr astate.attribute_map
|> apply_choice bool_value astate )
| _ ->
astate
@ -424,6 +435,67 @@ end
module Analyzer = LowerHil.MakeAbstractInterpreter (TransferFunctions (ProcCfg.Normal))
(** Compute the attributes (of static variables) set up by the class initializer. *)
let set_class_init_attributes summary (astate : RacerDDomain.t) =
let open RacerDDomain in
let attribute_map =
ConcurrencyUtils.get_java_class_initializer_summary_of summary
|> Option.bind ~f:Payload.of_summary
|> Option.value_map ~default:AttributeMapDomain.top ~f:(fun summary -> summary.attributes)
in
({astate with attribute_map} : t)
(** Compute the attributes of instance variables that all constructors agree on. *)
let set_constructor_attributes tenv summary (astate : RacerDDomain.t) =
let open RacerDDomain in
let procname = Summary.get_proc_name summary in
(* make a local [this] variable, for replacing all constructor attribute map keys' roots *)
let local_this = Pvar.mk Mangled.this procname |> Var.of_pvar in
let make_local exp =
(* contract here matches that of [StarvationDomain.summary_of_astate] *)
let var, typ = HilExp.AccessExpression.get_base exp in
if Var.is_global var then
(* let expressions rooted at globals unchanged, these are probably from class initialiser *)
exp
else (
assert (Var.is_this var) ;
HilExp.AccessExpression.replace_base ~remove_deref_after_base:false (local_this, typ) exp )
in
let localize_attrs attributes =
AttributeMapDomain.(fold (fun exp attr acc -> add (make_local exp) attr acc) attributes empty)
in
let attribute_map =
ConcurrencyUtils.get_java_constructor_summaries_of tenv summary
|> List.filter_map ~f:Payload.of_summary
(* make instances of [this] local to the current procedure and select only the attributes *)
|> List.map ~f:(fun (summary : summary) -> localize_attrs summary.attributes)
(* join all the attribute maps together *)
|> List.reduce ~f:AttributeMapDomain.join
|> Option.value ~default:AttributeMapDomain.top
in
{astate with attribute_map}
let set_initial_attributes tenv summary astate =
let procname = Summary.get_proc_name summary in
match procname with
| Procname.Java java_pname when Procname.Java.is_class_initializer java_pname ->
(* we are analyzing the class initializer, don't go through on-demand again *)
astate
| Procname.Java java_pname when Procname.Java.(is_constructor java_pname || is_static java_pname)
->
(* analyzing a constructor or static method, so we need the attributes established by the
class initializer *)
set_class_init_attributes summary astate
| Procname.Java _ ->
(* we are analyzing an instance method, so we need constructor-established attributes
which will include those by the class initializer *)
set_constructor_attributes tenv summary astate
| _ ->
astate
let analyze_procedure {Callbacks.exe_env; summary} =
let open RacerDDomain in
let proc_desc = Summary.get_proc_desc summary in
@ -480,11 +552,11 @@ let analyze_procedure {Callbacks.exe_env; summary} =
add_owned_formal acc base
else add_conditionally_owned_formal acc base index )
in
let initial = {bottom with ownership; threads; locks} in
let initial = set_initial_attributes tenv summary {bottom with ownership; threads; locks} in
let formal_map = FormalMap.make proc_desc in
let proc_data = ProcData.make summary tenv formal_map in
Analyzer.compute_post proc_data ~initial
|> Option.map ~f:(astate_to_summary proc_desc)
|> Option.map ~f:(astate_to_summary proc_desc formal_map)
|> Option.value_map ~default:summary ~f:(fun post -> Payload.update_summary post summary)
else Payload.update_summary empty_summary summary

@ -18,6 +18,21 @@ let pp_exp fmt exp =
AccessPath.pp fmt (AccessExpression.to_access_path exp)
let rec should_keep_exp formals (exp : AccessExpression.t) =
match exp with
| FieldOffset (prefix, fld) ->
(not (String.is_substring ~substring:"$SwitchMap" (Fieldname.get_field_name fld)))
&& should_keep_exp formals prefix
| ArrayOffset (prefix, _, _) | AddressOf prefix | Dereference prefix ->
should_keep_exp formals prefix
| Base (LogicalVar _, _) ->
false
| Base (((ProgramVar pvar as var), _) as base) ->
Var.appears_in_source_code var
&& (not (Pvar.is_static_local pvar))
&& (Pvar.is_global pvar || FormalMap.is_formal base formals)
module Access = struct
type t =
| Read of {exp: AccessExpression.t}
@ -54,23 +69,7 @@ module Access = struct
exp
let should_keep formals access =
let rec check_access (exp : AccessExpression.t) =
match exp with
| FieldOffset (prefix, fld) ->
(not (String.is_substring ~substring:"$SwitchMap" (Fieldname.get_field_name fld)))
&& check_access prefix
| ArrayOffset (prefix, _, _) | AddressOf prefix | Dereference prefix ->
check_access prefix
| Base (LogicalVar _, _) ->
false
| Base (((ProgramVar pvar as var), _) as base) ->
Var.appears_in_source_code var
&& (not (Pvar.is_static_local pvar))
&& (Pvar.is_global pvar || FormalMap.is_formal base formals)
in
get_access_exp access |> check_access
let should_keep formals access = get_access_exp access |> should_keep_exp formals
let map ~f access =
match access with
@ -390,7 +389,7 @@ module OwnershipDomain = struct
end
module Attribute = struct
type t = Nothing | Functional | OnMainThread | LockHeld [@@deriving equal]
type t = Nothing | Functional | OnMainThread | LockHeld | Synchronized [@@deriving equal]
let pp fmt t =
( match t with
@ -401,7 +400,9 @@ module Attribute = struct
| OnMainThread ->
"OnMainThread"
| LockHeld ->
"LockHeld" )
"LockHeld"
| Synchronized ->
"Synchronized" )
|> F.pp_print_string fmt
@ -419,16 +420,20 @@ end
module AttributeMapDomain = struct
include AbstractDomain.SafeInvertedMap (AccessExpression) (Attribute)
let find acc_exp t = find_opt acc_exp t |> Option.value ~default:Attribute.top
let get acc_exp t = find_opt acc_exp t |> Option.value ~default:Attribute.top
let is_functional t access_expression =
match find_opt access_expression t with Some Functional -> true | _ -> false
let is_synchronized t access_expression =
match find_opt access_expression t with Some Synchronized -> true | _ -> false
let rec attribute_of_expr attribute_map (e : HilExp.t) =
match e with
| AccessExpression access_expr ->
find access_expr attribute_map
get access_expr attribute_map
| Constant _ ->
Attribute.Functional
| Exception expr (* treat exceptions as transparent wrt attributes *) | Cast (_, expr) ->
@ -508,21 +513,28 @@ type summary =
; locks: LockDomain.t
; accesses: AccessDomain.t
; return_ownership: OwnershipAbstractValue.t
; return_attribute: Attribute.t }
; return_attribute: Attribute.t
; attributes: AttributeMapDomain.t }
let empty_summary =
{ threads= ThreadsDomain.bottom
; locks= LockDomain.bottom
; accesses= AccessDomain.bottom
; return_ownership= OwnershipAbstractValue.unowned
; return_attribute= Attribute.top }
; return_attribute= Attribute.top
; attributes= AttributeMapDomain.top }
let pp_summary fmt {threads; locks; accesses; return_ownership; return_attribute} =
let pp_summary fmt {threads; locks; accesses; return_ownership; return_attribute; attributes} =
F.fprintf fmt
"@\nThreads: %a, Locks: %a @\nAccesses %a @\nOwnership: %a @\nReturn Attributes: %a @\n"
"@\n\
Threads: %a, Locks: %a @\n\
Accesses %a @\n\
Ownership: %a @\n\
Return Attribute: %a @\n\
Attributes: %a @\n"
ThreadsDomain.pp threads LockDomain.pp locks AccessDomain.pp accesses OwnershipAbstractValue.pp
return_ownership Attribute.pp return_attribute
return_ownership Attribute.pp return_attribute AttributeMapDomain.pp attributes
let pp fmt {threads; locks; accesses; ownership; attribute_map} =
@ -548,16 +560,25 @@ let add_unannotated_call_access formals pname actuals loc (astate : t) =
{astate with accesses= AccessDomain.add_opt snapshot astate.accesses} )
let astate_to_summary proc_desc {threads; locks; accesses; ownership; attribute_map} =
let astate_to_summary proc_desc formals {threads; locks; accesses; ownership; attribute_map} =
let proc_name = Procdesc.get_proc_name proc_desc in
let return_var_exp =
AccessExpression.base
(Var.of_pvar (Pvar.get_ret_pvar proc_name), Procdesc.get_ret_type proc_desc)
in
let return_ownership = OwnershipDomain.get_owned return_var_exp ownership in
let return_attribute = AttributeMapDomain.find return_var_exp attribute_map in
let return_attribute = AttributeMapDomain.get return_var_exp attribute_map in
let locks =
(* if method is [synchronized] released the lock once. *)
if Procdesc.is_java_synchronized proc_desc then LockDomain.release_lock locks else locks
in
{threads; locks; accesses; return_ownership; return_attribute}
let attributes =
(* store only the [Synchronized] attribute for class initializers/constructors *)
if Procname.is_java_class_initializer proc_name || Procname.is_constructor proc_name then
AttributeMapDomain.filter
(fun exp attribute ->
match attribute with Synchronized -> should_keep_exp formals exp | _ -> false )
attribute_map
else AttributeMapDomain.top
in
{threads; locks; accesses; return_ownership; return_attribute; attributes}

@ -163,17 +163,20 @@ module Attribute : sig
| Functional (** holds a value returned from a callee marked [@Functional] *)
| OnMainThread (** boolean is true if the current procedure is running on the main thread *)
| LockHeld (** boolean is true if a lock is currently held *)
| Synchronized (** the object is a synchronized data structure *)
end
module AttributeMapDomain : sig
type t
val find : AccessExpression.t -> t -> Attribute.t
include
AbstractDomain.InvertedMapS with type key = AccessExpression.t and type value = Attribute.t
val add : AccessExpression.t -> Attribute.t -> t -> t
val get : AccessExpression.t -> t -> Attribute.t
(** find the [Attribute.t] associated with a given access expression or return [Attribute.bottom] *)
val is_functional : t -> AccessExpression.t -> bool
val is_synchronized : t -> AccessExpression.t -> bool
val propagate_assignment : AccessExpression.t -> HilExp.t -> t -> t
(** propagate attributes from the leaves to the root of an RHS Hil expression *)
end
@ -199,10 +202,11 @@ type summary =
; locks: LockDomain.t
; accesses: AccessDomain.t
; return_ownership: OwnershipAbstractValue.t
; return_attribute: Attribute.t }
; return_attribute: Attribute.t
; attributes: AttributeMapDomain.t }
val empty_summary : summary
val pp_summary : F.formatter -> summary -> unit
val astate_to_summary : Procdesc.t -> t -> summary
val astate_to_summary : Procdesc.t -> FormalMap.t -> t -> summary

@ -406,20 +406,27 @@ let should_flag_interface_call tenv exps call_flags pname =
false
let synchronized_container_classes =
[ "android.support.v4.util.Pools$SynchronizedPool"
; "androidx.core.util.Pools$SynchronizedPool"
; "java.util.concurrent.ConcurrentMap"
; "java.util.concurrent.CopyOnWriteArrayList" ]
let is_synchronized_container_constructor =
let open MethodMatcher in
let default = {default with methods= [Procname.Java.constructor_method_name]} in
List.map synchronized_container_classes ~f:(fun classname -> {default with classname})
|> of_records
let is_synchronized_container callee_pname (access_exp : HilExp.AccessExpression.t) tenv =
let is_threadsafe_collection pn tenv =
match pn with
| Procname.Java java_pname ->
let typename = Procname.Java.get_class_type_name java_pname in
let aux tn _ =
match Typ.Name.name tn with
| "java.util.concurrent.ConcurrentMap"
| "java.util.concurrent.CopyOnWriteArrayList"
| "android.support.v4.util.Pools$SynchronizedPool"
| "androidx.core.util.Pools$SynchronizedPool" ->
true
| _ ->
false
List.mem synchronized_container_classes ~equal:String.equal (Typ.Name.name tn)
in
PatternMatch.supertype_exists tenv aux typename
| _ ->

@ -64,3 +64,5 @@ val is_builder_passthrough : Procname.t -> bool
val is_initializer : Tenv.t -> Procname.t -> bool
(** should the given procedure be treated as a constructor/initializer? *)
val is_synchronized_container_constructor : Tenv.t -> Procname.t -> HilExp.t list -> bool

@ -292,16 +292,16 @@ module Analyzer = LowerHil.MakeAbstractInterpreter (TransferFunctions (ProcCfg.N
let set_class_init_attributes procname (astate : Domain.t) =
let open Domain in
let attributes =
Procname.get_class_type_name procname
|> Option.map ~f:(fun tname -> Procname.(Java (Java.get_class_initializer tname)))
|> Option.bind ~f:Payload.read_toplevel_procedure
ConcurrencyUtils.get_java_class_initializer_summary_of procname
|> Option.bind ~f:Payload.of_summary
|> Option.value_map ~default:AttributeDomain.top ~f:(fun summary -> summary.attributes)
in
({astate with attributes} : t)
(** Compute the attributes of instance variables that all constructors agree on. *)
let set_constructor_attributes tenv procname (astate : Domain.t) =
let set_constructor_attributes tenv summary (astate : Domain.t) =
let procname = Summary.get_proc_name summary in
let open Domain in
(* make a local [this] variable, for replacing all constructor attribute map keys' roots *)
let local_this = Pvar.mk Mangled.this procname |> Var.of_pvar in
@ -319,15 +319,8 @@ let set_constructor_attributes tenv procname (astate : Domain.t) =
AttributeDomain.(fold (fun exp attr acc -> add (make_local exp) attr acc) attributes empty)
in
let attributes =
Procname.get_class_type_name procname
(* retrieve its definition *)
|> Option.bind ~f:(Tenv.lookup tenv)
(* get the list of methods in the class *)
|> Option.value_map ~default:[] ~f:(fun (tstruct : Struct.t) -> tstruct.methods)
(* keep only the constructors *)
|> List.filter ~f:Procname.(function Java jname -> Java.is_constructor jname | _ -> false)
(* get the summaries of the constructors *)
|> List.filter_map ~f:Payload.read_toplevel_procedure
ConcurrencyUtils.get_java_constructor_summaries_of tenv summary
|> List.filter_map ~f:Payload.of_summary
(* make instances of [this] local to the current procedure and select only the attributes *)
|> List.map ~f:(fun (summary : Domain.summary) -> localize_attrs summary.attributes)
(* join all the attribute maps together *)
@ -337,7 +330,8 @@ let set_constructor_attributes tenv procname (astate : Domain.t) =
{astate with attributes}
let set_initial_attributes tenv procname astate =
let set_initial_attributes tenv summary astate =
let procname = Summary.get_proc_name summary in
if not Config.starvation_whole_program then astate
else
match procname with
@ -348,11 +342,11 @@ let set_initial_attributes tenv procname astate =
when Procname.Java.(is_constructor java_pname || is_static java_pname) ->
(* analyzing a constructor or static method, so we need the attributes established by the
class initializer *)
set_class_init_attributes procname astate
set_class_init_attributes summary astate
| Procname.Java _ ->
(* we are analyzing an instance method, so we need constructor-established attributes
which will include those by the class initializer *)
set_constructor_attributes tenv procname astate
set_constructor_attributes tenv summary astate
| _ ->
astate
@ -391,7 +385,7 @@ let analyze_procedure {Callbacks.exe_env; summary} =
let initial =
Domain.bottom
(* set the attributes of instance variables set up by all constructors or the class initializer *)
|> set_initial_attributes tenv procname
|> set_initial_attributes tenv summary
|> set_lock_state_for_synchronized_proc |> set_thread_status_by_annotation
in
Analyzer.compute_post proc_data ~initial

@ -308,4 +308,10 @@ class Containers {
Map<String, String> alias = mAliasedMap;
return alias.get("a");
}
Map<String, String> mConcurrentMap = new ConcurrentHashMap<String, String>();
void dynamicallyTypedConcurrentMapPutOk(String key, String value) {
mConcurrentMap.put(key, value);
}
}

Loading…
Cancel
Save