From 0da57a8b67f848dcd5ef3857b7c7ab17d587077f Mon Sep 17 00:00:00 2001 From: Etan Cohen Date: Wed, 22 May 2019 09:17:33 -0700 Subject: [PATCH 1/2] [CM] Fix NPE due to unvalidated callback value When unregistering callback due to ON_UNAVAILABLE did not check for a non-null callback. Bug: 132950880 Test: atest ConnectivityServiceTest Merged-In: Ib3fde31d88c36469cdee1e3578606d130a9817cb Change-Id: Ib3fde31d88c36469cdee1e3578606d130a9817cb (cherry picked from commit 51ddc176abd23bd3ddbc26124e5541a983a1db07) --- .../java/android/net/ConnectivityManager.java | 9 +++-- .../server/ConnectivityServiceTest.java | 38 ++++++++++++++----- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index 3bc40a7999..fbfbfc04d8 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -3449,6 +3449,11 @@ public class ConnectivityManager { final NetworkCallback callback; synchronized (sCallbacks) { callback = sCallbacks.get(request); + if (callback == null) { + Log.w(TAG, + "callback not found for " + getCallbackName(message.what) + " message"); + return; + } if (message.what == CALLBACK_UNAVAIL) { sCallbacks.remove(request); callback.networkRequest = ALREADY_UNREGISTERED; @@ -3457,10 +3462,6 @@ public class ConnectivityManager { if (DBG) { Log.d(TAG, getCallbackName(message.what) + " for network " + network); } - if (callback == null) { - Log.w(TAG, "callback not found for " + getCallbackName(message.what) + " message"); - return; - } switch (message.what) { case CALLBACK_PRECHECK: { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index b0cc20785c..03af9672c1 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -3816,11 +3816,20 @@ public class ConnectivityServiceTest { networkCallback.assertNoCallback(); } + @Test + public void testUnfulfillableNetworkRequest() throws Exception { + runUnfulfillableNetworkRequest(false); + } + + @Test + public void testUnfulfillableNetworkRequestAfterUnregister() throws Exception { + runUnfulfillableNetworkRequest(true); + } + /** * Validate the callback flow for a factory releasing a request as unfulfillable. */ - @Test - public void testUnfulfillableNetworkRequest() throws Exception { + private void runUnfulfillableNetworkRequest(boolean preUnregister) throws Exception { NetworkRequest nr = new NetworkRequest.Builder().addTransportType( NetworkCapabilities.TRANSPORT_WIFI).build(); final TestNetworkCallback networkCallback = new TestNetworkCallback(); @@ -3855,14 +3864,25 @@ public class ConnectivityServiceTest { } } - // Simulate the factory releasing the request as unfulfillable and expect onUnavailable! - testFactory.expectRemoveRequests(1); - testFactory.triggerUnfulfillable(requests.get(newRequestId)); - networkCallback.expectCallback(CallbackState.UNAVAILABLE, null); - testFactory.waitForRequests(); + if (preUnregister) { + mCm.unregisterNetworkCallback(networkCallback); - // unregister network callback - a no-op, but should not fail - mCm.unregisterNetworkCallback(networkCallback); + // Simulate the factory releasing the request as unfulfillable: no-op since + // the callback has already been unregistered (but a test that no exceptions are + // thrown). + testFactory.triggerUnfulfillable(requests.get(newRequestId)); + } else { + // Simulate the factory releasing the request as unfulfillable and expect onUnavailable! + testFactory.expectRemoveRequests(1); + testFactory.triggerUnfulfillable(requests.get(newRequestId)); + + networkCallback.expectCallback(CallbackState.UNAVAILABLE, null); + testFactory.waitForRequests(); + + // unregister network callback - a no-op (since already freed by the + // on-unavailable), but should not fail or throw exceptions. + mCm.unregisterNetworkCallback(networkCallback); + } testFactory.unregister(); handlerThread.quit(); From c8289e40a4c837e40fc402bada475f2319e179eb Mon Sep 17 00:00:00 2001 From: Etan Cohen Date: Thu, 23 May 2019 09:16:26 -0700 Subject: [PATCH 2/2] [CM] Fix NPE due to unvalidated callback value Fix flaky test resulting from the above fix. Bug: 132950880 Test: atest ConnectivityServiceTest Merged-In: Ia2cc04b42288ea987483e5ab0e0a10093dc49502 Change-Id: Ia2cc04b42288ea987483e5ab0e0a10093dc49502 (cherry picked from commit cc65a628eb1172dc7b942d7f51b702099c15c23b) --- tests/net/java/com/android/server/ConnectivityServiceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 03af9672c1..fa059fa9e8 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -3864,6 +3864,7 @@ public class ConnectivityServiceTest { } } + testFactory.expectRemoveRequests(1); if (preUnregister) { mCm.unregisterNetworkCallback(networkCallback); @@ -3873,7 +3874,6 @@ public class ConnectivityServiceTest { testFactory.triggerUnfulfillable(requests.get(newRequestId)); } else { // Simulate the factory releasing the request as unfulfillable and expect onUnavailable! - testFactory.expectRemoveRequests(1); testFactory.triggerUnfulfillable(requests.get(newRequestId)); networkCallback.expectCallback(CallbackState.UNAVAILABLE, null);