From 374c09c6c7f16e039455fe23cb82ad9eab1094c8 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Wed, 20 Nov 2019 14:16:19 -0800 Subject: [PATCH] [starvation][whole-program] allow scheduled work on unknown threads Summary: When we see a call to schedule some work on an executor and we don't have evidence that it is on some specific thread (UI/BG), instead of dropping the work, assign it `UnknownThread` and treat it as running on the background by default. Reviewed By: jvillard Differential Revision: D18615649 fbshipit-source-id: e8bad64b6 --- infer/src/concurrency/StarvationModels.ml | 3 +- infer/src/concurrency/StarvationModels.mli | 6 +- infer/src/concurrency/starvation.ml | 4 +- infer/src/concurrency/starvationDomain.ml | 18 +-- .../UnknownThread.java | 106 ++++++++++++++++++ .../java/starvation-whole-program/issues.exp | 4 + 6 files changed, 130 insertions(+), 11 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/starvation-whole-program/UnknownThread.java diff --git a/infer/src/concurrency/StarvationModels.ml b/infer/src/concurrency/StarvationModels.ml index 20f6a34ec..cd8d89248 100644 --- a/infer/src/concurrency/StarvationModels.ml +++ b/infer/src/concurrency/StarvationModels.ml @@ -223,7 +223,8 @@ let schedules_work_on_ui_thread = fun tenv pname -> matcher tenv pname [] -type executor_thread_constraint = ForUIThread | ForNonUIThread [@@deriving equal] +type executor_thread_constraint = ForUIThread | ForNonUIThread | ForUnknownThread +[@@deriving equal] (* Executors are usually stored in fields and annotated according to what type of thread they schedule work on. Given an expression representing such a field, try to find the kind of diff --git a/infer/src/concurrency/StarvationModels.mli b/infer/src/concurrency/StarvationModels.mli index 4b6a898ab..6423a453f 100644 --- a/infer/src/concurrency/StarvationModels.mli +++ b/infer/src/concurrency/StarvationModels.mli @@ -36,11 +36,13 @@ val schedules_work : Tenv.t -> Typ.Procname.t -> bool (** call known to schedule runnable first argument to some thread/executor *) (** an instance field holding a reference to an executor may be annotated as running on UI/non-UI thread *) -type executor_thread_constraint = ForUIThread | ForNonUIThread [@@deriving equal] +type executor_thread_constraint = ForUIThread | ForNonUIThread | ForUnknownThread +[@@deriving equal] val get_executor_thread_constraint : Tenv.t -> HilExp.AccessExpression.t -> executor_thread_constraint option -(** given an executor receiver, get its thread constraint, if any *) +(** given an executor receiver, get its thread constraint, if any. [None] means lookup somehow failed, + whereas [Some UnknownThread] means the receiver is an unannotated executor. *) val get_run_method_from_runnable : Tenv.t -> HilExp.AccessExpression.t -> Typ.Procname.t option (** given a receiver, find the [run()] method in the appropriate class *) diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 79db70988..cfd8f2cc9 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -99,7 +99,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct [ (* match an executor call that we have a model for *) lazy (StarvationModels.get_executor_thread_constraint tenv executor) ; (* match an executor call where the executor is stored in a variable *) - lazy (Domain.AttributeDomain.get_executor_constraint executor astate.attributes) ] + lazy (Domain.AttributeDomain.get_executor_constraint executor astate.attributes) + ; (* the receiver is an executor stored in a variable for which we have no knowledge *) + lazy (Some StarvationModels.ForUnknownThread) ] in Option.fold thread_opt ~init:astate ~f:(schedule_work runnable) | HilExp.AccessExpression runnable :: _ diff --git a/infer/src/concurrency/starvationDomain.ml b/infer/src/concurrency/starvationDomain.ml index c93f9a911..10016b1ea 100644 --- a/infer/src/concurrency/starvationDomain.ml +++ b/infer/src/concurrency/starvationDomain.ml @@ -460,7 +460,9 @@ module Attribute = struct | Executor StarvationModels.ForUIThread -> "Executor(UI)" | Executor StarvationModels.ForNonUIThread -> - "Executor(BG)" ) + "Executor(BG)" + | Executor StarvationModels.ForUnknownThread -> + "Executor(Unknown)" ) |> F.pp_print_string fmt @@ -630,12 +632,14 @@ let filter_blocking_calls ({critical_pairs} as astate) = let schedule_work loc thread_constraint astate procname = - let thread = - match thread_constraint with - | StarvationModels.ForUIThread -> - ThreadDomain.UIThread - | StarvationModels.ForNonUIThread -> - ThreadDomain.BGThread + let thread : ThreadDomain.t = + match (thread_constraint : StarvationModels.executor_thread_constraint) with + | ForUIThread -> + UIThread + | ForNonUIThread -> + BGThread + | ForUnknownThread -> + UnknownThread in let work_item = ScheduledWorkItem.{procname; loc; thread} in {astate with scheduled_work= ScheduledWorkDomain.add work_item astate.scheduled_work} diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/UnknownThread.java b/infer/tests/codetoanalyze/java/starvation-whole-program/UnknownThread.java new file mode 100644 index 000000000..dba06ecba --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/UnknownThread.java @@ -0,0 +1,106 @@ +/* + * 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.os.Binder; +import android.os.RemoteException; +import java.util.concurrent.Executor; + +// we treat executors of unknown thread as implicitly running in the background + +class UnknownThread { + static Binder binder; + + private static void doTransact() { + try { + binder.transact(0, null, null, 0); + } catch (RemoteException e) { + } + } + + @ForUiThread private final Executor mUiThreadExecutor = null; + @ForNonUiThread private final Executor mNonUiThreadExecutor = null; + Executor unknownThreadExecutor = null; + + private static Executor getSomeExecutor() { + return null; + } + + public void postBlockingCallToUnknownExecutorFieldOk() { + unknownThreadExecutor.execute( + new Runnable() { + @Override + public void run() { + doTransact(); + } + }); + } + + public void postBlockingCallToUnknownExecutorViaMethodOk() { + getSomeExecutor() + .execute( + new Runnable() { + @Override + public void run() { + doTransact(); + } + }); + } + + Object monitorA, monitorB; + + // text-book deadlock between unknown and background thread + public void postDeadlockToUnknownAndBackgroundBad() { + unknownThreadExecutor.execute( + new Runnable() { + @Override + public void run() { + synchronized (monitorA) { + synchronized (monitorB) { + } + } + } + }); + + mNonUiThreadExecutor.execute( + new Runnable() { + @Override + public void run() { + synchronized (monitorB) { + synchronized (monitorA) { + } + } + } + }); + } + + Object monitorC, monitorD; + + // text-book deadlock between unknown and background thread + public void postDeadlockToUIAndBackgroundBad() { + unknownThreadExecutor.execute( + new Runnable() { + @Override + public void run() { + synchronized (monitorC) { + synchronized (monitorD) { + } + } + } + }); + + mUiThreadExecutor.execute( + new Runnable() { + @Override + public void run() { + synchronized (monitorD) { + synchronized (monitorC) { + } + } + } + }); + } +} diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp index 7b4abe658..cc475155c 100644 --- a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp @@ -20,3 +20,7 @@ codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceC codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onNullBinding(android.content.ComponentName):void, 31, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onNullBinding(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceConnected(android.content.ComponentName,android.os.IBinder):void, 36, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceConnected(ComponentName,IBinder)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceDisconnected(android.content.ComponentName):void, 41, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceDisconnected(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] +codetoanalyze/java/starvation-whole-program/UnknownThread.java, UnknownThread.postDeadlockToUIAndBackgroundBad():void, 84, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void UnknownThread.postDeadlockToUIAndBackgroundBad()`,Method call: `void UnknownThread$5.run()`, locks `this.monitorC` in `class UnknownThread`, locks `this.monitorD` in `class UnknownThread`,[Trace 2] `void UnknownThread.postDeadlockToUIAndBackgroundBad()`,Method call: `void UnknownThread$6.run()`, locks `this.monitorD` in `class UnknownThread`, locks `this.monitorC` in `class UnknownThread`] +codetoanalyze/java/starvation-whole-program/UnknownThread.java, UnknownThread.postDeadlockToUIAndBackgroundBad():void, 95, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void UnknownThread.postDeadlockToUIAndBackgroundBad()`,Method call: `void UnknownThread$6.run()`, locks `this.monitorD` in `class UnknownThread`, locks `this.monitorC` in `class UnknownThread`,[Trace 2] `void UnknownThread.postDeadlockToUIAndBackgroundBad()`,Method call: `void UnknownThread$5.run()`, locks `this.monitorC` in `class UnknownThread`, locks `this.monitorD` in `class UnknownThread`] +codetoanalyze/java/starvation-whole-program/UnknownThread.java, UnknownThread.postDeadlockToUnknownAndBackgroundBad():void, 57, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void UnknownThread.postDeadlockToUnknownAndBackgroundBad()`,Method call: `void UnknownThread$3.run()`, locks `this.monitorA` in `class UnknownThread`, locks `this.monitorB` in `class UnknownThread`,[Trace 2] `void UnknownThread.postDeadlockToUnknownAndBackgroundBad()`,Method call: `void UnknownThread$4.run()`, locks `this.monitorB` in `class UnknownThread`, locks `this.monitorA` in `class UnknownThread`] +codetoanalyze/java/starvation-whole-program/UnknownThread.java, UnknownThread.postDeadlockToUnknownAndBackgroundBad():void, 68, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void UnknownThread.postDeadlockToUnknownAndBackgroundBad()`,Method call: `void UnknownThread$4.run()`, locks `this.monitorB` in `class UnknownThread`, locks `this.monitorA` in `class UnknownThread`,[Trace 2] `void UnknownThread.postDeadlockToUnknownAndBackgroundBad()`,Method call: `void UnknownThread$3.run()`, locks `this.monitorA` in `class UnknownThread`, locks `this.monitorB` in `class UnknownThread`]