From 31f1ff693b4429278ef6cfe4fafe2487c480f229 Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Thu, 5 Mar 2020 10:46:02 -0800 Subject: [PATCH] Use IBinder as key for ConnectivityDiagnostics storage in CS. This change updates ConnectivityService to use IBinder instances as keys when storing ConnectivityDiagnosticsCallbacks. When storing ConnectivityDiagnosticsCallbacks in ConnectivityService, the IConnectivityDiagnsoticsCallback is used as the key for ConnectivityService.mConnectivityDiagnosticsCallbacks. However, IConnectivityDiagnosticsCallback instances are received as different objects. This causes them to produce different hashCode() values, so attempts to remove an IConnectivityDiagnosticsCallback fail. Bug: 150867635 Test: atest FrameworksNetTests Change-Id: Ib99e68d5ae47fa27e12428f9a60a2c1204ac59a2 --- .../android/server/ConnectivityService.java | 24 +++++++++++-------- .../server/ConnectivityServiceTest.java | 16 ++++--------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index c48ee276c8..916a68d398 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -653,8 +653,8 @@ public class ConnectivityService extends IConnectivityManager.Stub final MultipathPolicyTracker mMultipathPolicyTracker; @VisibleForTesting - final Map - mConnectivityDiagnosticsCallbacks = new HashMap<>(); + final Map mConnectivityDiagnosticsCallbacks = + new HashMap<>(); /** * Implements support for the legacy "one network per network type" model. @@ -7796,11 +7796,12 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureRunningOnConnectivityServiceThread(); final IConnectivityDiagnosticsCallback cb = cbInfo.mCb; + final IBinder iCb = cb.asBinder(); final NetworkRequestInfo nri = cbInfo.mRequestInfo; // This means that the client registered the same callback multiple times. Do // not override the previous entry, and exit silently. - if (mConnectivityDiagnosticsCallbacks.containsKey(cb)) { + if (mConnectivityDiagnosticsCallbacks.containsKey(iCb)) { if (VDBG) log("Diagnostics callback is already registered"); // Decrement the reference count for this NetworkRequestInfo. The reference count is @@ -7810,10 +7811,10 @@ public class ConnectivityService extends IConnectivityManager.Stub return; } - mConnectivityDiagnosticsCallbacks.put(cb, cbInfo); + mConnectivityDiagnosticsCallbacks.put(iCb, cbInfo); try { - cb.asBinder().linkToDeath(cbInfo, 0); + iCb.linkToDeath(cbInfo, 0); } catch (RemoteException e) { cbInfo.binderDied(); return; @@ -7850,13 +7851,14 @@ public class ConnectivityService extends IConnectivityManager.Stub private void handleUnregisterConnectivityDiagnosticsCallback( @NonNull IConnectivityDiagnosticsCallback cb, int uid) { ensureRunningOnConnectivityServiceThread(); + final IBinder iCb = cb.asBinder(); - if (!mConnectivityDiagnosticsCallbacks.containsKey(cb)) { + if (!mConnectivityDiagnosticsCallbacks.containsKey(iCb)) { if (VDBG) log("Removing diagnostics callback that is not currently registered"); return; } - final NetworkRequestInfo nri = mConnectivityDiagnosticsCallbacks.get(cb).mRequestInfo; + final NetworkRequestInfo nri = mConnectivityDiagnosticsCallbacks.get(iCb).mRequestInfo; if (uid != nri.mUid) { if (VDBG) loge("Different uid than registrant attempting to unregister cb"); @@ -7868,7 +7870,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // enforceRequestCountLimit(). decrementNetworkRequestPerUidCount(nri); - cb.asBinder().unlinkToDeath(mConnectivityDiagnosticsCallbacks.remove(cb), 0); + final ConnectivityDiagnosticsCallbackInfo cbInfo = + mConnectivityDiagnosticsCallbacks.remove(iCb); + iCb.unlinkToDeath(cbInfo, 0); } private void handleNetworkTestedWithExtras( @@ -7943,14 +7947,14 @@ public class ConnectivityService extends IConnectivityManager.Stub private List getMatchingPermissionedCallbacks( @NonNull NetworkAgentInfo nai) { final List results = new ArrayList<>(); - for (Entry entry : + for (Entry entry : mConnectivityDiagnosticsCallbacks.entrySet()) { final ConnectivityDiagnosticsCallbackInfo cbInfo = entry.getValue(); final NetworkRequestInfo nri = cbInfo.mRequestInfo; if (nai.satisfies(nri.request)) { if (checkConnectivityDiagnosticsPermissions( nri.mPid, nri.mUid, nai, cbInfo.mCallingPackageName)) { - results.add(entry.getKey()); + results.add(entry.getValue().mCb); } } } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 1d7d3c0ae2..7b9d2bd452 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -6744,16 +6744,12 @@ public class ConnectivityServiceTest { verify(mIBinder).linkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); verify(mConnectivityDiagnosticsCallback).asBinder(); - assertTrue( - mService.mConnectivityDiagnosticsCallbacks.containsKey( - mConnectivityDiagnosticsCallback)); + assertTrue(mService.mConnectivityDiagnosticsCallbacks.containsKey(mIBinder)); mService.unregisterConnectivityDiagnosticsCallback(mConnectivityDiagnosticsCallback); verify(mIBinder, timeout(TIMEOUT_MS)) .unlinkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); - assertFalse( - mService.mConnectivityDiagnosticsCallbacks.containsKey( - mConnectivityDiagnosticsCallback)); + assertFalse(mService.mConnectivityDiagnosticsCallbacks.containsKey(mIBinder)); verify(mConnectivityDiagnosticsCallback, atLeastOnce()).asBinder(); } @@ -6771,9 +6767,7 @@ public class ConnectivityServiceTest { verify(mIBinder).linkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); verify(mConnectivityDiagnosticsCallback).asBinder(); - assertTrue( - mService.mConnectivityDiagnosticsCallbacks.containsKey( - mConnectivityDiagnosticsCallback)); + assertTrue(mService.mConnectivityDiagnosticsCallbacks.containsKey(mIBinder)); // Register the same callback again mService.registerConnectivityDiagnosticsCallback( @@ -6782,9 +6776,7 @@ public class ConnectivityServiceTest { // Block until all other events are done processing. HandlerUtilsKt.waitForIdle(mCsHandlerThread, TIMEOUT_MS); - assertTrue( - mService.mConnectivityDiagnosticsCallbacks.containsKey( - mConnectivityDiagnosticsCallback)); + assertTrue(mService.mConnectivityDiagnosticsCallbacks.containsKey(mIBinder)); } @Test