From a590ab837304e93783e5075af08031c1c41bf8ef Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Thu, 11 May 2017 13:16:17 +0900 Subject: [PATCH 1/2] ConnectivityManager: uses service error codes and exceptions This patch introduces between ConnectivityManager and ConnectivityService a mechanism for propagating back to clients informative errors in the form of error codes and associated custom runtime exceptions. Without error code, the service can only throw a limited number of different exceptions over Binder. Furthermore the throw site stack traces are always loss. Although for individual instances of a throw, the error message can be inspected, aggregations of stack traces from app crashes sanitize error messages and only leaves the stack traces. This makes debugging dificult for some service calls such as requestNetwork that can have a variety of failure modes. In this patch only one failure mode is codified. More can be added later at a light cost by: 1) defining an error code, 2) defining an associated exception, 3) mapping the code to the exception. This patch can serves as a template for doing so. Test: $ runtest frameworks-net, #testNetworkRequestMaximum() detects the new exception type. Bug: 36556809, 36701874 Change-Id: I611fd7915575c9e418f7149fcdc8a879d2a3716d --- .../java/android/net/ConnectivityManager.java | 29 +++++++++++++++++++ .../android/server/ConnectivityService.java | 4 ++- .../server/ConnectivityServiceTest.java | 9 +++--- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index efa195983f..6320134921 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -39,6 +39,7 @@ import android.os.Process; import android.os.RemoteException; import android.os.ResultReceiver; import android.os.ServiceManager; +import android.os.ServiceSpecificException; import android.provider.Settings; import android.telephony.SubscriptionManager; import android.util.ArrayMap; @@ -2701,6 +2702,28 @@ public class ConnectivityManager { } } + /** + * Constant error codes used by ConnectivityService to communicate about failures and errors + * across a Binder boundary. + * @hide + */ + public interface Errors { + static int TOO_MANY_REQUESTS = 1; + } + + /** @hide */ + public static class TooManyRequestsException extends RuntimeException {} + + private static RuntimeException convertServiceException(ServiceSpecificException e) { + switch (e.errorCode) { + case Errors.TOO_MANY_REQUESTS: + return new TooManyRequestsException(); + default: + Log.w(TAG, "Unknown service error code " + e.errorCode); + return new RuntimeException(e); + } + } + private static final int BASE = Protocol.BASE_CONNECTIVITY_MANAGER; /** @hide */ public static final int CALLBACK_PRECHECK = BASE + 1; @@ -2892,6 +2915,8 @@ public class ConnectivityManager { } } catch (RemoteException e) { throw e.rethrowFromSystemServer(); + } catch (ServiceSpecificException e) { + throw convertServiceException(e); } return request; } @@ -3177,6 +3202,8 @@ public class ConnectivityManager { mService.pendingRequestForNetwork(request.networkCapabilities, operation); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); + } catch (ServiceSpecificException e) { + throw convertServiceException(e); } } @@ -3279,6 +3306,8 @@ public class ConnectivityManager { mService.pendingListenForNetwork(request.networkCapabilities, operation); } catch (RemoteException e) { throw e.rethrowFromSystemServer(); + } catch (ServiceSpecificException e) { + throw convertServiceException(e); } } diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 88bc54d267..4f7b834448 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -98,6 +98,7 @@ import android.os.PowerManager; import android.os.Process; import android.os.RemoteException; import android.os.ResultReceiver; +import android.os.ServiceSpecificException; import android.os.SystemClock; import android.os.UserHandle; import android.os.UserManager; @@ -4064,7 +4065,8 @@ public class ConnectivityService extends IConnectivityManager.Stub synchronized (mUidToNetworkRequestCount) { int networkRequests = mUidToNetworkRequestCount.get(mUid, 0) + 1; if (networkRequests >= MAX_NETWORK_REQUESTS_PER_UID) { - throw new IllegalArgumentException("Too many NetworkRequests filed"); + throw new ServiceSpecificException( + ConnectivityManager.Errors.TOO_MANY_REQUESTS); } mUidToNetworkRequestCount.put(mUid, networkRequests); } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 517327882b..3c0b8aa7c5 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -43,6 +43,7 @@ import android.net.ConnectivityManager; import android.net.ConnectivityManager.NetworkCallback; import android.net.ConnectivityManager.PacketKeepalive; import android.net.ConnectivityManager.PacketKeepaliveCallback; +import android.net.ConnectivityManager.TooManyRequestsException; import android.net.INetworkPolicyManager; import android.net.INetworkStatsService; import android.net.IpPrefix; @@ -2981,7 +2982,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { networkCallbacks.add(networkCallback); } fail("Registering " + MAX_REQUESTS + " NetworkRequests did not throw exception"); - } catch (IllegalArgumentException expected) {} + } catch (TooManyRequestsException expected) {} for (NetworkCallback networkCallback : networkCallbacks) { mCm.unregisterNetworkCallback(networkCallback); } @@ -2994,7 +2995,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { networkCallbacks.add(networkCallback); } fail("Registering " + MAX_REQUESTS + " NetworkCallbacks did not throw exception"); - } catch (IllegalArgumentException expected) {} + } catch (TooManyRequestsException expected) {} for (NetworkCallback networkCallback : networkCallbacks) { mCm.unregisterNetworkCallback(networkCallback); } @@ -3010,7 +3011,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { } fail("Registering " + MAX_REQUESTS + " PendingIntent NetworkRequests did not throw exception"); - } catch (IllegalArgumentException expected) {} + } catch (TooManyRequestsException expected) {} for (PendingIntent pendingIntent : pendingIntents) { mCm.unregisterNetworkCallback(pendingIntent); } @@ -3025,7 +3026,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { } fail("Registering " + MAX_REQUESTS + " PendingIntent NetworkCallbacks did not throw exception"); - } catch (IllegalArgumentException expected) {} + } catch (TooManyRequestsException expected) {} for (PendingIntent pendingIntent : pendingIntents) { mCm.unregisterNetworkCallback(pendingIntent); } From 7abd43f6fb09f7e0a1d9b0d3fdd001d6a691864f Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 9 May 2017 14:09:02 +0900 Subject: [PATCH 2/2] ConnectivityManager: unit test for argument validation Bug: 36701874, 37107940 Test: new test passes Change-Id: Ie762ce758b3d94052b7438a67fc55bef4690cbbb --- .../android/net/ConnectivityManagerTest.java | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/net/java/android/net/ConnectivityManagerTest.java b/tests/net/java/android/net/ConnectivityManagerTest.java index ceb0135727..cc792cc749 100644 --- a/tests/net/java/android/net/ConnectivityManagerTest.java +++ b/tests/net/java/android/net/ConnectivityManagerTest.java @@ -45,6 +45,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import android.app.PendingIntent; import android.net.ConnectivityManager; import android.net.NetworkCapabilities; import android.content.Context; @@ -66,8 +67,6 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; - - @RunWith(AndroidJUnit4.class) @SmallTest public class ConnectivityManagerTest { @@ -296,6 +295,43 @@ public class ConnectivityManagerTest { manager.requestNetwork(request, callback); } + @Test + public void testArgumentValidation() throws Exception { + ConnectivityManager manager = new ConnectivityManager(mCtx, mService); + + NetworkRequest request = mock(NetworkRequest.class); + NetworkCallback callback = mock(NetworkCallback.class); + Handler handler = mock(Handler.class); + NetworkCallback nullCallback = null; + PendingIntent nullIntent = null; + + mustFail(() -> { manager.requestNetwork(null, callback); }); + mustFail(() -> { manager.requestNetwork(request, nullCallback); }); + mustFail(() -> { manager.requestNetwork(request, callback, null); }); + mustFail(() -> { manager.requestNetwork(request, callback, -1); }); + mustFail(() -> { manager.requestNetwork(request, nullIntent); }); + + mustFail(() -> { manager.registerNetworkCallback(null, callback, handler); }); + mustFail(() -> { manager.registerNetworkCallback(request, null, handler); }); + mustFail(() -> { manager.registerNetworkCallback(request, callback, null); }); + mustFail(() -> { manager.registerNetworkCallback(request, nullIntent); }); + + mustFail(() -> { manager.registerDefaultNetworkCallback(null, handler); }); + mustFail(() -> { manager.registerDefaultNetworkCallback(callback, null); }); + + mustFail(() -> { manager.unregisterNetworkCallback(nullCallback); }); + mustFail(() -> { manager.unregisterNetworkCallback(nullIntent); }); + mustFail(() -> { manager.releaseNetworkRequest(nullIntent); }); + } + + static void mustFail(Runnable fn) { + try { + fn.run(); + fail(); + } catch (Exception expected) { + } + } + static Message makeMessage(NetworkRequest req, int messageType) { Bundle bundle = new Bundle(); bundle.putParcelable(NetworkRequest.class.getSimpleName(), req);