[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

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();
+    }
+  }
+}
+```

infer/man/man1/infer-full.txt

@@ -382,6 +382,7 @@ OPTIONS
            disabling issue types does not make the corresponding checker not
            run.           Available issue types are as follows:
              
+           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),

infer/man/man1/infer-report.txt

@@ -85,6 +85,7 @@ OPTIONS
            disabling issue types does not make the corresponding checker not
            run.           Available issue types are as follows:
              
+           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),

infer/man/man1/infer.txt

@@ -382,6 +382,7 @@ OPTIONS
            disabling issue types does not make the corresponding checker not
            run.           Available issue types are as follows:
              
+           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),

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 =

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

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"]}

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] *)

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 _)
+  | _, (StrictModeCall _ | MayBlock _ | MonitorWait _ | MustNotOccurUnderLock _)
-  | (MayBlock _ | StrictModeCall _ | MonitorWait _), _ ->
+  | (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,6 +692,11 @@ 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 =
+  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 () =
@@ -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

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) }

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

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();
+    }
+  }
+}

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`]