diff --git a/infer/src/concurrency/StarvationModels.ml b/infer/src/concurrency/StarvationModels.ml index 934da6303..dfd9a5345 100644 --- a/infer/src/concurrency/StarvationModels.ml +++ b/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,41 +54,14 @@ 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 - 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))) ) ) +let float_of_const_int = function + | HilExp.Constant (Const.Cint duration_lit) -> + Some (IntLit.to_float duration_lit) | _ -> - false + None + +let is_excessive_secs duration = duration >. android_anr_time_limit type severity = Low | Medium | High [@@deriving compare] @@ -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= - (fun actuals -> - List.nth actuals 4 |> Option.value_map ~default:false ~f:HilExp.is_int_zero ) } ] + ; actuals_pred= (fun actuals -> List.nth actuals 4 |> Option.exists ~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 *) diff --git a/infer/tests/codetoanalyze/java/starvation/ThreadCalls.java b/infer/tests/codetoanalyze/java/starvation/ThreadCalls.java new file mode 100644 index 000000000..506720752 --- /dev/null +++ b/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); + } + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/ThreadSleep.java b/infer/tests/codetoanalyze/java/starvation/ThreadSleep.java deleted file mode 100644 index 1d45837d8..000000000 --- a/infer/tests/codetoanalyze/java/starvation/ThreadSleep.java +++ /dev/null @@ -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(); - } - } -} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index f2deb1d63..becefe5e7 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/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)`]