From 7f6798999aa47e25cd9f34918dde466bfe47a803 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Tue, 9 Feb 2021 06:37:18 -0800 Subject: [PATCH] [starvation] warn on SettableFuture.set under lock Summary: `SettableFuture.set` invokes callbacks registered prior to the call, which may also try to acquire extra locks. If the called of `set` already holds a lock this creates lock dependencies which may lead to deadlocks. Here we warn whenever `set` is called under a lock taken in a different source file. This avoids reporting when a class internally manages locks and calls `set`, reasoning that developers will be aware this is happening. Reviewed By: jvillard Differential Revision: D25562190 fbshipit-source-id: d1b5cb69c --- .../ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md | 24 +++ infer/man/man1/infer-full.txt | 3 +- infer/man/man1/infer-report.txt | 3 +- infer/man/man1/infer.txt | 3 +- infer/src/base/IssueType.ml | 6 + infer/src/base/IssueType.mli | 2 + infer/src/concurrency/StarvationModels.ml | 6 + infer/src/concurrency/StarvationModels.mli | 3 + infer/src/concurrency/starvation.ml | 173 ++++++++++++------ infer/src/concurrency/starvationDomain.ml | 28 ++- infer/src/concurrency/starvationDomain.mli | 5 +- .../java/starvation/NotUnderLock.java | 26 +++ .../codetoanalyze/java/starvation/issues.exp | 1 + 13 files changed, 219 insertions(+), 64 deletions(-) create mode 100644 infer/documentation/issues/ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md create mode 100644 infer/tests/codetoanalyze/java/starvation/NotUnderLock.java diff --git a/infer/documentation/issues/ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md b/infer/documentation/issues/ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md new file mode 100644 index 000000000..82f771a1b --- /dev/null +++ b/infer/documentation/issues/ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md @@ -0,0 +1,24 @@ +A call which may execute arbitrary code (such as registered, or chained, callbacks) is made while a lock is held. +This code may deadlock whenever the callbacks obtain locks themselves, so it is an unsafe pattern. +This warning is issued only at the innermost lock acquisition around the final call. + +Example: +```java +public class NotUnderLock { + SettableFuture future = null; + + public void callFutureSetOk() { + future.set(null); + } + + public synchronized void firstAcquisitionBad() { + callFutureSetOk(); + } + + public void secondAcquisitionOk(Object o) { + synchronized (o) { + firstAcquisitionBad(); + } + } +} +``` diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index beae07049..45400f0af 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -382,7 +382,8 @@ OPTIONS disabling issue types does not make the corresponding checker not run. Available issue types are as follows: - ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), + ARBITRARY_CODE_EXECUTION_UNDER_LOCK (enabled by default), + ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), ARRAY_OUT_OF_BOUNDS_L2 (disabled by default), ARRAY_OUT_OF_BOUNDS_L3 (disabled by default), ASSIGN_POINTER_WARNING (enabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index cb338a650..173fa22e8 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -85,7 +85,8 @@ OPTIONS disabling issue types does not make the corresponding checker not run. Available issue types are as follows: - ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), + ARBITRARY_CODE_EXECUTION_UNDER_LOCK (enabled by default), + ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), ARRAY_OUT_OF_BOUNDS_L2 (disabled by default), ARRAY_OUT_OF_BOUNDS_L3 (disabled by default), ASSIGN_POINTER_WARNING (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index d515595eb..8b2b66c0e 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -382,7 +382,8 @@ OPTIONS disabling issue types does not make the corresponding checker not run. Available issue types are as follows: - ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), + ARBITRARY_CODE_EXECUTION_UNDER_LOCK (enabled by default), + ARRAY_OUT_OF_BOUNDS_L1 (disabled by default), ARRAY_OUT_OF_BOUNDS_L2 (disabled by default), ARRAY_OUT_OF_BOUNDS_L3 (disabled by default), ASSIGN_POINTER_WARNING (enabled by default), diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 5ac9275d4..3d127e765 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -268,6 +268,12 @@ let abduction_case_not_implemented = register_hidden ~id:"Abduction_case_not_implemented" Error Biabduction +let arbitrary_code_execution_under_lock = + register ~id:"ARBITRARY_CODE_EXECUTION_UNDER_LOCK" ~hum:"Arbitrary Code Execution Under lock" + Error Starvation + ~user_documentation:[%blob "../../documentation/issues/ARBITRARY_CODE_EXECUTION_UNDER_LOCK.md"] + + let array_of_pointsto = register_hidden ~id:"Array_of_pointsto" Error Biabduction let array_out_of_bounds_l1 = diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index ca6de5e76..5a6e258b9 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -69,6 +69,8 @@ val set_enabled : t -> bool -> unit val abduction_case_not_implemented : t +val arbitrary_code_execution_under_lock : t + val array_of_pointsto : t val array_out_of_bounds_l1 : t diff --git a/infer/src/concurrency/StarvationModels.ml b/infer/src/concurrency/StarvationModels.ml index be8d95d4e..341c9e6be 100644 --- a/infer/src/concurrency/StarvationModels.ml +++ b/infer/src/concurrency/StarvationModels.ml @@ -416,3 +416,9 @@ let is_java_main_method (pname : Procname.t) = && String.equal "main" (Procname.get_method pname) && Typ.equal StdTyp.void (Procname.Java.get_return_typ java_pname) && check_main_args (Procname.Java.get_parameters java_pname) + + +let may_execute_arbitrary_code = + let open MethodMatcher in + of_record + {default with classname= "com.google.common.util.concurrent.SettableFuture"; methods= ["set"]} diff --git a/infer/src/concurrency/StarvationModels.mli b/infer/src/concurrency/StarvationModels.mli index 7998a7258..5fdf71a80 100644 --- a/infer/src/concurrency/StarvationModels.mli +++ b/infer/src/concurrency/StarvationModels.mli @@ -79,3 +79,6 @@ val is_assume_true : Tenv.t -> Procname.t -> HilExp.t list -> bool val is_java_main_method : Procname.t -> bool (** does the method look like a Java [main] *) + +val may_execute_arbitrary_code : Tenv.t -> Procname.t -> HilExp.t list -> bool +(** for example [com.google.common.util.concurrent.SettableFuture.set] *) diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 569529f41..96befa192 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -206,6 +206,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct List.hd actuals |> Option.map ~f:(fun exp -> do_assume exp astate) else None in + let treat_arbitrary_code_exec () = + if StarvationModels.may_execute_arbitrary_code tenv callee actuals then + StarvationDomain.arbitrary_code_execution ~callee ~loc astate |> Option.some + else None + in (* constructor calls are special-cased because they side-effect the receiver and do not return anything *) let treat_modeled_summaries () = @@ -221,7 +226,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct Domain.integrate_summary ~tenv ~lhs ~subst callsite astate summary ) in IList.eval_until_first_some - [treat_handler_constructor; treat_thread_constructor; treat_assume; treat_modeled_summaries] + [ treat_handler_constructor + ; treat_thread_constructor + ; treat_assume + ; treat_arbitrary_code_exec + ; treat_modeled_summaries ] |> Option.value ~default:astate @@ -458,6 +467,8 @@ module ReportMap : sig val add_lockless_violation : report_add_t + val add_arbitrary_code_execution_under_lock : report_add_t + val issue_log_of : t -> IssueLog.t val store_multi_file : t -> unit @@ -469,6 +480,7 @@ end = struct | Deadlock of int | StrictModeViolation of int | LocklessViolation of int + | ArbitraryCodeExecutionUnderLock of int let issue_type_of_problem = function | Deadlock _ -> @@ -479,6 +491,8 @@ end = struct IssueType.strict_mode_violation | LocklessViolation _ -> IssueType.lockless_violation + | ArbitraryCodeExecutionUnderLock _ -> + IssueType.arbitrary_code_execution_under_lock type report_t = {problem: problem; pname: Procname.t; ltr: Errlog.loc_trace; message: string} @@ -522,6 +536,14 @@ end = struct add tenv pattrs loc report map + let add_arbitrary_code_execution_under_lock tenv pattrs loc ltr message (map : t) = + let pname = ProcAttributes.get_proc_name pattrs in + let report = + {problem= ArbitraryCodeExecutionUnderLock (-List.length ltr); pname; ltr; message} + in + add tenv pattrs loc report map + + let issue_log_of loc_map = let log_report ~issue_log loc {problem; pname; ltr; message} = let issue_type = issue_type_of_problem problem in @@ -565,6 +587,12 @@ end = struct | _ -> None in + let filter_arbitrary_code_execution_under_lock = function + | {problem= ArbitraryCodeExecutionUnderLock l} as r -> + Some (l, r) + | _ -> + None + in let compare_reports weight_compare (w, r) (w', r') = match weight_compare w w' with 0 -> String.compare r.message r'.message | result -> result in @@ -573,10 +601,14 @@ end = struct let starvations = List.filter_map problems ~f:filter_map_starvation in let strict_mode_violations = List.filter_map problems ~f:filter_map_strict_mode_violation in let lockless_violations = List.filter_map problems ~f:filter_map_lockless_violation in + let arbitrary_code_executions_under_lock = + List.filter_map problems ~f:filter_arbitrary_code_execution_under_lock + in log_reports (compare_reports Int.compare) loc deadlocks issue_log |> log_reports (compare_reports Int.compare) loc lockless_violations |> log_reports (compare_reports StarvationModels.compare_severity) loc starvations |> log_reports (compare_reports Int.compare) loc strict_mode_violations + |> log_reports (compare_reports Int.compare) loc arbitrary_code_executions_under_lock in Location.Map.fold log_location loc_map IssueLog.empty @@ -601,8 +633,8 @@ let should_report_deadlock_on_current_proc current_elem endpoint_elem = (not Config.deduplicate) || match (endpoint_elem.CriticalPair.elem.event, current_elem.CriticalPair.elem.event) with - | _, (MayBlock _ | StrictModeCall _ | MonitorWait _) - | (MayBlock _ | StrictModeCall _ | MonitorWait _), _ -> + | _, (StrictModeCall _ | MayBlock _ | MonitorWait _ | MustNotOccurUnderLock _) + | (StrictModeCall _ | MayBlock _ | MonitorWait _ | MustNotOccurUnderLock _), _ -> (* should never happen *) L.die InternalError "Deadlock cannot occur without two lock events: %a" CriticalPair.pp current_elem @@ -619,8 +651,6 @@ let should_report_deadlock_on_current_proc current_elem endpoint_elem = let should_report attrs = - (not (PredSymb.equal_access (ProcAttributes.get_access attrs) Private)) - && match ProcAttributes.get_proc_name attrs with | Procname.Java java_pname -> (not (Procname.Java.is_autogen_method java_pname)) @@ -648,6 +678,8 @@ let fold_reportable_summaries analyze_ondemand tenv clazz ~init ~f = List.fold methods ~init ~f +let is_private attrs = PredSymb.equal_access (ProcAttributes.get_access attrs) Private + (* Note about how many times we report a deadlock: normally twice, at each trace starting point. Due to the fact we look for deadlocks in the summaries of the class at the root of a path, this will fail when (a) the lock is of class type (ie as used in static sync methods), because @@ -660,55 +692,60 @@ let fold_reportable_summaries analyze_ondemand tenv clazz ~init ~f = [should_report_starvation] means [pair] is on the UI thread and not on a constructor *) let report_on_parallel_composition ~should_report_starvation tenv pattrs pair lock other_pname other_pair report_map = - let open Domain in - let pname = ProcAttributes.get_proc_name pattrs in - let make_trace_and_loc () = - let first_trace = CriticalPair.make_trace ~header:"[Trace 1] " pname pair in - let second_trace = CriticalPair.make_trace ~header:"[Trace 2] " other_pname other_pair in - let ltr = first_trace @ second_trace in - let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in - (ltr, loc) - in - if CriticalPair.can_run_in_parallel pair other_pair then - let acquisitions = other_pair.CriticalPair.elem.acquisitions in - match other_pair.CriticalPair.elem.event with - | MayBlock {severity} as event - when should_report_starvation - && Acquisitions.lock_is_held_in_other_thread tenv lock acquisitions -> - let error_message = - Format.asprintf - "Method %a runs on UI thread and%a, which may be held by another thread which %a." - pname_pp pname Lock.pp_locks lock Event.describe event - in - let ltr, loc = make_trace_and_loc () in - ReportMap.add_starvation severity tenv pattrs loc ltr error_message report_map - | MonitorWait {lock= monitor_lock} - when should_report_starvation - && Acquisitions.lock_is_held_in_other_thread tenv lock acquisitions - && not (Lock.equal lock monitor_lock) -> - let error_message = - Format.asprintf - "Method %a runs on UI thread and%a, which may be held by another thread which %a." - pname_pp pname Lock.pp_locks lock Event.describe other_pair.CriticalPair.elem.event - in - let ltr, loc = make_trace_and_loc () in - ReportMap.add_starvation High tenv pattrs loc ltr error_message report_map - | LockAcquire _ -> ( - match CriticalPair.may_deadlock tenv ~lhs:pair ~lhs_lock:lock ~rhs:other_pair with - | Some other_lock when should_report_deadlock_on_current_proc pair other_pair -> + if + is_private pattrs + || AnalysisCallbacks.proc_resolve_attributes other_pname |> Option.exists ~f:is_private + then report_map + else + let open Domain in + let pname = ProcAttributes.get_proc_name pattrs in + let make_trace_and_loc () = + let first_trace = CriticalPair.make_trace ~header:"[Trace 1] " pname pair in + let second_trace = CriticalPair.make_trace ~header:"[Trace 2] " other_pname other_pair in + let ltr = first_trace @ second_trace in + let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in + (ltr, loc) + in + if CriticalPair.can_run_in_parallel pair other_pair then + let acquisitions = other_pair.CriticalPair.elem.acquisitions in + match other_pair.CriticalPair.elem.event with + | MayBlock {severity} as event + when should_report_starvation + && Acquisitions.lock_is_held_in_other_thread tenv lock acquisitions -> + let error_message = + Format.asprintf + "Method %a runs on UI thread and%a, which may be held by another thread which %a." + pname_pp pname Lock.pp_locks lock Event.describe event + in + let ltr, loc = make_trace_and_loc () in + ReportMap.add_starvation severity tenv pattrs loc ltr error_message report_map + | MonitorWait {lock= monitor_lock} + when should_report_starvation + && Acquisitions.lock_is_held_in_other_thread tenv lock acquisitions + && not (Lock.equal lock monitor_lock) -> let error_message = Format.asprintf - "Potential deadlock. %a (Trace 1) and %a (Trace 2) acquire locks %a and %a in \ - reverse orders." - pname_pp pname pname_pp other_pname Lock.describe lock Lock.describe other_lock + "Method %a runs on UI thread and%a, which may be held by another thread which %a." + pname_pp pname Lock.pp_locks lock Event.describe other_pair.CriticalPair.elem.event in let ltr, loc = make_trace_and_loc () in - ReportMap.add_deadlock tenv pattrs loc ltr error_message report_map + ReportMap.add_starvation High tenv pattrs loc ltr error_message report_map + | LockAcquire _ -> ( + match CriticalPair.may_deadlock tenv ~lhs:pair ~lhs_lock:lock ~rhs:other_pair with + | Some other_lock when should_report_deadlock_on_current_proc pair other_pair -> + let error_message = + Format.asprintf + "Potential deadlock. %a (Trace 1) and %a (Trace 2) acquire locks %a and %a in \ + reverse orders." + pname_pp pname pname_pp other_pname Lock.describe lock Lock.describe other_lock + in + let ltr, loc = make_trace_and_loc () in + ReportMap.add_deadlock tenv pattrs loc ltr error_message report_map + | _ -> + report_map ) | _ -> - report_map ) - | _ -> - report_map - else report_map + report_map + else report_map let report_on_pair ~analyze_ondemand tenv pattrs (pair : Domain.CriticalPair.t) report_map = @@ -718,34 +755,62 @@ let report_on_pair ~analyze_ondemand tenv pattrs (pair : Domain.CriticalPair.t) let should_report_starvation = CriticalPair.is_uithread pair && not (Procname.is_constructor pname) in + let is_not_private = not (is_private pattrs) in let make_trace_and_loc () = let loc = CriticalPair.get_loc pair in let ltr = CriticalPair.make_trace ~include_acquisitions:false pname pair in (ltr, loc) in match event with - | MayBlock {severity} when should_report_starvation -> + | MayBlock {severity} when is_not_private && should_report_starvation -> let error_message = Format.asprintf "Method %a runs on UI thread and may block; %a." pname_pp pname Event.describe event in let ltr, loc = make_trace_and_loc () in ReportMap.add_starvation severity tenv pattrs loc ltr error_message report_map - | MonitorWait _ when should_report_starvation -> + | MonitorWait _ when is_not_private && should_report_starvation -> let error_message = Format.asprintf "Method %a runs on UI thread and may block; %a." pname_pp pname Event.describe event in let ltr, loc = make_trace_and_loc () in ReportMap.add_starvation High tenv pattrs loc ltr error_message report_map - | StrictModeCall _ when should_report_starvation -> + | StrictModeCall _ when is_not_private && should_report_starvation -> let error_message = Format.asprintf "Method %a runs on UI thread and may violate Strict Mode; %a." pname_pp pname Event.describe event in let ltr, loc = make_trace_and_loc () in ReportMap.add_strict_mode_violation tenv pattrs loc ltr error_message report_map - | LockAcquire _ when StarvationModels.is_annotated_lockless tenv pname -> + | MustNotOccurUnderLock _ when not (Acquisitions.is_empty pair.elem.acquisitions) -> ( + (* warn only at the innermost procedure taking a lock around the final call *) + let procs_with_acquisitions = + Acquisitions.fold + (fun (acquisition : Acquisition.t) acc -> Procname.Set.add acquisition.procname acc) + pair.elem.acquisitions Procname.Set.empty + in + match Procname.Set.is_singleton_or_more procs_with_acquisitions with + | IContainer.Empty -> + L.die InternalError "Found empty set of acquisitions after checking for non-emptiness.@\n" + | IContainer.More -> + (* acquisitions found in more than one proc, ignore *) + report_map + | IContainer.Singleton acquiring_pname when not (Procname.equal acquiring_pname pname) -> + (* we are at a caller of the acquiring procname, so ignore *) + report_map + | IContainer.Singleton _ -> + let error_message = + Format.asprintf + "Method %a %a under a lock; executed code may acquire arbitrary locks leading to \ + potential deadlock." + pname_pp pname Event.describe event + in + let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in + let ltr = CriticalPair.make_trace pname pair in + ReportMap.add_arbitrary_code_execution_under_lock tenv pattrs loc ltr error_message + report_map ) + | LockAcquire _ when is_not_private && StarvationModels.is_annotated_lockless tenv pname -> let error_message = Format.asprintf "Method %a is annotated %s but%a." pname_pp pname (MF.monospaced_to_string Annotations.lockless) @@ -754,7 +819,7 @@ let report_on_pair ~analyze_ondemand tenv pattrs (pair : Domain.CriticalPair.t) let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in let ltr = CriticalPair.make_trace pname pair in ReportMap.add_lockless_violation tenv pattrs loc ltr error_message report_map - | LockAcquire {locks} -> ( + | LockAcquire {locks} when is_not_private -> ( match List.find locks ~f:(fun lock -> Acquisitions.lock_is_held lock pair.elem.acquisitions) with diff --git a/infer/src/concurrency/starvationDomain.ml b/infer/src/concurrency/starvationDomain.ml index 9ee706cd5..e80315495 100644 --- a/infer/src/concurrency/starvationDomain.ml +++ b/infer/src/concurrency/starvationDomain.ml @@ -191,8 +191,9 @@ module Event = struct type t = | LockAcquire of {locks: Lock.t list; thread: ThreadDomain.t} | MayBlock of {callee: Procname.t; severity: StarvationModels.severity; thread: ThreadDomain.t} - | StrictModeCall of {callee: Procname.t; thread: ThreadDomain.t} | MonitorWait of {lock: Lock.t; thread: ThreadDomain.t} + | MustNotOccurUnderLock of {callee: Procname.t; thread: ThreadDomain.t} + | StrictModeCall of {callee: Procname.t; thread: ThreadDomain.t} [@@deriving compare] let pp fmt = function @@ -207,20 +208,26 @@ module Event = struct F.fprintf fmt "StrictModeCall(%a, %a)" Procname.pp callee ThreadDomain.pp thread | MonitorWait {lock; thread} -> F.fprintf fmt "MonitorWait(%a, %a)" Lock.pp lock ThreadDomain.pp thread + | MustNotOccurUnderLock {callee; thread} -> + F.fprintf fmt "MustNotOccurUnderLock(%a, %a)" Procname.pp callee ThreadDomain.pp thread let describe fmt elem = match elem with | LockAcquire {locks} -> Pp.comma_seq Lock.pp_locks fmt locks - | MayBlock {callee} | StrictModeCall {callee} -> + | MayBlock {callee} | StrictModeCall {callee} | MustNotOccurUnderLock {callee} -> F.fprintf fmt "calls %a" describe_pname callee | MonitorWait {lock} -> F.fprintf fmt "calls `wait` on %a" Lock.describe lock let get_thread = function - | LockAcquire {thread} | MayBlock {thread} | StrictModeCall {thread} | MonitorWait {thread} -> + | LockAcquire {thread} + | MayBlock {thread} + | MonitorWait {thread} + | MustNotOccurUnderLock {thread} + | StrictModeCall {thread} -> thread @@ -232,10 +239,12 @@ module Event = struct LockAcquire {lock_acquire with thread} | MayBlock may_block -> MayBlock {may_block with thread} - | StrictModeCall strict_mode_call -> - StrictModeCall {strict_mode_call with thread} | MonitorWait monitor_wait -> MonitorWait {monitor_wait with thread} + | MustNotOccurUnderLock not_under_lock -> + MustNotOccurUnderLock {not_under_lock with thread} + | StrictModeCall strict_mode_call -> + StrictModeCall {strict_mode_call with thread} let apply_caller_thread caller_thread event = @@ -254,11 +263,13 @@ module Event = struct let make_object_wait lock thread = MonitorWait {lock; thread} + let make_arbitrary_code_exec callee thread = MustNotOccurUnderLock {callee; thread} + let get_acquired_locks = function LockAcquire {locks} -> locks | _ -> [] let apply_subst subst event = match event with - | MayBlock _ | StrictModeCall _ -> + | MayBlock _ | StrictModeCall _ | MustNotOccurUnderLock _ -> Some event | MonitorWait {lock; thread} -> ( match Lock.apply_subst subst lock with @@ -806,6 +817,11 @@ let strict_mode_call ~callee ~loc astate = make_call_with_event new_event ~loc astate +let arbitrary_code_execution ~callee ~loc astate = + let new_event = Event.make_arbitrary_code_exec callee astate.thread in + make_call_with_event new_event ~loc astate + + let release ({lock_state} as astate) locks = { astate with lock_state= List.fold locks ~init:lock_state ~f:(fun acc l -> LockState.release l acc) } diff --git a/infer/src/concurrency/starvationDomain.mli b/infer/src/concurrency/starvationDomain.mli index 79938c0fd..cde5f0881 100644 --- a/infer/src/concurrency/starvationDomain.mli +++ b/infer/src/concurrency/starvationDomain.mli @@ -59,8 +59,9 @@ module Event : sig type t = | LockAcquire of {locks: Lock.t list; thread: ThreadDomain.t} | MayBlock of {callee: Procname.t; severity: StarvationModels.severity; thread: ThreadDomain.t} - | StrictModeCall of {callee: Procname.t; thread: ThreadDomain.t} | MonitorWait of {lock: Lock.t; thread: ThreadDomain.t} + | MustNotOccurUnderLock of {callee: Procname.t; thread: ThreadDomain.t} + | StrictModeCall of {callee: Procname.t; thread: ThreadDomain.t} [@@deriving compare] val describe : F.formatter -> t -> unit @@ -198,6 +199,8 @@ val future_get : callee:Procname.t -> loc:Location.t -> HilExp.t list -> t -> t val strict_mode_call : callee:Procname.t -> loc:Location.t -> t -> t +val arbitrary_code_execution : callee:Procname.t -> loc:Location.t -> t -> t + val add_guard : acquire_now:bool -> procname:Procname.t diff --git a/infer/tests/codetoanalyze/java/starvation/NotUnderLock.java b/infer/tests/codetoanalyze/java/starvation/NotUnderLock.java new file mode 100644 index 000000000..e5d2f49ec --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/NotUnderLock.java @@ -0,0 +1,26 @@ +/* + * 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. + */ + +import com.google.common.util.concurrent.SettableFuture; + +public class NotUnderLock { + SettableFuture future = null; + + private void callFutureSetOk() { + future.set(null); + } + + private synchronized void firstAcquisitionBad() { + callFutureSetOk(); + } + + private void secondAcquisitionOk(Object o) { + synchronized (o) { + firstAcquisitionBad(); + } + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 287043e72..0e82b7a83 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -41,6 +41,7 @@ codetoanalyze/java/starvation/MyActivity.java, MyActivity.onStart():void, 33, ST codetoanalyze/java/starvation/MyActivity.java, MyActivity.onStop():void, 53, STARVATION, no_bucket, ERROR, [`void MyActivity.onStop()`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation/NonBlk.java, NonBlk.deadlockABBad():void, 33, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void NonBlk.deadlockABBad()`, locks `this` in `class NonBlk`, locks `this.future` in `class NonBlk`,[Trace 2] `void NonBlk.deadlockBABad()`, locks `this.future` in `class NonBlk`, locks `this` in `class NonBlk`] codetoanalyze/java/starvation/NonBlk.java, NonBlk.deadlockBABad():void, 40, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void NonBlk.deadlockBABad()`, locks `this.future` in `class NonBlk`, locks `this` in `class NonBlk`,[Trace 2] `void NonBlk.deadlockABBad()`, locks `this` in `class NonBlk`, locks `this.future` in `class NonBlk`] +codetoanalyze/java/starvation/NotUnderLock.java, NotUnderLock.firstAcquisitionBad():void, 17, ARBITRARY_CODE_EXECUTION_UNDER_LOCK, no_bucket, ERROR, [`void NotUnderLock.firstAcquisitionBad()`, locks `this` in `class NotUnderLock`,Method call: `void NotUnderLock.callFutureSetOk()`,calls `boolean SettableFuture.set(Object)`] codetoanalyze/java/starvation/ObjWait.java, ObjWait.indirectWaitOnMainWithoutTimeoutBad():void, 46, STARVATION, no_bucket, ERROR, [[Trace 1] `void ObjWait.indirectWaitOnMainWithoutTimeoutBad()`, locks `this.lock` in `class ObjWait`,[Trace 2] `void ObjWait.lockAndWaitOnAnyWithoutTimeoutBad()`, locks `this.lock` in `class ObjWait`, locks `this.x` in `class ObjWait`,calls `wait` on `this.x` in `class ObjWait`] codetoanalyze/java/starvation/ObjWait.java, ObjWait.waitOnMainWithExcessiveTimeout1Bad():void, 31, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`,calls `wait` on `this.o` in `class ObjWait`] codetoanalyze/java/starvation/ObjWait.java, ObjWait.waitOnMainWithExcessiveTimeout2Bad():void, 38, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithExcessiveTimeout2Bad()`,calls `wait` on `this.o` in `class ObjWait`]