From 0c4d2d7a92a7917a4a723e91f86aa30248e4842e Mon Sep 17 00:00:00 2001
From: Nikos Gorogiannis <nikosg@fb.com>
Date: Mon, 18 Nov 2019 01:57:27 -0800
Subject: [PATCH] [starvation-whole-program] recognize Android callback methods
as scheduled work
Summary: Android may spontaneously call these methods on the UI thread, so recognize the fact.
Reviewed By: ezgicicek
Differential Revision: D18530477
fbshipit-source-id: a8a798779
---
infer/src/concurrency/ConcurrencyModels.mli | 3 +
infer/src/concurrency/starvation.ml | 12 +-
.../starvation-whole-program/MyActivity.java | 111 ++++++++++++++++++
.../java/starvation-whole-program/issues.exp | 8 ++
4 files changed, 129 insertions(+), 5 deletions(-)
create mode 100644 infer/tests/codetoanalyze/java/starvation-whole-program/MyActivity.java
diff --git a/infer/src/concurrency/ConcurrencyModels.mli b/infer/src/concurrency/ConcurrencyModels.mli
index d912a2937..baa965e2a 100644
--- a/infer/src/concurrency/ConcurrencyModels.mli
+++ b/infer/src/concurrency/ConcurrencyModels.mli
@@ -61,3 +61,6 @@ val annotated_as_worker_thread :
val runs_on_ui_thread :
attrs_of_pname:(Typ.Procname.t -> ProcAttributes.t option) -> Tenv.t -> Typ.Procname.t -> bool
(** is method not transitively annotated @WorkerThread and is modeled or annotated @UIThread or equivalent? *)
+
+val is_modeled_ui_method : Tenv.t -> Typ.Procname.t -> bool
+(** is method a known Android UI thread callback (eg [Activity.onCreate]) *)
diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml
index 79a919885..0bb125d75 100644
--- a/infer/src/concurrency/starvation.ml
+++ b/infer/src/concurrency/starvation.ml
@@ -578,13 +578,15 @@ let get_summary_of_scheduled_work (work_item : Domain.ScheduledWorkItem.t) =
(* given a summary, do [f work critical_pairs] for each [work] item scheduled in the summary,
where [critical_pairs] are those of the method scheduled, adapted to the thread it's scheduled for *)
-let iter_summary ~f (summary : Summary.t) =
- let caller = Summary.get_proc_name summary in
+let iter_summary ~f exe_env (summary : Summary.t) =
let open Domain in
Payloads.starvation summary.payloads
- |> Option.iter ~f:(fun ({scheduled_work} : summary) ->
+ |> Option.iter ~f:(fun ({scheduled_work; critical_pairs} : summary) ->
+ let pname = Summary.get_proc_name summary in
+ let tenv = Exe_env.get_tenv exe_env pname in
+ if ConcurrencyModels.is_modeled_ui_method tenv pname then f pname critical_pairs ;
ScheduledWorkDomain.iter
- (fun work -> get_summary_of_scheduled_work work |> Option.iter ~f:(f caller))
+ (fun work -> get_summary_of_scheduled_work work |> Option.iter ~f:(f pname))
scheduled_work )
@@ -636,7 +638,7 @@ let whole_program_analysis () =
let work_set = WorkHashSet.create 1 in
let exe_env = Exe_env.mk () in
L.progress "Processing on-disk summaries...@." ;
- SpecsFiles.iter ~f:(iter_summary ~f:(WorkHashSet.add_pairs work_set)) ;
+ SpecsFiles.iter ~f:(iter_summary exe_env ~f:(WorkHashSet.add_pairs work_set)) ;
L.progress "Loaded %d pairs@." (WorkHashSet.length work_set) ;
L.progress "Reporting on processed summaries...@." ;
report exe_env work_set
diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/MyActivity.java b/infer/tests/codetoanalyze/java/starvation-whole-program/MyActivity.java
new file mode 100644
index 000000000..acba89adf
--- /dev/null
+++ b/infer/tests/codetoanalyze/java/starvation-whole-program/MyActivity.java
@@ -0,0 +1,111 @@
+/*
+ * 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.app.Activity;
+import android.os.Binder;
+import android.os.Bundle;
+import android.os.RemoteException;
+import java.util.concurrent.Executor;
+
+class MyActivity extends Activity {
+ Binder b;
+
+ private void bad() {
+ try {
+ b.transact(0, null, null, 0);
+ } catch (RemoteException r) {
+ }
+ }
+
+ // overrides so no Bad suffixes
+
+ @Override
+ protected void onCreate(Bundle savedInstanceState) {
+ bad();
+ }
+
+ @Override
+ public void onStart() {
+ bad();
+ }
+
+ @Override
+ public void onRestart() {
+ bad();
+ }
+
+ Object monitorA;
+ @ForNonUiThread private final Executor mNonUiThreadExecutor = null;
+
+ // method is a UI thread callback, and schedules a transaction in the background
+ // but it synchronises on the lock protecting the transaction, thus stalling the main thread
+ @Override
+ public void onStop() {
+ synchronized (monitorA) {
+ }
+
+ mNonUiThreadExecutor.execute(
+ new Runnable() {
+ @Override
+ public void run() {
+ synchronized (monitorA) {
+ bad();
+ }
+ }
+ });
+ }
+
+ Object monitorB, monitorC;
+
+ // method is a UI thread callback and deadlocks with work scheduled in
+ // another callback (onPause) but which schedules work to a background thread
+ @Override
+ public void onDestroy() {
+ synchronized (monitorC) {
+ synchronized (monitorB) {
+ }
+ }
+ }
+
+ @Override
+ public void onPause() {
+ mNonUiThreadExecutor.execute(
+ new Runnable() {
+ @Override
+ public void run() {
+ synchronized (monitorB) {
+ synchronized (monitorC) {
+ }
+ }
+ }
+ });
+ }
+
+ Object FP_monitorD, FP_monitorE;
+
+ // False positive: by the time the work is scheduled, no lock is held, so no deadlock
+ // Locks are named FP_* so that the report is clearly an FP (we can't change the name of the
+ // override).
+ @Override
+ public void onResume() {
+ synchronized (FP_monitorD) {
+ synchronized (FP_monitorE) {
+ }
+ }
+
+ mNonUiThreadExecutor.execute(
+ new Runnable() {
+ @Override
+ public void run() {
+ synchronized (FP_monitorE) {
+ synchronized (FP_monitorD) {
+ }
+ }
+ }
+ });
+ }
+}
diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp
index 38ef75db1..d385fef9e 100644
--- a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp
+++ b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp
@@ -5,3 +5,11 @@ codetoanalyze/java/starvation-whole-program/Deadlock.java, Deadlock.postOnBGThre
codetoanalyze/java/starvation-whole-program/DirectStarvation.java, DirectStarvation.postBlockingCallToUIThreadBad():void, 29, STARVATION, no_bucket, ERROR, [`void DirectStarvation.postBlockingCallToUIThreadBad()`,Method call: `void DirectStarvation$1.run()`,Method call: `void DirectStarvation.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
codetoanalyze/java/starvation-whole-program/IndirectStarvation.java, IndirectStarvation.postBlockingCallToBackgroundThreadAndLockBad():void, 32, STARVATION, no_bucket, ERROR, [[Trace 1] `void IndirectStarvation.postBlockingCallToBackgroundThreadAndLockBad()`,Method call: `void IndirectStarvation$1.run()`, locks `this.monitorA` in `class IndirectStarvation`,[Trace 2] `void IndirectStarvation.postBlockingCallToBackgroundThreadAndLockBad()`,Method call: `void IndirectStarvation$2.run()`, locks `this.monitorA` in `class IndirectStarvation`,Method call: `void IndirectStarvation.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
codetoanalyze/java/starvation-whole-program/ModeledExecutors.java, ModeledExecutors.postBlockingCallToUIThreadBad():void, 25, STARVATION, no_bucket, ERROR, [`void ModeledExecutors.postBlockingCallToUIThreadBad()`,Method call: `void ModeledExecutors$1.run()`,Method call: `void ModeledExecutors.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onCreate(android.os.Bundle):void, 28, STARVATION, no_bucket, ERROR, [`void MyActivity.onCreate(Bundle)`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onDestroy():void, 68, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void MyActivity.onDestroy()`, locks `this.monitorC` in `class MyActivity`, locks `this.monitorB` in `class MyActivity`,[Trace 2] `void MyActivity.onPause()`,Method call: `void MyActivity$2.run()`, locks `this.monitorB` in `class MyActivity`, locks `this.monitorC` in `class MyActivity`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onPause():void, 76, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void MyActivity.onPause()`,Method call: `void MyActivity$2.run()`, locks `this.monitorB` in `class MyActivity`, locks `this.monitorC` in `class MyActivity`,[Trace 2] `void MyActivity.onDestroy()`, locks `this.monitorC` in `class MyActivity`, locks `this.monitorB` in `class MyActivity`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onRestart():void, 38, STARVATION, no_bucket, ERROR, [`void MyActivity.onRestart()`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onResume():void, 95, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void MyActivity.onResume()`, locks `this.FP_monitorD` in `class MyActivity`, locks `this.FP_monitorE` in `class MyActivity`,[Trace 2] `void MyActivity.onResume()`,Method call: `void MyActivity$3.run()`, locks `this.FP_monitorE` in `class MyActivity`, locks `this.FP_monitorD` in `class MyActivity`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onResume():void, 100, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void MyActivity.onResume()`,Method call: `void MyActivity$3.run()`, locks `this.FP_monitorE` in `class MyActivity`, locks `this.FP_monitorD` in `class MyActivity`,[Trace 2] `void MyActivity.onResume()`, locks `this.FP_monitorD` in `class MyActivity`, locks `this.FP_monitorE` in `class MyActivity`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onStart():void, 33, STARVATION, no_bucket, ERROR, [`void MyActivity.onStart()`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]
+codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onStop():void, 48, STARVATION, no_bucket, ERROR, [[Trace 1] `void MyActivity.onStop()`, locks `this.monitorA` in `class MyActivity`,[Trace 2] `void MyActivity.onStop()`,Method call: `void MyActivity$1.run()`, locks `this.monitorA` in `class MyActivity`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]