From 36c998c6b7905d31766390733ea0c8d542e8d7a0 Mon Sep 17 00:00:00 2001
From: Christopher Tate <ctate@google.com>
Date: Tue, 28 May 2019 12:31:50 -0700
Subject: [PATCH] Prevent double teardown of service connections

Asynchronicities in activity teardown -> service connection teardown
introduced a race in which the teardown could race with new service
bindings to "the same" service instance, and then wind up attempting to
shut down a new, valid instance inappropriately.  Fixed by making sure
to clear the "what needs to be torn down" bookkeeping as part of the
act of doing that teardown, removing the possibility for stale state.

Fixes: 131029480
Test: manual
Test: atest CtsAppTestCases
Change-Id: I33a63f524d147ff6ec97dd3efb0127dcace8bf3c
---
 .../android/app/ActivityManagerInternal.java    |  2 +-
 .../server/am/ActivityManagerService.java       | 17 ++++++++++++-----
 .../wm/ActivityServiceConnectionsHolder.java    |  8 +++++++-
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/core/java/android/app/ActivityManagerInternal.java b/core/java/android/app/ActivityManagerInternal.java
index 500a04fc7be5c..46a520a5289e2 100644
--- a/core/java/android/app/ActivityManagerInternal.java
+++ b/core/java/android/app/ActivityManagerInternal.java
@@ -278,7 +278,7 @@ public abstract class ActivityManagerInternal {
             String resolvedType, boolean fgRequired, String callingPackage, int userId,
             boolean allowBackgroundActivityStarts) throws TransactionTooLargeException;
 
-    public abstract void disconnectActivityFromServices(Object connectionHolder);
+    public abstract void disconnectActivityFromServices(Object connectionHolder, Object conns);
     public abstract void cleanUpServices(int userId, ComponentName component, Intent baseIntent);
     public abstract ActivityInfo getActivityInfoForUser(ActivityInfo aInfo, int userId);
     public abstract void ensureBootCompleted();
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 0619eb95dec78..2321031e30d8a 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -18156,13 +18156,20 @@ public class ActivityManagerService extends IActivityManager.Stub
             }
         }
 
+        // The arguments here are untyped because the base ActivityManagerInternal class
+        // doesn't have compile-time visiblity into ActivityServiceConnectionHolder or
+        // ConnectionRecord.
         @Override
-        public void disconnectActivityFromServices(Object connectionHolder) {
+        public void disconnectActivityFromServices(Object connectionHolder, Object conns) {
+            // 'connectionHolder' is an untyped ActivityServiceConnectionsHolder
+            // 'conns' is an untyped HashSet<ConnectionRecord>
+            final ActivityServiceConnectionsHolder holder =
+                    (ActivityServiceConnectionsHolder) connectionHolder;
+            final HashSet<ConnectionRecord> toDisconnect = (HashSet<ConnectionRecord>) conns;
             synchronized(ActivityManagerService.this) {
-                final ActivityServiceConnectionsHolder c =
-                        (ActivityServiceConnectionsHolder) connectionHolder;
-                c.forEachConnection(cr -> mServices.removeConnectionLocked(
-                        (ConnectionRecord) cr, null, c));
+                for (ConnectionRecord cr : toDisconnect) {
+                    mServices.removeConnectionLocked(cr, null, holder);
+                }
             }
         }
 
diff --git a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
index ad4624875d063..c56a9e2ac5600 100644
--- a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
+++ b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
@@ -101,7 +101,13 @@ public class ActivityServiceConnectionsHolder<T> {
         if (mConnections == null || mConnections.isEmpty()) {
             return;
         }
-        mService.mH.post(() -> mService.mAmInternal.disconnectActivityFromServices(this));
+        // Capture and null out mConnections, to guarantee that we process
+        // disconnect of these specific connections exactly once even if
+        // we're racing with rapid activity lifecycle churn and this
+        // method is invoked more than once on this object.
+        final Object disc = mConnections;
+        mConnections = null;
+        mService.mH.post(() -> mService.mAmInternal.disconnectActivityFromServices(this, disc));
     }
 
     public void dump(PrintWriter pw, String prefix) {