From 14ec16da12dc07b6cb149fc6e37e62acb825bd10 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 4 May 2018 02:31:43 -0700 Subject: [PATCH] [starvation] catch calls to AcccountManager.setUserData Summary: Calls to AccountManager.setUserData are associated with ANRs. Reviewed By: sblackshear Differential Revision: D7859585 fbshipit-source-id: 215a522 --- infer/src/concurrency/RacerDConfig.ml | 38 ++++++++++++++++-- infer/src/concurrency/RacerDConfig.mli | 23 +---------- infer/src/concurrency/starvation.ml | 8 +--- .../codetoanalyze/java/starvation/AccMgr.java | 39 +++++++++++++++++++ .../codetoanalyze/java/starvation/issues.exp | 2 + 5 files changed, 78 insertions(+), 32 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/starvation/AccMgr.java diff --git a/infer/src/concurrency/RacerDConfig.ml b/infer/src/concurrency/RacerDConfig.ml index ae1efc6f8..047947cab 100644 --- a/infer/src/concurrency/RacerDConfig.ml +++ b/infer/src/concurrency/RacerDConfig.ml @@ -546,7 +546,10 @@ module Models = struct && not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting) - let is_call_of_class_or_superclass class_names ?(method_prefix= false) method_name tenv pn = + let is_call_of_class_or_superclass ?(method_prefix= false) ?(actuals_pred= fun _ -> true) + class_names method_name tenv pn actuals = + actuals_pred actuals + && match pn with | Typ.Procname.Java java_pname -> let classname = Typ.Procname.Java.get_class_type_name java_pname in @@ -562,25 +565,52 @@ module Models = struct 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*`. + Initially, (Writer|OutputStream).(flush|close) were also matched, but this generated too + many reports *) let is_blocking_java_io = is_call_of_class_or_superclass ["java.io.Reader"; "java.io.InputStream"] ~method_prefix:true "read" + (** is the method called CountDownLath.await or on subclass? *) let is_countdownlatch_await = is_call_of_class_or_superclass ["java.util.concurrent.CountDownLatch"] "await" + (** 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. + *) let is_two_way_binder_transact = - let matcher = is_call_of_class_or_superclass ["android.os.IBinder"] "transact" in - fun tenv actuals pn -> + let actuals_pred actuals = List.nth actuals 4 |> Option.value_map ~default:false ~f:HilExp.is_int_zero - && matcher tenv pn + in + is_call_of_class_or_superclass ~actuals_pred ["android.os.IBinder"] "transact" + (** is it a call to android.view.View.getWindowVisibleDisplayFrame or on sublass? *) let is_getWindowVisibleDisplayFrame = is_call_of_class_or_superclass ["android.view.View"] "getWindowVisibleDisplayFrame" + (** 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_accountManager_setUserData = + is_call_of_class_or_superclass ["android.accounts.AccountManager"] "setUserData" + + + let may_block = + let matchers = + [ is_blocking_java_io + ; is_countdownlatch_await + ; is_two_way_binder_transact + ; is_getWindowVisibleDisplayFrame + ; is_future_get + ; is_accountManager_setUserData ] + in + fun tenv pn actuals -> List.exists matchers ~f:(fun matcher -> matcher tenv pn actuals) end diff --git a/infer/src/concurrency/RacerDConfig.mli b/infer/src/concurrency/RacerDConfig.mli index cd0ba4f7e..48a5f3236 100644 --- a/infer/src/concurrency/RacerDConfig.mli +++ b/infer/src/concurrency/RacerDConfig.mli @@ -79,25 +79,6 @@ module Models : sig (** return true if procedure is at an abstraction boundary or reporting has been explicitly requested via @ThreadSafe *) - val is_blocking_java_io : Tenv.t -> Typ.Procname.t -> bool - (** 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*`. - Initially, (Writer|OutputStream).(flush|close) were also matched, but this generated too - many reports *) - - val is_countdownlatch_await : Tenv.t -> Typ.Procname.t -> bool - (** is the method called CountDownLath.await or on subclass? *) - - val is_two_way_binder_transact : Tenv.t -> HilExp.t list -> Typ.Procname.t -> bool - (** 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. - *) - - val is_getWindowVisibleDisplayFrame : Tenv.t -> Typ.Procname.t -> bool - (** is it a call to android.view.View.getWindowVisibleDisplayFrame or on sublass? *) - - val is_future_get : Tenv.t -> Typ.Procname.t -> bool - (** is it a call to Future.get() or on sublass? *) + val may_block : Tenv.t -> Typ.Procname.t -> HilExp.t list -> bool + (** is the method call potentially blocking, given the actuals passed? *) end diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 8e58e686c..e409fb80a 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -79,13 +79,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | LockedIfTrue -> astate | NoEffect -> - if - Models.is_countdownlatch_await tenv callee - || Models.is_two_way_binder_transact tenv actuals callee - || Models.is_blocking_java_io tenv callee - || Models.is_getWindowVisibleDisplayFrame tenv callee - || Models.is_future_get tenv callee - then + if Models.may_block tenv callee actuals then let caller = Procdesc.get_proc_name pdesc in Domain.blocking_call ~caller ~callee loc astate else if is_on_main_thread callee then Domain.set_on_main_thread astate diff --git a/infer/tests/codetoanalyze/java/starvation/AccMgr.java b/infer/tests/codetoanalyze/java/starvation/AccMgr.java new file mode 100644 index 000000000..02cea4acb --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/AccMgr.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import android.accounts.Account; +import android.accounts.AccountManager; +import android.support.annotation.UiThread; + +class AccMgr { + AccountManager am; + Account account; + String key, data; + Object lock; + + @UiThread + void onUiThreadBad() throws InterruptedException { + am.setUserData(account, key, data); + } + + @UiThread + void lockOnUiThreadBad() throws InterruptedException { + synchronized(lock) {} + } + + void setUserDataUnderLock() { + synchronized(lock) { + am.setUserData(account, key, data); + } + } + + void onOtherThreadOk() { + am.setUserData(account, key, data); + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 69027fa38..e6ae72f39 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -1,3 +1,5 @@ +codetoanalyze/java/starvation/AccMgr.java, void AccMgr.lockOnUiThreadBad(), 0, STARVATION, ERROR, [[Trace 1] void AccMgr.lockOnUiThreadBad(),locks `this.AccMgr.lock` in class `AccMgr*`,[Trace 2] void AccMgr.setUserDataUnderLock(),locks `this.AccMgr.lock` in class `AccMgr*`,calls void AccountManager.setUserData(Account,String,String) from void AccMgr.setUserDataUnderLock()] +codetoanalyze/java/starvation/AccMgr.java, void AccMgr.onUiThreadBad(), 0, STARVATION, ERROR, [ void AccMgr.onUiThreadBad(),calls void AccountManager.setUserData(Account,String,String) from void AccMgr.onUiThreadBad()] codetoanalyze/java/starvation/Binders.java, void Binders.annotationBad(), 0, STARVATION, ERROR, [ void Binders.annotationBad(),Method call: void Binders.doTransact(),calls boolean Binder.transact(int,Parcel,Parcel,int) from void Binders.doTransact()] codetoanalyze/java/starvation/Binders.java, void Binders.interBad(), 0, STARVATION, ERROR, [ void Binders.interBad(),calls boolean Binder.transact(int,Parcel,Parcel,int) from void Binders.interBad()] codetoanalyze/java/starvation/Binders.java, void Binders.intraBad(), 0, STARVATION, ERROR, [ void Binders.intraBad(),Method call: void Binders.doTransact(),calls boolean Binder.transact(int,Parcel,Parcel,int) from void Binders.doTransact()]