From 5bcc838f4e5923c069e9554a9d050d4286a81790 Mon Sep 17 00:00:00 2001
From: Chalard Jean <jchalard@google.com>
Date: Mon, 19 Jul 2021 19:57:02 +0900
Subject: [PATCH] Fix a possible system server crash
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The scenario is as follows : an app registers a network callback,
then unregisters it and dies immediately after. In this scenario,
the system server will receive a notification of the binder death
and enqueue a call to handleRemoveNetworkRequest. If the callback
unregister message has been process first, this call would result
in unlinkToDeath being called twice on the same Binder, crashing.
This patch fixes the problem by using handleReleaseNetworkRequest
instead of Remove, which looks up the NRI in a map on the handler
thread before calling Remove, returning without doing anything if
the NRI has already been removed.

Test: ConnectivityServiceTest
Test: New test for this
Bug: 194394697
Change-Id: I82a28c37450146838410bf5a059aac295a985fca
---
 .../android/server/ConnectivityService.java   |  8 +++-
 .../server/ConnectivityServiceTest.java       | 40 +++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 770aa8a7cf..21c5abd7d1 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -5909,7 +5909,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
         public void binderDied() {
             log("ConnectivityService NetworkRequestInfo binderDied(" +
                     "uid/pid:" + mUid + "/" + mPid + ", " + mBinder + ")");
-            mHandler.post(() -> handleRemoveNetworkRequest(this));
+            // As an immutable collection, mRequests cannot change by the time the
+            // lambda is evaluated on the handler thread so calling .get() from a binder thread
+            // is acceptable. Use handleReleaseNetworkRequest and not directly
+            // handleRemoveNetworkRequest so as to force a lookup in the requests map, in case
+            // the app already unregistered the request.
+            mHandler.post(() -> handleReleaseNetworkRequest(mRequests.get(0),
+                    mUid, false /* callOnUnavailable */));
         }
 
         @Override
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 496102402e..432e03db4b 100644
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -280,6 +280,7 @@ import android.os.HandlerThread;
 import android.os.IBinder;
 import android.os.INetworkManagementService;
 import android.os.Looper;
+import android.os.Messenger;
 import android.os.Parcel;
 import android.os.ParcelFileDescriptor;
 import android.os.Parcelable;
@@ -2168,6 +2169,45 @@ public class ConnectivityServiceTest {
         mCm.unregisterNetworkCallback(fgMobileListenCallback);
     }
 
+    @Test
+    public void testBinderDeathAfterUnregister() throws Exception {
+        final NetworkCapabilities caps = new NetworkCapabilities.Builder()
+                .addTransportType(TRANSPORT_WIFI)
+                .build();
+        final Handler handler = new Handler(ConnectivityThread.getInstanceLooper());
+        final Messenger messenger = new Messenger(handler);
+        final CompletableFuture<Binder.DeathRecipient> deathRecipient = new CompletableFuture<>();
+        final Binder binder = new Binder() {
+            private DeathRecipient mDeathRecipient;
+            @Override
+            public void linkToDeath(@NonNull final DeathRecipient recipient, final int flags) {
+                synchronized (this) {
+                    mDeathRecipient = recipient;
+                }
+                super.linkToDeath(recipient, flags);
+                deathRecipient.complete(recipient);
+            }
+
+            @Override
+            public boolean unlinkToDeath(@NonNull final DeathRecipient recipient, final int flags) {
+                synchronized (this) {
+                    if (null == mDeathRecipient) {
+                        throw new IllegalStateException();
+                    }
+                    mDeathRecipient = null;
+                }
+                return super.unlinkToDeath(recipient, flags);
+            }
+        };
+        final NetworkRequest request = mService.listenForNetwork(caps, messenger, binder,
+                NetworkCallback.FLAG_NONE, mContext.getOpPackageName(),
+                mContext.getAttributionTag());
+        mService.releaseNetworkRequest(request);
+        deathRecipient.get().binderDied();
+        // Wait for the release message to be processed.
+        waitForIdle();
+    }
+
     @Test
     public void testValidatedCellularOutscoresUnvalidatedWiFi() throws Exception {
         // Test bringing up unvalidated WiFi