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)
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 7c7c32d..c8a0b7f 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -5890,7 +5890,13 @@
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 103ed8b..0f0ac12 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.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;
@@ -2175,6 +2176,45 @@
}
@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
mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI);