[starvation] model Thread.join as blocking

Summary:
Also add logic for recognising excessive timeouts.  Refactor the code
around timeouts a little.

Reviewed By: artempyanykh

Differential Revision: D18807836

fbshipit-source-id: df5a1b566

infer/src/concurrency/StarvationModels.ml

@@ -27,7 +27,7 @@ let should_skip_analysis = MethodMatcher.of_json Config.starvation_skip_analysis
 let android_anr_time_limit = 5.0
 
 (* get time unit in seconds *)
-let timeunit_of_exp =
+let secs_of_timeunit =
   let time_units =
     String.Map.of_alist_exn
       [ ("NANOSECONDS", 0.000_000_001)
@@ -54,42 +54,15 @@ let timeunit_of_exp =
   fun timeunit_exp -> str_of_exp timeunit_exp |> Option.bind ~f:(String.Map.find time_units)
 
 
-(** check whether actuals of a method call either empty, denoting indefinite timeout, or evaluate to
+let float_of_const_int = function
-    a finite timeout greater than the android anr limit *)
-let empty_or_excessive_timeout actuals =
-  let duration_of_exp = function
   | HilExp.Constant (Const.Cint duration_lit) ->
       Some (IntLit.to_float duration_lit)
   | _ ->
       None
-  in
-  (* all arguments in seconds *)
-  let is_excessive_secs duration = duration >. android_anr_time_limit in
-  match actuals with
-  | [_] ->
-      (* this is a wait or lock call without timeout, thus it can block indefinitely *)
-      true
-  | [_; snd_arg] ->
-      (* this is an Object.wait(_) call, second argument should be a duration in milliseconds *)
-      duration_of_exp snd_arg
-      |> Option.value_map ~default:false ~f:(fun duration -> is_excessive_secs (0.001 *. duration))
-  | [_; snd_arg; third_arg] ->
-      (* this is either a call to Object.wait(_, _) or to a java.util.concurent.lock(_, _) method.
-         In the first case the arguments are a duration in milliseconds and an extra duration in
-         nanoseconds; in the second case, the arguments are a duration and a time unit. *)
-      duration_of_exp snd_arg
-      |> Option.value_map ~default:false ~f:(fun duration ->
-             match timeunit_of_exp third_arg with
-             | Some timeunit ->
-                 is_excessive_secs (timeunit *. duration)
-             | None ->
-                 duration_of_exp third_arg
-                 |> Option.value_map ~default:false ~f:(fun extra ->
-                        is_excessive_secs (0.001 *. (duration +. (0.000_001 *. extra))) ) )
-  | _ ->
-      false
 
 
+let is_excessive_secs duration = duration >. android_anr_time_limit
+
 type severity = Low | Medium | High [@@deriving compare]
 
 let pp_severity fmt sev =
@@ -97,33 +70,66 @@ let pp_severity fmt sev =
   F.pp_print_string fmt msg
 
 
+let no_args_or_excessive_timeout_and_timeunit = function
+  | [_] ->
+      (* no arguments, unconditionally blocks *)
+      true
+  | [_; timeout; timeunit] ->
+      (* two arguments, a timeout and a time unit *)
+      Option.both (float_of_const_int timeout) (secs_of_timeunit timeunit)
+      |> Option.map ~f:(fun (duration, timeunit_secs) -> duration *. timeunit_secs)
+      |> Option.exists ~f:is_excessive_secs
+  | _ ->
+      false
+
+
+let no_args_or_excessive_millis_and_nanos = function
+  | [_] ->
+      (* this is a wait without timeout, it can block indefinitely *)
+      true
+  | [_; snd_arg] ->
+      (* 2nd argument is a duration in milliseconds *)
+      float_of_const_int snd_arg
+      |> Option.exists ~f:(fun duration -> is_excessive_secs (0.001 *. duration))
+  | [_; snd_arg; third_arg] ->
+      (* 2nd argument is a duration in milliseconds, 3rd is extra duration in nanoseconds. *)
+      Option.both (float_of_const_int snd_arg) (float_of_const_int third_arg)
+      |> Option.map ~f:(fun (duration, extra) -> 0.001 *. (duration +. (0.000_001 *. extra)))
+      |> Option.exists ~f:is_excessive_secs
+  | _ ->
+      false
+
+
 let standard_matchers =
   let open MethodMatcher in
   let high_sev =
     [ {default with classname= "java.lang.Thread"; methods= ["sleep"]}
+    ; { default with
+        classname= "java.lang.Thread"
+      ; methods= ["join"]
+      ; actuals_pred= no_args_or_excessive_millis_and_nanos }
     ; { default with
         classname= "java.util.concurrent.CountDownLatch"
       ; methods= ["await"]
-      ; actuals_pred= empty_or_excessive_timeout }
+      ; actuals_pred= no_args_or_excessive_timeout_and_timeunit }
       (* an IBinder.transact call is an RPC.  If the 4th argument (5th counting `this` as the first)
          is int-zero then a reply is expected and returned from the remote process, thus potentially
          blocking.  If the 4th argument is anything else, we assume a one-way call which doesn't block. *)
     ; { default with
         classname= "android.os.IBinder"
       ; methods= ["transact"]
-      ; actuals_pred=
+      ; actuals_pred= (fun actuals -> List.nth actuals 4 |> Option.exists ~f:HilExp.is_int_zero) }
-          (fun actuals ->
+    ]
-            List.nth actuals 4 |> Option.value_map ~default:false ~f:HilExp.is_int_zero ) } ]
   in
   let low_sev =
     [ { default with
         classname= "java.util.concurrent.Future"
       ; methods= ["get"]
-      ; actuals_pred= empty_or_excessive_timeout }
+      ; actuals_pred= no_args_or_excessive_timeout_and_timeunit }
     ; { default with
         classname= "android.os.AsyncTask"
       ; methods= ["get"]
-      ; actuals_pred= empty_or_excessive_timeout } ]
+      ; actuals_pred= no_args_or_excessive_timeout_and_timeunit } ]
   in
   let high_sev_matcher = List.map high_sev ~f:of_record |> of_list in
   let low_sev_matcher = List.map low_sev ~f:of_record |> of_list in
@@ -142,7 +148,7 @@ let is_monitor_wait =
       { default with
         classname= "java.lang.Object"
       ; methods= ["wait"]
-      ; actuals_pred= empty_or_excessive_timeout })
+      ; actuals_pred= no_args_or_excessive_millis_and_nanos })
 
 
 (* selection is a bit arbitrary as some would be generated anyway if not here; no harm though *)

infer/tests/codetoanalyze/java/starvation/ThreadCalls.java

@@ -0,0 +1,68 @@
+/*
+ * 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 android.support.annotation.UiThread;
+
+class ThreadCalls {
+  void sleepOnAnyThreadOk() throws InterruptedException {
+    Thread.sleep(60);
+  }
+
+  @UiThread
+  void sleepOnUIThreadBad() throws InterruptedException {
+    Thread.sleep(60);
+  }
+
+  Object lock;
+
+  @UiThread
+  void indirectSleepOnUIThreadBad() {
+    synchronized (lock) {
+    }
+  }
+
+  void lockAndSleepOnNonUIThread() throws InterruptedException {
+    synchronized (lock) {
+      sleepOnAnyThreadOk();
+    }
+  }
+
+  void joinOnAnyThreadOk(Thread thread) throws InterruptedException {
+    thread.join();
+  }
+
+  @UiThread
+  void joinOnUIThreadBad(Thread thread) throws InterruptedException {
+    thread.join();
+  }
+
+  @UiThread
+  void joinWithTimeout1OnUIThreadOk(Thread thread) throws InterruptedException {
+    // 50 milliseconds
+    thread.join(50);
+  }
+
+  @UiThread
+  void joinWithTimeout2OnUIThreadOk(Thread thread) throws InterruptedException {
+    // 500 milliseconds + 10000 nanoseconds
+    thread.join(500, 10000);
+  }
+
+  Object joinLock;
+
+  @UiThread
+  void indirectJoinOnUIThreadBad() {
+    synchronized (joinLock) {
+    }
+  }
+
+  void lockAndSleepOnNonUIThread(Thread thread) throws InterruptedException {
+    synchronized (joinLock) {
+      joinOnAnyThreadOk(thread);
+    }
+  }
+}

infer/tests/codetoanalyze/java/starvation/ThreadSleep.java

@@ -1,33 +0,0 @@
-/*
- * 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 android.support.annotation.UiThread;
-
-class ThreadSleep {
-  void sleepOnAnyThreadOk() throws InterruptedException {
-    Thread.sleep(60);
-  }
-
-  @UiThread
-  void sleepOnUIThreadBad() throws InterruptedException {
-    Thread.sleep(60);
-  }
-
-  Object lock;
-
-  @UiThread
-  void indirectSleepOnUIThreadBad() {
-    synchronized (lock) {
-    }
-  }
-
-  void lockAndSleepOnNonUIThread() throws InterruptedException {
-    synchronized (lock) {
-      sleepOnAnyThreadOk();
-    }
-  }
-}

infer/tests/codetoanalyze/java/starvation/issues.exp

@@ -67,6 +67,10 @@ codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.viol
 codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.violateStrictModeBad():void, 38, STRICT_MODE_VIOLATION, no_bucket, ERROR, [`void StrictModeViolation.violateStrictModeBad()`,calls `boolean File.setReadOnly()`]
 codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.violateStrictModeBad():void, 39, STRICT_MODE_VIOLATION, no_bucket, ERROR, [`void StrictModeViolation.violateStrictModeBad()`,calls `boolean File.setWritable(boolean)`]
 codetoanalyze/java/starvation/SuppLint.java, SuppLint.onUiThreadBad():void, 25, STARVATION, no_bucket, ERROR, [`void SuppLint.onUiThreadBad()`,calls `Object Future.get()`]
+codetoanalyze/java/starvation/ThreadCalls.java, ThreadCalls.indirectJoinOnUIThreadBad():void, 59, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadCalls.indirectJoinOnUIThreadBad()`, locks `this.joinLock` in `class ThreadCalls`,[Trace 2] `void ThreadCalls.lockAndSleepOnNonUIThread(Thread)`, locks `this.joinLock` in `class ThreadCalls`,Method call: `void ThreadCalls.joinOnAnyThreadOk(Thread)`,calls `void Thread.join()`]
+codetoanalyze/java/starvation/ThreadCalls.java, ThreadCalls.indirectSleepOnUIThreadBad():void, 24, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadCalls.indirectSleepOnUIThreadBad()`, locks `this.lock` in `class ThreadCalls`,[Trace 2] `void ThreadCalls.lockAndSleepOnNonUIThread()`, locks `this.lock` in `class ThreadCalls`,Method call: `void ThreadCalls.sleepOnAnyThreadOk()`,calls `void Thread.sleep(long)`]
+codetoanalyze/java/starvation/ThreadCalls.java, ThreadCalls.joinOnUIThreadBad(java.lang.Thread):void, 40, STARVATION, no_bucket, ERROR, [`void ThreadCalls.joinOnUIThreadBad(Thread)`,calls `void Thread.join()`]
+codetoanalyze/java/starvation/ThreadCalls.java, ThreadCalls.sleepOnUIThreadBad():void, 17, STARVATION, no_bucket, ERROR, [`void ThreadCalls.sleepOnUIThreadBad()`,calls `void Thread.sleep(long)`]
 codetoanalyze/java/starvation/ThreadDeadlock.java, ThreadDeadlock.annotatedUiThreadBad():void, 35, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadDeadlock.annotatedUiThreadBad()`, locks `this` in `class ThreadDeadlock`, locks `this.lockB` in `class ThreadDeadlock`,[Trace 2] `void ThreadDeadlock.annotatedWorkerThreadBad()`, locks `this.lockB` in `class ThreadDeadlock`, locks `this` in `class ThreadDeadlock`]
 codetoanalyze/java/starvation/ThreadDeadlock.java, ThreadDeadlock.annotatedWorkerThreadBad():void, 42, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadDeadlock.annotatedWorkerThreadBad()`, locks `this.lockB` in `class ThreadDeadlock`, locks `this` in `class ThreadDeadlock`,[Trace 2] `void ThreadDeadlock.annotatedUiThreadBad()`, locks `this` in `class ThreadDeadlock`, locks `this.lockB` in `class ThreadDeadlock`]
 codetoanalyze/java/starvation/ThreadDeadlock.java, ThreadDeadlock.assertOnBackgroundThreadBad():void, 60, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadDeadlock.assertOnBackgroundThreadBad()`, locks `this.lockC` in `class ThreadDeadlock`, locks `this` in `class ThreadDeadlock`,[Trace 2] `void ThreadDeadlock.assertOnUIThreadBad()`, locks `this` in `class ThreadDeadlock`, locks `this.lockC` in `class ThreadDeadlock`]
@@ -81,5 +85,3 @@ codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.conditio
 codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.conditionalNegatedIsMainThread_Bad():void, 60, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorF` in `class ThreadSensitivity`, locks `this.monitorE` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorE` in `class ThreadSensitivity`, locks `this.monitorF` in `class ThreadSensitivity`]
 codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.confusedAssertBad(boolean,boolean):void, 78, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.confusedAssertBad(boolean,boolean)`, locks `this.monitorG` in `class ThreadSensitivity`, locks `this.monitorH` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.confusedAssertBad(boolean,boolean)`, locks `this.monitorH` in `class ThreadSensitivity`, locks `this.monitorG` in `class ThreadSensitivity`]
 codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.confusedAssertBad(boolean,boolean):void, 83, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.confusedAssertBad(boolean,boolean)`, locks `this.monitorH` in `class ThreadSensitivity`, locks `this.monitorG` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.confusedAssertBad(boolean,boolean)`, locks `this.monitorG` in `class ThreadSensitivity`, locks `this.monitorH` in `class ThreadSensitivity`]
-codetoanalyze/java/starvation/ThreadSleep.java, ThreadSleep.indirectSleepOnUIThreadBad():void, 24, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadSleep.indirectSleepOnUIThreadBad()`, locks `this.lock` in `class ThreadSleep`,[Trace 2] `void ThreadSleep.lockAndSleepOnNonUIThread()`, locks `this.lock` in `class ThreadSleep`,Method call: `void ThreadSleep.sleepOnAnyThreadOk()`,calls `void Thread.sleep(long)`]
-codetoanalyze/java/starvation/ThreadSleep.java, ThreadSleep.sleepOnUIThreadBad():void, 17, STARVATION, no_bucket, ERROR, [`void ThreadSleep.sleepOnUIThreadBad()`,calls `void Thread.sleep(long)`]