From 27d8a659067b4c19ba9e005c6196b932f7fee46b Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 7 Dec 2018 05:25:55 -0800 Subject: [PATCH] [starvation][concurrency] split guard lock treatment and add support for non-recursive locks (per language) Reviewed By: jeremydubreil Differential Revision: D13320542 fbshipit-source-id: feb5d58a8 --- infer/src/concurrency/ConcurrencyModels.ml | 172 ++++++++++++------ infer/src/concurrency/ConcurrencyModels.mli | 14 +- infer/src/concurrency/RacerD.ml | 8 +- infer/src/concurrency/starvation.ml | 63 +++++-- infer/src/concurrency/starvationDomain.ml | 113 +++++++++--- infer/src/concurrency/starvationDomain.mli | 25 ++- .../codetoanalyze/cpp/starvation/basics.cpp | 74 +++++++- .../codetoanalyze/cpp/starvation/issues.exp | 10 +- .../cpp/starvation/recursive.cpp | 48 +++++ 9 files changed, 402 insertions(+), 125 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/starvation/recursive.cpp diff --git a/infer/src/concurrency/ConcurrencyModels.ml b/infer/src/concurrency/ConcurrencyModels.ml index 31834ce41..bfb8dc2a2 100644 --- a/infer/src/concurrency/ConcurrencyModels.ml +++ b/infer/src/concurrency/ConcurrencyModels.ml @@ -8,11 +8,17 @@ open! IStd module F = Format module MF = MarkupFormatter +module L = Logging type lock_effect = | Lock of HilExp.t list | Unlock of HilExp.t list | LockedIfTrue of HilExp.t list + | GuardConstruct of {guard: HilExp.t; lock: HilExp.t; acquire_now: bool} + | GuardLock of HilExp.t + | GuardLockedIfTrue of HilExp.t + | GuardUnlock of HilExp.t + | GuardDestroy of HilExp.t | NoEffect type thread = BackgroundThread | MainThread | MainThreadIfTrue | UnknownThread @@ -77,96 +83,142 @@ end = struct ; {shd with cls= "folly::SharedMutex"} ; {shd with cls= "folly::SharedMutexImpl"} ; {def with cls= "folly::SpinLock"} - ; {def with cls= "std::shared_lock"} ; {def with cls= "std::mutex"} - ; shd - ; {def with cls= "std::unique_lock"; tlk= "owns_lock" :: def.tlk} ] + ; shd ] - let mk_model_matcher ~f = - let lock_methods = - List.concat_map lock_models ~f:(fun mdl -> - List.map (f mdl) ~f:(fun mtd -> mdl.cls ^ "::" ^ mtd) ) - in - let matcher = QualifiedCppName.Match.of_fuzzy_qual_names lock_methods in + let mk_matcher methods = + let matcher = QualifiedCppName.Match.of_fuzzy_qual_names methods in fun pname -> QualifiedCppName.Match.match_qualifiers matcher (Typ.Procname.get_qualifiers pname) - let is_lock = mk_model_matcher ~f:(fun mdl -> mdl.lck) - - let is_unlock = mk_model_matcher ~f:(fun mdl -> mdl.unl) + let is_lock, is_unlock, is_trylock, is_std_lock = + (* TODO std::try_lock *) + let mk_model_matcher ~f = + let lock_methods = + List.concat_map lock_models ~f:(fun mdl -> + List.map (f mdl) ~f:(fun mtd -> mdl.cls ^ "::" ^ mtd) ) + in + mk_matcher lock_methods + in + ( mk_model_matcher ~f:(fun mdl -> mdl.lck) + , mk_model_matcher ~f:(fun mdl -> mdl.unl) + , mk_model_matcher ~f:(fun mdl -> mdl.tlk) + , mk_matcher ["std::lock"] ) - let is_trylock = mk_model_matcher ~f:(fun mdl -> mdl.tlk) let lock_types_matcher = let class_names = List.map lock_models ~f:(fun mdl -> mdl.cls) in QualifiedCppName.Match.of_fuzzy_qual_names class_names - (* TODO std::scoped_lock *) - + (** C++ guard classes used for scope-based lock management. + NB we pretend all classes below implement the mutex interface even though only + [shared_lock] and [unique_lock] do, for simplicity. The comments summarise which + methods are implemented. *) let guards = - [ "apache::thrift::concurrency::Guard" - ; "apache::thrift::concurrency::RWGuard" - ; "folly::SharedMutex::ReadHolder" - ; "folly::SharedMutex::WriteHolder" - ; "folly::LockedPtr" - ; "folly::SpinLockGuard" - ; "std::lock_guard" - ; "std::shared_lock" - ; "std::unique_lock" ] - - - let get_guard_constructor, get_guard_destructor = + (* TODO std::scoped_lock *) + [ (* no lock/unlock *) + "apache::thrift::concurrency::Guard" + ; (* no lock/unlock *) + "apache::thrift::concurrency::RWGuard" + ; (* only unlock *) + "folly::SharedMutex::ReadHolder" + ; (* only unlock *) + "folly::SharedMutex::WriteHolder" + ; (* read/write locks under operator() etc *) + "folly::LockedPtr" + ; (* no lock/unlock *) + "folly::SpinLockGuard" + ; (* no lock/unlock *) + "std::lock_guard" + ; (* everything *) + "std::shared_lock" + ; (* everything *) + "std::unique_lock" ] + + + let ( get_guard_constructor + , get_guard_destructor + , get_guard_lock + , get_guard_unlock + , get_guard_trylock ) = let get_class_and_qual_name guard = let qual_name = QualifiedCppName.of_qual_string guard in let class_name, _ = Option.value_exn (QualifiedCppName.extract_last qual_name) in (class_name, qual_name) in - ( (fun guard -> - let class_name, qual_name = get_class_and_qual_name guard in - let qual_constructor = QualifiedCppName.append_qualifier qual_name ~qual:class_name in - QualifiedCppName.to_qual_string qual_constructor ) - , fun guard -> - let class_name, qual_name = get_class_and_qual_name guard in - let qual_destructor = - QualifiedCppName.append_qualifier qual_name ~qual:("~" ^ class_name) - in - QualifiedCppName.to_qual_string qual_destructor ) - - - let is_guard_lock = - let constructors = List.map guards ~f:get_guard_constructor in - let matcher = QualifiedCppName.Match.of_fuzzy_qual_names constructors in - fun pname actuals -> - QualifiedCppName.Match.match_qualifiers matcher (Typ.Procname.get_qualifiers pname) - (* Passing additional parameter allows to defer the lock *) - && Int.equal 2 (List.length actuals) - - - let is_guard_unlock = - let destructors = List.map guards ~f:get_guard_destructor in - let matcher = QualifiedCppName.Match.of_fuzzy_qual_names destructors in - fun pname -> - QualifiedCppName.Match.match_qualifiers matcher (Typ.Procname.get_qualifiers pname) + let make_with_classname ~f guard = + let class_name, qual_name = get_class_and_qual_name guard in + let qual = f class_name in + let qual_constructor = QualifiedCppName.append_qualifier qual_name ~qual in + QualifiedCppName.to_qual_string qual_constructor + in + let make_lock_unlock ~mthd guard = + let qual_name = QualifiedCppName.of_qual_string guard in + let qual_mthd = QualifiedCppName.append_qualifier qual_name ~qual:mthd in + QualifiedCppName.to_qual_string qual_mthd + in + let make_trylock ~mthds guard = + let qual_name = QualifiedCppName.of_qual_string guard in + List.map mthds ~f:(fun qual -> + QualifiedCppName.append_qualifier qual_name ~qual |> QualifiedCppName.to_qual_string ) + in + ( make_with_classname ~f:(fun class_name -> class_name) + , make_with_classname ~f:(fun class_name -> "~" ^ class_name) + , make_lock_unlock ~mthd:"lock" + , make_lock_unlock ~mthd:"unlock" + , make_trylock ~mthds:["try_lock"; "owns_lock"] ) - let is_std_lock = - let matcher = QualifiedCppName.Match.of_fuzzy_qual_names ["std::lock"] in - fun pname -> - QualifiedCppName.Match.match_qualifiers matcher (Typ.Procname.get_qualifiers pname) + let is_guard_constructor, is_guard_destructor, is_guard_unlock, is_guard_lock, is_guard_trylock = + let make ~f = + let constructors = List.map guards ~f in + mk_matcher constructors + in + let make_trylock ~f = + let methods = List.concat_map guards ~f in + mk_matcher methods + in + ( make ~f:get_guard_constructor + , make ~f:get_guard_destructor + , make ~f:get_guard_unlock + , make ~f:get_guard_lock + , make_trylock ~f:get_guard_trylock ) let get_lock_effect pname actuals = + let log_parse_error error = + L.internal_error "%s pname:%a actuals:%a@." error Typ.Procname.pp pname + (PrettyPrintable.pp_collection ~pp_item:HilExp.pp) + actuals + in + let guard_action ~f ~error = + match actuals with [guard] -> f guard | _ -> log_parse_error error ; NoEffect + in let fst_arg = match actuals with x :: _ -> [x] | _ -> [] in - let snd_arg = match actuals with _ :: x :: _ -> [x] | _ -> [] in if is_std_lock pname then Lock actuals else if is_lock pname then Lock fst_arg - else if is_guard_lock pname actuals then Lock snd_arg else if is_unlock pname then Unlock fst_arg - else if is_guard_unlock pname then Unlock snd_arg else if is_trylock pname then LockedIfTrue fst_arg + else if is_guard_constructor pname then ( + match actuals with + | [guard; lock] -> + GuardConstruct {guard; lock; acquire_now= true} + | [guard; lock; _defer_lock] -> + GuardConstruct {guard; lock; acquire_now= false} + | _ -> + log_parse_error "Cannot parse guard constructor call" ; + NoEffect ) + else if is_guard_lock pname then + guard_action ~f:(fun guard -> GuardLock guard) ~error:"Can't parse guard lock" + else if is_guard_unlock pname then + guard_action ~f:(fun guard -> GuardUnlock guard) ~error:"Can't parse guard unlock" + else if is_guard_destructor pname then + guard_action ~f:(fun guard -> GuardDestroy guard) ~error:"Can't parse guard destructor" + else if is_guard_trylock pname then + guard_action ~f:(fun guard -> GuardLockedIfTrue guard) ~error:"Can't parse guard trylock" else NoEffect end diff --git a/infer/src/concurrency/ConcurrencyModels.mli b/infer/src/concurrency/ConcurrencyModels.mli index 1b3a74369..847fb3cf1 100644 --- a/infer/src/concurrency/ConcurrencyModels.mli +++ b/infer/src/concurrency/ConcurrencyModels.mli @@ -10,10 +10,16 @@ module F = Format (** effect of call plus Hil expressions being un/locked, if known *) type lock_effect = - | Lock of HilExp.t list - | Unlock of HilExp.t list - | LockedIfTrue of HilExp.t list - | NoEffect + | Lock of HilExp.t list (** simultaneously acquire a list of locks *) + | Unlock of HilExp.t list (** simultaneously release a list of locks *) + | LockedIfTrue of HilExp.t list (** simultaneously attempt to acquire a list of locks *) + | GuardConstruct of {guard: HilExp.t; lock: HilExp.t; acquire_now: bool} + (** mutex guard construction - clang only *) + | GuardLock of HilExp.t (** lock underlying mutex via guard - clang only *) + | GuardLockedIfTrue of HilExp.t (** lock underlying mutex if true via guard - clang only *) + | GuardUnlock of HilExp.t (** unlock underlying mutex via guard - clang only *) + | GuardDestroy of HilExp.t (** destroy guard and unlock underlying mutex - clang only *) + | NoEffect (** function call has no lock-relevant effect *) type thread = BackgroundThread | MainThread | MainThreadIfTrue | UnknownThread diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index eab813e45..a92f08cd0 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -413,20 +413,22 @@ module TransferFunctions (CFG : ProcCfg.S) = struct ThreadsDomain.AnyThread in match get_lock_effect callee_pname actuals with - | Lock _ -> + | Lock _ | GuardLock _ | GuardConstruct {acquire_now= true} -> { astate with locks= LocksDomain.acquire_lock astate.locks ; threads= update_for_lock_use astate.threads } - | Unlock _ -> + | Unlock _ | GuardDestroy _ | GuardUnlock _ -> { astate with locks= LocksDomain.release_lock astate.locks ; threads= update_for_lock_use astate.threads } - | LockedIfTrue _ -> + | LockedIfTrue _ | GuardLockedIfTrue _ -> let attribute_map = AttributeMapDomain.add_attribute ret_access_path (Choice Choice.LockHeld) astate.attribute_map in {astate with attribute_map; threads= update_for_lock_use astate.threads} + | GuardConstruct {acquire_now= false} -> + astate | NoEffect -> ( let summary_opt = get_summary pdesc callee_pname actuals loc tenv astate in let callee_pdesc = Ondemand.get_proc_desc callee_pname in diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 690789c2d..8c68eb449 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -58,6 +58,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let exec_instr (astate : Domain.t) {ProcData.pdesc; tenv; extras} _ (instr : HilInstr.t) = let open ConcurrencyModels in let open StarvationModels in + let log_parse_error error pname actuals = + L.internal_error "%s pname:%a actuals:%a@." error Typ.Procname.pp pname + (PrettyPrintable.pp_collection ~pp_item:HilExp.pp) + actuals + in let get_lock_path = function | HilExp.AccessExpression access_exp -> ( match AccessExpression.to_access_path access_exp with @@ -73,25 +78,41 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | _ -> None in + let is_java = Procdesc.get_proc_name pdesc |> Typ.Procname.is_java in let do_lock locks loc astate = - List.filter_map ~f:get_lock_path locks |> Domain.acquire astate loc + List.filter_map ~f:get_lock_path locks |> Domain.acquire ~recursive_locks:is_java astate loc in let do_unlock locks astate = List.filter_map ~f:get_lock_path locks |> Domain.release astate in - let do_call callee loc = + let do_call callee loc astate = Payload.read pdesc callee - |> Option.value_map ~default:astate ~f:(Domain.integrate_summary astate callee loc) + |> Option.value_map ~default:astate + ~f:(Domain.integrate_summary ~recursive_locks:is_java astate callee loc) in - let is_java = Procdesc.get_proc_name pdesc |> Typ.Procname.is_java in match instr with + | Assign _ | Assume _ | Call (_, Indirect _, _, _, _) | ExitScope _ -> + astate + | Call (_, Direct callee, actuals, _, _) when should_skip_analysis tenv callee actuals -> + astate | Call (_, Direct callee, actuals, _, loc) -> ( match get_lock_effect callee actuals with | Lock locks -> do_lock locks loc astate + | GuardLock guard -> + Domain.lock_guard ~recursive_locks:is_java astate guard loc + | GuardConstruct {guard; lock; acquire_now} -> ( + match get_lock_path lock with + | Some lock_path -> + Domain.add_guard astate guard lock_path ~acquire_now ~recursive_locks:false loc + | None -> + log_parse_error "Couldn't parse lock in guard constructor" callee actuals ; + astate ) | Unlock locks -> do_unlock locks astate - | LockedIfTrue _ -> - astate - | NoEffect when should_skip_analysis tenv callee actuals -> + | GuardUnlock guard -> + Domain.unlock_guard astate guard + | GuardDestroy guard -> + Domain.remove_guard astate guard + | LockedIfTrue _ | GuardLockedIfTrue _ -> astate | NoEffect when is_synchronized_library_call tenv callee -> (* model a synchronized call without visible internal behaviour *) @@ -102,17 +123,14 @@ module TransferFunctions (CFG : ProcCfg.S) = struct Domain.set_on_ui_thread astate loc explanation | NoEffect when is_java && StarvationModels.is_strict_mode_violation tenv callee actuals -> Domain.strict_mode_call callee loc astate - | NoEffect when is_java -> ( - match may_block tenv callee actuals with - | Some sev -> - Domain.blocking_call callee sev loc astate - | None -> - do_call callee loc ) | NoEffect -> - (* in C++/Obj C we only care about deadlocks, not starvation errors *) - do_call callee loc ) - | _ -> - astate + if is_java then + may_block tenv callee actuals + |> Option.map ~f:(fun sev -> Domain.blocking_call callee sev loc astate) + |> IOption.value_default_f ~f:(fun () -> do_call callee loc astate) + else + (* in C++/Obj C we only care about deadlocks, not starvation errors *) + do_call callee loc astate ) let pp_session_name _node fmt = F.pp_print_string fmt "starvation" @@ -126,6 +144,7 @@ let analyze_procedure {Callbacks.proc_desc; tenv; summary} = let formals = FormalMap.make proc_desc in let proc_data = ProcData.make proc_desc tenv formals in let loc = Procdesc.get_loc proc_desc in + let recursive_locks = Procdesc.get_proc_name proc_desc |> Typ.Procname.is_java in let initial = if not (Procdesc.is_java_synchronized proc_desc) then StarvationDomain.empty else @@ -138,7 +157,7 @@ let analyze_procedure {Callbacks.proc_desc; tenv; summary} = | _ -> FormalMap.get_formal_base 0 formals |> Option.map ~f:(fun base -> (base, [])) in - StarvationDomain.acquire StarvationDomain.empty loc (Option.to_list lock) + StarvationDomain.acquire ~recursive_locks StarvationDomain.empty loc (Option.to_list lock) in let initial = ConcurrencyModels.runs_on_ui_thread tenv proc_desc @@ -379,6 +398,14 @@ let report_deadlocks env {StarvationDomain.order; ui} report_map' = match elem.Order.elem.eventually.elem with | MayBlock _ | StrictModeCall _ -> report_map + | LockAcquire endpoint_lock when Lock.equal endpoint_lock elem.Order.elem.first -> + let error_message = + Format.asprintf "Potential self deadlock. %a %a twice." pname_pp current_pname Lock.pp + endpoint_lock + in + let ltr = Order.make_trace ~header:"In method" current_pname elem in + let loc = Order.get_loc elem in + ReportMap.add_deadlock current_pname loc ltr error_message report_map | LockAcquire endpoint_lock -> Lock.owner_class endpoint_lock |> Option.value_map ~default:report_map ~f:(fun endpoint_class -> diff --git a/infer/src/concurrency/starvationDomain.ml b/infer/src/concurrency/starvationDomain.ml index 74e31336e..279a2b7de 100644 --- a/infer/src/concurrency/starvationDomain.ml +++ b/infer/src/concurrency/starvationDomain.ml @@ -11,6 +11,7 @@ module MF = MarkupFormatter let pname_pp = MF.wrap_monospaced Typ.Procname.pp module Lock = struct + (* TODO (T37174859): change to [HilExp.t] *) type t = AccessPath.t (* compare type, base variable modulo this and access list *) @@ -198,29 +199,46 @@ module UIThreadDomain = struct (UIThreadExplanationDomain.with_callsite ui_explain callsite) end +module FlatLock = AbstractDomain.Flat (Lock) + +module GuardToLockMap = struct + include AbstractDomain.InvertedMap (HilExp) (FlatLock) + + let remove_guard astate guard = remove guard astate + + let add_guard astate ~guard ~lock = add guard (FlatLock.v lock) astate +end + type t = - {lock_state: LockState.t; events: EventDomain.t; order: OrderDomain.t; ui: UIThreadDomain.t} + { events: EventDomain.t + ; guard_map: GuardToLockMap.t + ; lock_state: LockState.t + ; order: OrderDomain.t + ; ui: UIThreadDomain.t } let empty = - { lock_state= LockState.empty - ; events= EventDomain.empty + { events= EventDomain.empty + ; guard_map= GuardToLockMap.empty + ; lock_state= LockState.empty ; order= OrderDomain.empty ; ui= UIThreadDomain.empty } -let is_empty {lock_state; events; order; ui} = - UIThreadDomain.is_empty ui && EventDomain.is_empty events && OrderDomain.is_empty order - && LockState.is_empty lock_state +let is_empty {events; guard_map; lock_state; order; ui} = + EventDomain.is_empty events && GuardToLockMap.is_empty guard_map && OrderDomain.is_empty order + && LockState.is_empty lock_state && UIThreadDomain.is_empty ui -let pp fmt {lock_state; events; order; ui} = - F.fprintf fmt "{lock_state= %a; events= %a; order= %a; ui= %a}" LockState.pp lock_state - EventDomain.pp events OrderDomain.pp order UIThreadDomain.pp ui +let pp fmt {events; guard_map; lock_state; order; ui} = + F.fprintf fmt "{events= %a; guard_map= %a; lock_state= %a; order= %a; ui= %a}" EventDomain.pp + events GuardToLockMap.pp guard_map LockState.pp lock_state OrderDomain.pp order + UIThreadDomain.pp ui let join lhs rhs = - { lock_state= LockState.join lhs.lock_state rhs.lock_state - ; events= EventDomain.join lhs.events rhs.events + { events= EventDomain.join lhs.events rhs.events + ; guard_map= GuardToLockMap.join lhs.guard_map rhs.guard_map + ; lock_state= LockState.join lhs.lock_state rhs.lock_state ; order= OrderDomain.join lhs.order rhs.order ; ui= UIThreadDomain.join lhs.ui rhs.ui } @@ -228,16 +246,17 @@ let join lhs rhs = let widen ~prev ~next ~num_iters:_ = join prev next let ( <= ) ~lhs ~rhs = - UIThreadDomain.( <= ) ~lhs:lhs.ui ~rhs:rhs.ui - && EventDomain.( <= ) ~lhs:lhs.events ~rhs:rhs.events + EventDomain.( <= ) ~lhs:lhs.events ~rhs:rhs.events + && GuardToLockMap.( <= ) ~lhs:lhs.guard_map ~rhs:rhs.guard_map && OrderDomain.( <= ) ~lhs:lhs.order ~rhs:rhs.order && LockState.( <= ) ~lhs:lhs.lock_state ~rhs:rhs.lock_state + && UIThreadDomain.( <= ) ~lhs:lhs.ui ~rhs:rhs.ui (* for every lock b held locally, add a pair (b, event) *) -let add_order_pairs lock_state event acc = +let add_order_pairs ~recursive_locks lock_state event acc = (* add no pairs whatsoever if we already hold that lock *) - if LockState.is_taken event lock_state then acc + if recursive_locks && LockState.is_taken event lock_state then acc else let add_first_and_eventually acc f = match f.Event.elem with @@ -250,28 +269,32 @@ let add_order_pairs lock_state event acc = LockState.fold_over_events add_first_and_eventually lock_state acc -let acquire ({lock_state; events; order} as astate) loc locks = +let acquire ~recursive_locks ({lock_state; events; order} as astate) loc locks = let new_events = List.map locks ~f:(fun lock -> Event.make_acquire lock loc) in { astate with events= List.fold new_events ~init:events ~f:(fun acc e -> EventDomain.add e acc) ; order= List.fold new_events ~init:order ~f:(fun acc e -> - OrderDomain.union acc (add_order_pairs lock_state e order) ) + OrderDomain.union acc (add_order_pairs ~recursive_locks lock_state e order) ) ; lock_state= List.fold2_exn locks new_events ~init:lock_state ~f:(fun acc lock e -> LockState.acquire lock e acc ) } -let blocking_call callee sev loc ({lock_state; events; order} as astate) = - let new_event = Event.make_blocking_call callee sev loc in +let make_call_with_event new_event astate = { astate with - events= EventDomain.add new_event events; order= add_order_pairs lock_state new_event order } + events= EventDomain.add new_event astate.events + ; order= add_order_pairs ~recursive_locks:false astate.lock_state new_event astate.order } + +let blocking_call callee sev loc astate = + let new_event = Event.make_blocking_call callee sev loc in + make_call_with_event new_event astate -let strict_mode_call callee loc ({lock_state; events; order} as astate) = + +let strict_mode_call callee loc astate = let new_event = Event.make_strict_mode_call callee loc in - { astate with - events= EventDomain.add new_event events; order= add_order_pairs lock_state new_event order } + make_call_with_event new_event astate let release ({lock_state} as astate) locks = @@ -279,20 +302,27 @@ let release ({lock_state} as astate) locks = lock_state= List.fold locks ~init:lock_state ~f:(fun acc l -> LockState.release l acc) } -let integrate_summary ({lock_state; events; order; ui} as astate) callee_pname loc callee_summary = +let integrate_summary ~recursive_locks ({lock_state; events; order; ui} as astate) callee_pname loc + callee_summary = let callsite = CallSite.make callee_pname loc in let callee_order = OrderDomain.with_callsite callee_summary.order callsite in let callee_ui = UIThreadDomain.with_callsite callee_summary.ui callsite in let filtered_order = - OrderDomain.filter - (fun {elem= {eventually}} -> LockState.is_taken eventually lock_state |> not) - callee_order + if recursive_locks then + OrderDomain.filter + (fun {elem= {eventually}} -> LockState.is_taken eventually lock_state |> not) + callee_order + else callee_order in let callee_events = EventDomain.with_callsite callee_summary.events callsite in let filtered_events = - EventDomain.filter (fun e -> LockState.is_taken e lock_state |> not) callee_events + if recursive_locks then + EventDomain.filter (fun e -> LockState.is_taken e lock_state |> not) callee_events + else callee_events + in + let order' = + EventDomain.fold (add_order_pairs ~recursive_locks lock_state) filtered_events filtered_order in - let order' = EventDomain.fold (add_order_pairs lock_state) filtered_events filtered_order in { astate with events= EventDomain.join events filtered_events ; order= OrderDomain.join order order' @@ -307,6 +337,31 @@ let set_on_ui_thread ({ui} as astate) loc explain = {astate with ui} +let add_guard astate guard lock ~acquire_now ~recursive_locks loc = + let astate = {astate with guard_map= GuardToLockMap.add_guard ~guard ~lock astate.guard_map} in + if acquire_now then acquire ~recursive_locks astate loc [lock] else astate + + +let remove_guard astate guard = + GuardToLockMap.find_opt guard astate.guard_map + |> Option.value_map ~default:astate ~f:(fun lock_opt -> + let locks = FlatLock.get lock_opt |> Option.to_list in + let astate = release astate locks in + {astate with guard_map= GuardToLockMap.remove_guard astate.guard_map guard} ) + + +let unlock_guard astate guard = + GuardToLockMap.find_opt guard astate.guard_map + |> Option.value_map ~default:astate ~f:(fun lock_opt -> + FlatLock.get lock_opt |> Option.to_list |> release astate ) + + +let lock_guard ~recursive_locks astate guard loc = + GuardToLockMap.find_opt guard astate.guard_map + |> Option.value_map ~default:astate ~f:(fun lock_opt -> + FlatLock.get lock_opt |> Option.to_list |> acquire astate ~recursive_locks loc ) + + type summary = t let pp_summary = pp diff --git a/infer/src/concurrency/starvationDomain.mli b/infer/src/concurrency/starvationDomain.mli index 745efe2a7..976c63b2d 100644 --- a/infer/src/concurrency/starvationDomain.mli +++ b/infer/src/concurrency/starvationDomain.mli @@ -69,12 +69,18 @@ module UIThreadDomain : AbstractDomain.WithBottom with type t = UIThreadExplanationDomain.t AbstractDomain.Types.bottom_lifted +module GuardToLockMap : AbstractDomain.WithBottom + type t = - {lock_state: LockState.t; events: EventDomain.t; order: OrderDomain.t; ui: UIThreadDomain.t} + { events: EventDomain.t + ; guard_map: GuardToLockMap.t + ; lock_state: LockState.t + ; order: OrderDomain.t + ; ui: UIThreadDomain.t } include AbstractDomain.WithBottom with type t := t -val acquire : t -> Location.t -> Lock.t list -> t +val acquire : recursive_locks:bool -> t -> Location.t -> Lock.t list -> t (** simultaneously acquire a number of locks, no-op if list is empty *) val release : t -> Lock.t list -> t @@ -88,8 +94,21 @@ val set_on_ui_thread : t -> Location.t -> string -> t (** set the property "runs on UI thread" to true by attaching the given explanation string as to why this method is thought to do so *) +val add_guard : + t -> HilExp.t -> Lock.t -> acquire_now:bool -> recursive_locks:bool -> Location.t -> t +(** Install a mapping from the guard expression to the lock provided, and optionally lock it. *) + +val lock_guard : recursive_locks:bool -> t -> HilExp.t -> Location.t -> t +(** Acquire the lock the guard was constructed with. *) + +val remove_guard : t -> HilExp.t -> t +(** Destroy the guard and release its lock. *) + +val unlock_guard : t -> HilExp.t -> t +(** Release the lock the guard was constructed with. *) + type summary = t val pp_summary : F.formatter -> summary -> unit -val integrate_summary : t -> Typ.Procname.t -> Location.t -> summary -> t +val integrate_summary : recursive_locks:bool -> t -> Typ.Procname.t -> Location.t -> summary -> t diff --git a/infer/tests/codetoanalyze/cpp/starvation/basics.cpp b/infer/tests/codetoanalyze/cpp/starvation/basics.cpp index b6ed4033f..21fb82db3 100644 --- a/infer/tests/codetoanalyze/cpp/starvation/basics.cpp +++ b/infer/tests/codetoanalyze/cpp/starvation/basics.cpp @@ -13,7 +13,8 @@ class Basic { public: Basic() {} - void thread1() { + // deadlock between thread1_bad() and thread2_bad() + void thread1_bad() { mutex_1.lock(); mutex_2.lock(); @@ -21,7 +22,7 @@ class Basic { mutex_1.unlock(); } - void thread2() { + void thread2_bad() { mutex_2.lock(); mutex_1.lock(); @@ -38,12 +39,33 @@ class WithGuard { public: WithGuard() {} - void thread1() { + // deadlock between thread1_bad() and thread2_bad() + void thread1_bad() { std::lock_guard lock1(mutex_1); std::lock_guard lock2(mutex_2); } - void thread2() { + void thread2_bad() { + std::lock_guard lock2(mutex_2); + std::lock_guard lock1(mutex_1); + } + + private: + std::mutex mutex_1; + std::mutex mutex_2; +}; + +class DeferredGuard { + public: + DeferredGuard() {} + + // NO deadlock between thread1_bad() and thread2_bad() + void thread1_ok() { + std::unique_lock lock1(mutex_1, std::defer_lock); + std::unique_lock lock2(mutex_2); + } + + void thread2_ok() { std::lock_guard lock2(mutex_2); std::lock_guard lock1(mutex_1); } @@ -58,13 +80,13 @@ class StdLock { StdLock() {} // no reports, std::lock magically avoids deadlocks - void thread1() { + void foo_ok() { std::lock(mutex_1, mutex_2); mutex_1.unlock(); mutex_2.unlock(); } - void thread2() { + void bar_ok() { std::lock(mutex_2, mutex_1); mutex_2.unlock(); mutex_1.unlock(); @@ -74,4 +96,44 @@ class StdLock { std::mutex mutex_1; std::mutex mutex_2; }; + +class SelfDeadlock { + public: + SelfDeadlock() {} + + void thread_bad() { + mutex_.lock(); + mutex_.lock(); + mutex_.unlock(); + mutex_.unlock(); + } + + void interproc2_bad() { std::lock_guard lock(mutex_); } + + void interproc1_bad() { + std::lock_guard lock(mutex_); + interproc2_bad(); + } + + void foo_ok() { + { std::lock_guard lock(mutex_); } + int i = 0; + { std::lock_guard lock(mutex_); } + } + + void bar_ok() { + std::unique_lock lock1(mutex_, std::defer_lock); + std::lock_guard lock2(mutex_); + } + + void complicated_bad() { + std::unique_lock lock1(mutex_, std::defer_lock); + std::lock_guard lock2(mutex_); + lock1.lock(); + } + + private: + std::mutex mutex_; +}; + } // namespace basics diff --git a/infer/tests/codetoanalyze/cpp/starvation/issues.exp b/infer/tests/codetoanalyze/cpp/starvation/issues.exp index 36989d1c8..7287cb436 100644 --- a/infer/tests/codetoanalyze/cpp/starvation/issues.exp +++ b/infer/tests/codetoanalyze/cpp/starvation/issues.exp @@ -1,2 +1,8 @@ -codetoanalyze/cpp/starvation/basics.cpp, basics::Basic_thread1, 17, DEADLOCK, no_bucket, ERROR, [[Trace 1] `basics::Basic_thread1`,locks `this.mutex_1` in class `basics::Basic*`,locks `this.mutex_2` in class `basics::Basic*`,[Trace 2] `basics::Basic_thread2`,locks `this.mutex_2` in class `basics::Basic*`,locks `this.mutex_1` in class `basics::Basic*`] -codetoanalyze/cpp/starvation/basics.cpp, basics::WithGuard_thread1, 42, DEADLOCK, no_bucket, ERROR, [[Trace 1] `basics::WithGuard_thread1`,locks `this.mutex_1` in class `basics::WithGuard*`,locks `this.mutex_2` in class `basics::WithGuard*`,[Trace 2] `basics::WithGuard_thread2`,locks `this.mutex_2` in class `basics::WithGuard*`,locks `this.mutex_1` in class `basics::WithGuard*`] +codetoanalyze/cpp/starvation/basics.cpp, basics::Basic_thread1_bad, 18, DEADLOCK, no_bucket, ERROR, [[Trace 1] `basics::Basic_thread1_bad`,locks `this.mutex_1` in class `basics::Basic*`,locks `this.mutex_2` in class `basics::Basic*`,[Trace 2] `basics::Basic_thread2_bad`,locks `this.mutex_2` in class `basics::Basic*`,locks `this.mutex_1` in class `basics::Basic*`] +codetoanalyze/cpp/starvation/basics.cpp, basics::SelfDeadlock_complicated_bad, 131, DEADLOCK, no_bucket, ERROR, [In method`basics::SelfDeadlock_complicated_bad`,locks `this.mutex_` in class `basics::SelfDeadlock*`,locks `this.mutex_` in class `basics::SelfDeadlock*`] +codetoanalyze/cpp/starvation/basics.cpp, basics::SelfDeadlock_interproc1_bad, 114, DEADLOCK, no_bucket, ERROR, [In method`basics::SelfDeadlock_interproc1_bad`,locks `this.mutex_` in class `basics::SelfDeadlock*`,Method call: `basics::SelfDeadlock_interproc2_bad`,locks `this.mutex_` in class `basics::SelfDeadlock*`] +codetoanalyze/cpp/starvation/basics.cpp, basics::SelfDeadlock_thread_bad, 105, DEADLOCK, no_bucket, ERROR, [In method`basics::SelfDeadlock_thread_bad`,locks `this.mutex_` in class `basics::SelfDeadlock*`,locks `this.mutex_` in class `basics::SelfDeadlock*`] +codetoanalyze/cpp/starvation/basics.cpp, basics::WithGuard_thread1_bad, 44, DEADLOCK, no_bucket, ERROR, [[Trace 1] `basics::WithGuard_thread1_bad`,locks `this.mutex_1` in class `basics::WithGuard*`,locks `this.mutex_2` in class `basics::WithGuard*`,[Trace 2] `basics::WithGuard_thread2_bad`,locks `this.mutex_2` in class `basics::WithGuard*`,locks `this.mutex_1` in class `basics::WithGuard*`] +codetoanalyze/cpp/starvation/recursive.cpp, Recursive_FP_multi_ok, 26, DEADLOCK, no_bucket, ERROR, [In method`Recursive_FP_multi_ok`,locks `this.recursive_mutex_` in class `Recursive*`,locks `this.recursive_mutex_` in class `Recursive*`] +codetoanalyze/cpp/starvation/recursive.cpp, Recursive_FP_path_sensitive_ok, 36, DEADLOCK, no_bucket, ERROR, [In method`Recursive_FP_path_sensitive_ok`,locks `this.mutex_` in class `Recursive*`,locks `this.mutex_` in class `Recursive*`] +codetoanalyze/cpp/starvation/recursive.cpp, Recursive_FP_unknown_ok, 31, DEADLOCK, no_bucket, ERROR, [In method`Recursive_FP_unknown_ok`,locks `this.umutex_` in class `Recursive*`,locks `this.umutex_` in class `Recursive*`] diff --git a/infer/tests/codetoanalyze/cpp/starvation/recursive.cpp b/infer/tests/codetoanalyze/cpp/starvation/recursive.cpp new file mode 100644 index 000000000..9321c9471 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/starvation/recursive.cpp @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include + +struct UnknownMutex { + UnknownMutex() {} + + void lock() {} + + void unlock() {} + + UnknownMutex(const UnknownMutex&) = delete; + UnknownMutex& operator=(const UnknownMutex&) = delete; +}; + +class Recursive { + public: + Recursive() {} + + void FP_multi_ok() { + std::lock_guard l(recursive_mutex_); + { std::lock_guard l(recursive_mutex_); } + } + + void FP_unknown_ok() { + std::lock_guard l(umutex_); + { std::lock_guard l(umutex_); } + } + + void FP_path_sensitive_ok() { + std::lock_guard l(mutex_); + bool flag = false; + + if (flag) { + std::lock_guard l(mutex_); + } + } + + private: + std::recursive_mutex recursive_mutex_; + std::mutex mutex_; + UnknownMutex umutex_; +};