commit 16a691f87b8b00fe32f2faf1d7e222d770da7745
author Martijn Coenen <maco@google.com> Thu Sep 24 19:49:35 2020 +0200
committer android-build-team Robot <android-build-team-robot@google.com> Fri Sep 25 21:01:23 2020 +0000
tree b6bc0a1de8f13d0388471465ceca55e6bfdd0cc0
parent 6cb400d3ac6d48be026f734695a18bdcbe821fff

Don't dump binder proxies with the lock held.

There's still a path where the AMS lock is held when we're trying to
dump binder proxies; in this case, just don't dump any proxies at all,
since there's no safe way to do it with the AMS lock held.

Bug: 168353824
Test: adb shell am dump binder-proxies
Change-Id: Ia9b23821a953ac73fe6d67cc169998548680083d
(cherry picked from commit 7174ebbf55f9fe2dc53dbf17e812298b220d98db)

services/core/java/com/android/server/am/ActivityManagerService.java

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 491579a..bf92f4e 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -10530,7 +10530,7 @@
 
     private void dumpEverything(FileDescriptor fd, PrintWriter pw, String[] args, int opti,
             boolean dumpAll, String dumpPackage, boolean dumpClient, boolean dumpNormalPriority,
-            int dumpAppId) {
+            int dumpAppId, boolean dumpProxies) {
 
         ActiveServices.ServiceDumper sdumper;
 
@@ -10589,7 +10589,7 @@
             }
             sdumper.dumpWithClient();
         }
-        if (dumpPackage == null) {
+        if (dumpPackage == null && dumpProxies) {
             // Intentionally dropping the lock for this, because dumpBinderProxies() will make many
             // outgoing binder calls to retrieve interface descriptors; while that is system code,
             // there is nothing preventing an app from overriding this implementation by talking to
@@ -10998,13 +10998,14 @@
                 // dumpEverything() will take the lock when needed, and momentarily drop
                 // it for dumping client state.
                 dumpEverything(fd, pw, args, opti, dumpAll, dumpPackage, dumpClient,
-                        dumpNormalPriority, dumpAppId);
+                        dumpNormalPriority, dumpAppId, true /* dumpProxies */);
             } else {
                 // Take the lock here, so we get a consistent state for the entire dump;
-                // dumpEverything() will take the lock as well, but that is fine.
+                // dumpEverything() will take the lock as well, which is fine for everything
+                // except dumping proxies, which can take a long time; exclude them.
                 synchronized(this) {
                     dumpEverything(fd, pw, args, opti, dumpAll, dumpPackage, dumpClient,
-                            dumpNormalPriority, dumpAppId);
+                            dumpNormalPriority, dumpAppId, false /* dumpProxies */);
                 }
             }
         }