From 0bad224c44fcbe34a5ecaadb1debe4c8e896c7e5 Mon Sep 17 00:00:00 2001
From: Chalard Jean <jchalard@google.com>
Date: Tue, 27 Jul 2021 04:54:52 +0000
Subject: [PATCH] Fix a possible system server crash

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
Merged-In: I82a28c37450146838410bf5a059aac295a985fca
Change-Id: Iddab205cf2754d326be816e6e8e92c2cc0b95771
(cherry picked from commit d79bd5c622d7a76b37a71f8f131697cba079c6e9)
---
 .../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 7c7c32df3c..c8a0b7f0d9 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -5890,7 +5890,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 103ed8bbe6..0f0ac1223c 100644
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -278,6 +278,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;
@@ -2174,6 +2175,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