From e3f075433587ca232899894bba955f38fb28ca7b Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 11 May 2018 06:11:12 -0700 Subject: [PATCH] [starvation] add static timeout recognition and 5 second android limit Reviewed By: jvillard Differential Revision: D7953454 fbshipit-source-id: 76a72f0 --- infer/src/concurrency/RacerDConfig.ml | 57 +++++++++++++++++-- .../java/starvation/FutureGet.java | 44 ++++++++++++++ .../codetoanalyze/java/starvation/issues.exp | 7 ++- 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/infer/src/concurrency/RacerDConfig.ml b/infer/src/concurrency/RacerDConfig.ml index df0e4eeb8..628cca6ff 100644 --- a/infer/src/concurrency/RacerDConfig.ml +++ b/infer/src/concurrency/RacerDConfig.ml @@ -586,6 +586,38 @@ module Models = struct false + (** magical value from https://developer.android.com/topic/performance/vitals/anr *) + let android_anr_time_limit = 5.0 + + (** can this timeout cause an anr? *) + let is_excessive_timeout = + let time_units = + String.Map.of_alist_exn + [ ("NANOSECONDS", 0.000_000_001) + ; ("MICROSECONDS", 0.000_001) + ; ("MILLISECONDS", 0.001) + ; ("SECONDS", 1.0) + ; ("MINUTES", 60.0) + ; ("HOURS", 3_600.0) + ; ("DAYS", 86_400.0) ] + in + fun duration_exp timeunit_exp -> + match (duration_exp, timeunit_exp) with + | HilExp.Constant (Const.Cint duration_lit), HilExp.AccessExpression timeunit_acc_exp -> ( + match AccessExpression.to_access_path timeunit_acc_exp with + | _, [AccessPath.FieldAccess field] + when String.equal "java.util.concurrent.TimeUnit" (Typ.Fieldname.Java.get_class field) -> + let fieldname = Typ.Fieldname.Java.get_field field in + let duration = float_of_int (IntLit.to_int duration_lit) in + String.Map.find time_units fieldname + |> Option.value_map ~default:false ~f:(fun unit_in_secs -> + unit_in_secs *. duration >. android_anr_time_limit ) + | _ -> + false ) + | _ -> + false + + (** It's surprisingly difficult making any sense of the official documentation on java.io classes as to whether methods may block. We approximate these by calls in traditionally blocking classes (non-channel based I/O), to methods `(Reader|InputStream).read*`. @@ -596,9 +628,19 @@ module Models = struct "read" + let actuals_are_empty_or_timeout = function + | [_] -> + true + | [_; duration; timeunit] -> + is_excessive_timeout duration timeunit + | _ -> + false + + (** is the method called CountDownLath.await or on subclass? *) let is_countdownlatch_await = - is_call_of_class_or_superclass ["java.util.concurrent.CountDownLatch"] "await" + is_call_of_class_or_superclass ~actuals_pred:actuals_are_empty_or_timeout + ["java.util.concurrent.CountDownLatch"] "await" (** an IBinder.transact call is an RPC. If the 4th argument (5th counting `this` as the first) @@ -618,15 +660,18 @@ module Models = struct (** is it a call to Future.get() or on sublass? *) - let is_future_get = is_call_of_class_or_superclass ["java.util.concurrent.Future"] "get" + let is_future_get = + is_call_of_class_or_superclass ~actuals_pred:actuals_are_empty_or_timeout + ["java.util.concurrent.Future"] "get" + let is_accountManager_setUserData = is_call_of_class_or_superclass ["android.accounts.AccountManager"] "setUserData" - let is_asyncTask_get_without_timeout = - let actuals_pred actuals = Int.equal 1 (List.length actuals) in - is_call_of_class_or_superclass ~actuals_pred ["android.os.AsyncTask"] "get" + let is_asyncTask_get = + is_call_of_class_or_superclass ~actuals_pred:actuals_are_empty_or_timeout + ["android.os.AsyncTask"] "get" let may_block = @@ -637,7 +682,7 @@ module Models = struct ; is_getWindowVisibleDisplayFrame ; is_future_get ; is_accountManager_setUserData - ; is_asyncTask_get_without_timeout ] + ; is_asyncTask_get ] in fun tenv pn actuals -> List.exists matchers ~f:(fun matcher -> matcher tenv pn actuals) end diff --git a/infer/tests/codetoanalyze/java/starvation/FutureGet.java b/infer/tests/codetoanalyze/java/starvation/FutureGet.java index f9f9ba1b2..57388e6db 100644 --- a/infer/tests/codetoanalyze/java/starvation/FutureGet.java +++ b/infer/tests/codetoanalyze/java/starvation/FutureGet.java @@ -9,7 +9,9 @@ import java.util.concurrent.Future; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; import android.support.annotation.UiThread; +import java.util.concurrent.TimeUnit; class FutureGet { Future future; @@ -34,4 +36,46 @@ class FutureGet { void getOnOtherThreadOk() throws InterruptedException, ExecutionException { future.get(); } + + @UiThread + void getTimeoutOneDayBad() throws InterruptedException, ExecutionException { + try { + future.get(1L, TimeUnit.DAYS); + } catch (TimeoutException e) {} + } + + @UiThread + void getTimeoutOneSecondOk() throws InterruptedException, ExecutionException { + try { + future.get(1L, TimeUnit.SECONDS); + } catch (TimeoutException e) {} + } + + @UiThread + void getTimeoutOneHourBad() throws InterruptedException, ExecutionException { + try { + future.get(1L, TimeUnit.HOURS); + } catch (TimeoutException e) {} + } + + @UiThread + void getTimeoutFourSecondsOk() throws InterruptedException, ExecutionException { + try { + future.get(4L, TimeUnit.SECONDS); + } catch (TimeoutException e) {} + } + + @UiThread + void getTimeout4999MilliSecondsOk() throws InterruptedException, ExecutionException { + try { + future.get(4999L, TimeUnit.MILLISECONDS); + } catch (TimeoutException e) {} + } + + @UiThread + void getTimeout50000001MicroSecondsBad() throws InterruptedException, ExecutionException { + try { + future.get(5000001L, TimeUnit.MICROSECONDS); + } catch (TimeoutException e) {} + } } diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index a2391a66b..2b1517209 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -7,8 +7,11 @@ codetoanalyze/java/starvation/Binders.java, void Binders.interBad(), 25, STARVAT codetoanalyze/java/starvation/Binders.java, void Binders.intraBad(), 30, STARVATION, ERROR, [ void Binders.intraBad(),Method call: void Binders.doTransact(),calls boolean Binder.transact(int,Parcel,Parcel,int) from void Binders.doTransact()] codetoanalyze/java/starvation/Countdwn.java, void Countdwn.awaitOnMainByAnnotBad(), 22, STARVATION, ERROR, [ void Countdwn.awaitOnMainByAnnotBad(),calls void CountDownLatch.await() from void Countdwn.awaitOnMainByAnnotBad()] codetoanalyze/java/starvation/Countdwn.java, void Countdwn.awaitOnMainByCallBad(), 16, STARVATION, ERROR, [ void Countdwn.awaitOnMainByCallBad(),calls void CountDownLatch.await() from void Countdwn.awaitOnMainByCallBad()] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getDirectBad(), 19, STARVATION, ERROR, [ void FutureGet.getDirectBad(),calls Object Future.get() from void FutureGet.getDirectBad()] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getIndirectBad(), 24, STARVATION, ERROR, [[Trace 1] void FutureGet.getIndirectBad(),locks `this.FutureGet.lock` in class `FutureGet*`,[Trace 2] void FutureGet.getUnderLock(),locks `this.FutureGet.lock` in class `FutureGet*`,calls Object Future.get() from void FutureGet.getUnderLock()] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getDirectBad(), 21, STARVATION, ERROR, [ void FutureGet.getDirectBad(),calls Object Future.get() from void FutureGet.getDirectBad()] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getIndirectBad(), 26, STARVATION, ERROR, [[Trace 1] void FutureGet.getIndirectBad(),locks `this.FutureGet.lock` in class `FutureGet*`,[Trace 2] void FutureGet.getUnderLock(),locks `this.FutureGet.lock` in class `FutureGet*`,calls Object Future.get() from void FutureGet.getUnderLock()] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout50000001MicroSecondsBad(), 76, STARVATION, ERROR, [ void FutureGet.getTimeout50000001MicroSecondsBad(),calls Object Future.get(long,TimeUnit) from void FutureGet.getTimeout50000001MicroSecondsBad()] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneDayBad(), 41, STARVATION, ERROR, [ void FutureGet.getTimeoutOneDayBad(),calls Object Future.get(long,TimeUnit) from void FutureGet.getTimeoutOneDayBad()] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneHourBad(), 55, STARVATION, ERROR, [ void FutureGet.getTimeoutOneHourBad(),calls Object Future.get(long,TimeUnit) from void FutureGet.getTimeoutOneHourBad()] codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeExpensiveLockOnUiThreadBad(), 23, STARVATION, ERROR, [[Trace 1] void IndirectBlock.takeExpensiveLockOnUiThreadBad(),locks `this.IndirectBlock.expensiveLock` in class `IndirectBlock*`,[Trace 2] void IndirectBlock.doTransactUnderLock(),locks `this.IndirectBlock.expensiveLock` in class `IndirectBlock*`,calls boolean Binder.transact(int,Parcel,Parcel,int) from void IndirectBlock.doTransactUnderLock()] codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc), 34, STARVATION, ERROR, [[Trace 1] void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc),Method call: void IndirectInterproc.takeLock(),locks `this` in class `IndirectInterproc*`,[Trace 2] void IndirectInterproc.doTransactUnderLock(Binder),locks `this` in class `IndirectInterproc*`,calls boolean Binder.transact(int,Parcel,Parcel,int) from void IndirectInterproc.doTransactUnderLock(Binder)] codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.(InnerClass,Object), 49, DEADLOCK, ERROR, [[Trace 1] InnerClass$InnerClassA.(InnerClass,Object),locks `this` in class `InnerClass$InnerClassA*`,Method call: void InnerClass.bar(),locks `this` in class `InnerClass*`,[Trace 2] void InnerClass.outerInnerBad(InnerClass$InnerClassA),locks `this` in class `InnerClass*`,Method call: void InnerClass$InnerClassA.baz(),locks `this` in class `InnerClass$InnerClassA*`]