From dfa390e0802bbbdd00d7911f893cc94ec57bf159 Mon Sep 17 00:00:00 2001
From: Martijn Coenen <maco@google.com>
Date: Tue, 5 Jun 2018 11:02:23 +0200
Subject: [PATCH] Serialize calls into BinderProxy.

The BinderProxy class is not thread-safe, hence all calls into it
must be serialized. This was achieved by holding the gProxyLock in
JNI code. However, a recent change added calls into BinderProxy
from ActivityManagerService without holding that lock, causing
ConcurrentModificationExceptions.

Instead of dumping debug info from AMS, make the call directly
from JNI, so we can make sure gProxyLock is held correctly.

Also, only dump on debug builds.

Bug: 71353150
Bug: 109701487
Test: sailfish builds, boots, info gets dumped with lowered limits.
Change-Id: I446a71ce4115b9936a01a170401ef98ba3818c0b
---
 core/java/android/os/Binder.java              | 28 ++++++++++---------
 core/jni/android_util_Binder.cpp              | 21 ++++++++++++++
 .../server/am/ActivityManagerService.java     |  1 -
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java
index 3d76c2501440d..8e8565c3574ca 100644
--- a/core/java/android/os/Binder.java
+++ b/core/java/android/os/Binder.java
@@ -137,15 +137,6 @@ public class Binder implements IBinder {
         sTracingEnabled = false;
     }
 
-    /**
-     * Dump proxy debug information.
-     *
-     * @hide
-     */
-    public static void dumpProxyDebugInfo() {
-        BinderProxy.dumpProxyDebugInfo();
-    }
-
     /**
      * Check if binder transaction tracing is enabled.
      *
@@ -950,7 +941,8 @@ final class BinderProxy implements IBinder {
                     // about to crash.
                     final int totalUnclearedSize = unclearedSize();
                     if (totalUnclearedSize >= CRASH_AT_SIZE) {
-                        dumpProxyDebugInfo();
+                        dumpProxyInterfaceCounts();
+                        dumpPerUidProxyCounts();
                         Runtime.getRuntime().gc();
                         throw new AssertionError("Binder ProxyMap has too many entries: "
                                 + totalSize + " (total), " + totalUnclearedSize + " (uncleared), "
@@ -1035,11 +1027,21 @@ final class BinderProxy implements IBinder {
     private static ProxyMap sProxyMap = new ProxyMap();
 
     /**
+      * Dump proxy debug information.
+      *
+      * Note: this method is not thread-safe; callers must serialize with other
+      * accesses to sProxyMap, in particular {@link #getInstance(long, long)}.
+      *
       * @hide
       */
-    public static void dumpProxyDebugInfo() {
-        sProxyMap.dumpProxyInterfaceCounts();
-        sProxyMap.dumpPerUidProxyCounts();
+    private static void dumpProxyDebugInfo() {
+        if (Build.IS_DEBUGGABLE) {
+            sProxyMap.dumpProxyInterfaceCounts();
+            // Note that we don't call dumpPerUidProxyCounts(); this is because this
+            // method may be called as part of the uid limit being hit, and calling
+            // back into the UID tracking code would cause us to try to acquire a mutex
+            // that is held during that callback.
+        }
     }
 
     /**
diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp
index 1b206fd8abddd..c3ba9ba828268 100644
--- a/core/jni/android_util_Binder.cpp
+++ b/core/jni/android_util_Binder.cpp
@@ -108,6 +108,7 @@ static struct binderproxy_offsets_t
     jclass mClass;
     jmethodID mGetInstance;
     jmethodID mSendDeathNotice;
+    jmethodID mDumpProxyDebugInfo;
 
     // Object state.
     jfieldID mNativeData;  // Field holds native pointer to BinderProxyNativeData.
@@ -1006,9 +1007,27 @@ static void android_os_BinderInternal_handleGc(JNIEnv* env, jobject clazz)
 static void android_os_BinderInternal_proxyLimitcallback(int uid)
 {
     JNIEnv *env = AndroidRuntime::getJNIEnv();
+    {
+        // Calls into BinderProxy must be serialized
+        AutoMutex _l(gProxyLock);
+        env->CallStaticObjectMethod(gBinderProxyOffsets.mClass,
+                                    gBinderProxyOffsets.mDumpProxyDebugInfo);
+    }
+    if (env->ExceptionCheck()) {
+        ScopedLocalRef<jthrowable> excep(env, env->ExceptionOccurred());
+        report_exception(env, excep.get(),
+            "*** Uncaught exception in dumpProxyDebugInfo");
+    }
+
     env->CallStaticVoidMethod(gBinderInternalOffsets.mClass,
                               gBinderInternalOffsets.mProxyLimitCallback,
                               uid);
+
+    if (env->ExceptionCheck()) {
+        ScopedLocalRef<jthrowable> excep(env, env->ExceptionOccurred());
+        report_exception(env, excep.get(),
+            "*** Uncaught exception in binderProxyLimitCallbackFromNative");
+    }
 }
 
 static void android_os_BinderInternal_setBinderProxyCountEnabled(JNIEnv* env, jobject clazz,
@@ -1389,6 +1408,8 @@ static int int_register_android_os_BinderProxy(JNIEnv* env)
             "(JJ)Landroid/os/BinderProxy;");
     gBinderProxyOffsets.mSendDeathNotice = GetStaticMethodIDOrDie(env, clazz, "sendDeathNotice",
             "(Landroid/os/IBinder$DeathRecipient;)V");
+    gBinderProxyOffsets.mDumpProxyDebugInfo = GetStaticMethodIDOrDie(env, clazz, "dumpProxyDebugInfo",
+            "()V");
     gBinderProxyOffsets.mNativeData = GetFieldIDOrDie(env, clazz, "mNativeData", "J");
 
     clazz = FindClassOrDie(env, "java/lang/Class");
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index e95a9327cfb32..c25f8ff843a84 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -15305,7 +15305,6 @@ public class ActivityManagerService extends IActivityManager.Stub
                         public void onLimitReached(int uid) {
                             Slog.wtf(TAG, "Uid " + uid + " sent too many Binders to uid "
                                     + Process.myUid());
-                            Binder.dumpProxyDebugInfo();
                             if (uid == Process.SYSTEM_UID) {
                                 Slog.i(TAG, "Skipping kill (uid is SYSTEM)");
                             } else {