From 0b7642a8203cc25209316436e449df06faf7c5ff Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 20 Jun 2017 14:07:59 +0900 Subject: [PATCH 1/2] ConnectivityManager: allow usage of TYPE_NONE This patch allows to use TYPE_NONE for the legacy network type variable of NetworkInfo. This usage is "safe" with respect to legacy APIs using network types as most of them already returns null or do nothing for TYPE_NONE. Of the existing APIs in ConnectivityManager that accept a network type argument, those which were already returning null or doing nothing for TYPE_NONE are: getNetworkInfo(int) getNetworkForType(int) stopUsingNetworkFeature(int, String) networkCapabilitiesForType(int) requestRouteToHostAddress(int, InetAddress) reportInetCondition(int, int) isNetworkSupported(int) getLinkProperties(int) Only setProvisioningNotificationVisible needs an additional guard against TYPE_NONE. Bug: 30088447 Bug: 62844794 Test: runtest frameworks-net Change-Id: I4455f2726d06406047086368628c1f253d854d8d --- .../java/android/net/ConnectivityManager.java | 6 +- .../java/android/net/NetworkCapabilities.java | 26 +-- core/java/android/net/NetworkInfo.java | 3 +- .../android/server/ConnectivityService.java | 3 + .../server/ConnectivityServiceTest.java | 156 +++++++++++++++--- 5 files changed, 156 insertions(+), 38 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index 02f0f184b9..55127a8d35 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -579,6 +579,8 @@ public class ConnectivityManager { /** {@hide} */ public static final int MAX_NETWORK_TYPE = TYPE_VPN; + private static final int MIN_NETWORK_TYPE = TYPE_MOBILE; + /** * If you want to set the default network preference,you can directly * change the networkAttributes array in framework's config.xml. @@ -636,7 +638,7 @@ public class ConnectivityManager { * validate a network type. */ public static boolean isNetworkTypeValid(int networkType) { - return networkType >= 0 && networkType <= MAX_NETWORK_TYPE; + return MIN_NETWORK_TYPE <= networkType && networkType <= MAX_NETWORK_TYPE; } /** @@ -649,6 +651,8 @@ public class ConnectivityManager { */ public static String getNetworkTypeName(int type) { switch (type) { + case TYPE_NONE: + return "NONE"; case TYPE_MOBILE: return "MOBILE"; case TYPE_WIFI: diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 2dd7f757ae..76646b8939 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -21,6 +21,7 @@ import android.os.Parcelable; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.BitUtils; +import com.android.internal.util.Preconditions; import java.util.Objects; @@ -429,6 +430,11 @@ public final class NetworkCapabilities implements Parcelable { /** @hide */ public static final int MAX_TRANSPORT = TRANSPORT_LOWPAN; + /** @hide */ + public static boolean isValidTransport(int transportType) { + return (MIN_TRANSPORT <= transportType) && (transportType <= MAX_TRANSPORT); + } + private static final String[] TRANSPORT_NAMES = { "CELLULAR", "WIFI", @@ -453,9 +459,7 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public NetworkCapabilities addTransportType(int transportType) { - if (transportType < MIN_TRANSPORT || transportType > MAX_TRANSPORT) { - throw new IllegalArgumentException("TransportType out of range"); - } + checkValidTransportType(transportType); mTransportTypes |= 1 << transportType; setNetworkSpecifier(mNetworkSpecifier); // used for exception checking return this; @@ -469,9 +473,7 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public NetworkCapabilities removeTransportType(int transportType) { - if (transportType < MIN_TRANSPORT || transportType > MAX_TRANSPORT) { - throw new IllegalArgumentException("TransportType out of range"); - } + checkValidTransportType(transportType); mTransportTypes &= ~(1 << transportType); setNetworkSpecifier(mNetworkSpecifier); // used for exception checking return this; @@ -495,10 +497,7 @@ public final class NetworkCapabilities implements Parcelable { * @return {@code true} if set on this instance. */ public boolean hasTransport(int transportType) { - if (transportType < MIN_TRANSPORT || transportType > MAX_TRANSPORT) { - return false; - } - return ((mTransportTypes & (1 << transportType)) != 0); + return isValidTransport(transportType) && ((mTransportTypes & (1 << transportType)) != 0); } private void combineTransportTypes(NetworkCapabilities nc) { @@ -906,9 +905,14 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public static String transportNameOf(int transport) { - if (transport < 0 || TRANSPORT_NAMES.length <= transport) { + if (!isValidTransport(transport)) { return "UNKNOWN"; } return TRANSPORT_NAMES[transport]; } + + private static void checkValidTransportType(int transport) { + Preconditions.checkArgument( + isValidTransport(transport), "Invalid TransportType " + transport); + } } diff --git a/core/java/android/net/NetworkInfo.java b/core/java/android/net/NetworkInfo.java index 42f5feb583..84c32bec8e 100644 --- a/core/java/android/net/NetworkInfo.java +++ b/core/java/android/net/NetworkInfo.java @@ -127,7 +127,8 @@ public class NetworkInfo implements Parcelable { * @hide */ public NetworkInfo(int type, int subtype, String typeName, String subtypeName) { - if (!ConnectivityManager.isNetworkTypeValid(type)) { + if (!ConnectivityManager.isNetworkTypeValid(type) + && type != ConnectivityManager.TYPE_NONE) { throw new IllegalArgumentException("Invalid network type: " + type); } mNetworkType = type; diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 8ccedc0b54..61122676fe 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3911,6 +3911,9 @@ public class ConnectivityService extends IConnectivityManager.Stub public void setProvisioningNotificationVisible(boolean visible, int networkType, String action) { enforceConnectivityInternalPermission(); + if (!ConnectivityManager.isNetworkTypeValid(networkType)) { + return; + } final long ident = Binder.clearCallingIdentity(); try { // Concatenate the range of types onto the range of NetIDs. diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 0263c57879..953bb00f6b 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -19,6 +19,7 @@ package com.android.server; import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.TYPE_ETHERNET; import static android.net.ConnectivityManager.TYPE_MOBILE; +import static android.net.ConnectivityManager.TYPE_NONE; import static android.net.ConnectivityManager.TYPE_WIFI; import static android.net.ConnectivityManager.getNetworkTypeName; import static android.net.NetworkCapabilities.*; @@ -111,6 +112,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.BooleanSupplier; +import java.util.function.Predicate; /** * Tests for {@link ConnectivityService}. @@ -326,6 +328,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { case TRANSPORT_CELLULAR: mScore = 50; break; + case TRANSPORT_LOWPAN: + mScore = 20; + break; default: throw new UnsupportedOperationException("unimplemented network type"); } @@ -407,6 +412,15 @@ public class ConnectivityServiceTest extends AndroidTestCase { * @param validated Indicate if network should pretend to be validated. */ public void connect(boolean validated) { + connect(validated, true); + } + + /** + * Transition this NetworkAgent to CONNECTED state. + * @param validated Indicate if network should pretend to be validated. + * @param hasInternet Indicate if network should pretend to have NET_CAPABILITY_INTERNET. + */ + public void connect(boolean validated, boolean hasInternet) { assertEquals("MockNetworkAgents can only be connected once", mNetworkInfo.getDetailedState(), DetailedState.IDLE); assertFalse(mNetworkCapabilities.hasCapability(NET_CAPABILITY_INTERNET)); @@ -429,7 +443,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { }; mCm.registerNetworkCallback(request, callback); } - addCapability(NET_CAPABILITY_INTERNET); + if (hasInternet) { + addCapability(NET_CAPABILITY_INTERNET); + } connectWithoutInternet(); @@ -783,7 +799,10 @@ public class ConnectivityServiceTest extends AndroidTestCase { * Fails if TIMEOUT_MS goes by before {@code conditionVariable} opens. */ static private void waitFor(ConditionVariable conditionVariable) { - assertTrue(conditionVariable.block(TIMEOUT_MS)); + if (conditionVariable.block(TIMEOUT_MS)) { + return; + } + fail("ConditionVariable was blocked for more than " + TIMEOUT_MS + "ms"); } @Override @@ -838,7 +857,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { case TRANSPORT_CELLULAR: return TYPE_MOBILE; default: - throw new IllegalStateException("Unknown transport " + transport); + return TYPE_NONE; } } @@ -849,6 +868,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { // Test getActiveNetwork() assertNotNull(mCm.getActiveNetwork()); assertEquals(mCm.getActiveNetwork(), mCm.getActiveNetworkForUid(Process.myUid())); + if (!NetworkCapabilities.isValidTransport(transport)) { + throw new IllegalStateException("Unknown transport " + transport); + } switch (transport) { case TRANSPORT_WIFI: assertEquals(mCm.getActiveNetwork(), mWiFiNetworkAgent.getNetwork()); @@ -857,7 +879,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { assertEquals(mCm.getActiveNetwork(), mCellNetworkAgent.getNetwork()); break; default: - throw new IllegalStateException("Unknown transport" + transport); + break; } // Test getNetworkInfo(Network) assertNotNull(mCm.getNetworkInfo(mCm.getActiveNetwork())); @@ -1277,7 +1299,26 @@ public class ConnectivityServiceTest extends AndroidTestCase { return expectCallback(state, agent, TIMEOUT_MS); } - void expectAvailableCallbacks(MockNetworkAgent agent, boolean expectSuspended, int timeoutMs) { + CallbackInfo expectCallbackLike(Predicate fn) { + return expectCallbackLike(fn, TIMEOUT_MS); + } + + CallbackInfo expectCallbackLike(Predicate fn, int timeoutMs) { + int timeLeft = timeoutMs; + while (timeLeft > 0) { + long start = SystemClock.elapsedRealtime(); + CallbackInfo info = nextCallback(timeLeft); + if (fn.test(info)) { + return info; + } + timeLeft -= (SystemClock.elapsedRealtime() - start); + } + fail("Did not receive expected callback after " + timeoutMs + "ms"); + return null; + } + + void expectAvailableCallbacks( + MockNetworkAgent agent, boolean expectSuspended, int timeoutMs) { expectCallback(CallbackState.AVAILABLE, agent, timeoutMs); if (expectSuspended) { expectCallback(CallbackState.SUSPENDED, agent, timeoutMs); @@ -1834,26 +1875,18 @@ public class ConnectivityServiceTest extends AndroidTestCase { @SmallTest public void testNoMutableNetworkRequests() throws Exception { PendingIntent pendingIntent = PendingIntent.getBroadcast(mContext, 0, new Intent("a"), 0); - NetworkRequest.Builder builder = new NetworkRequest.Builder(); - builder.addCapability(NET_CAPABILITY_VALIDATED); - try { - mCm.requestNetwork(builder.build(), new NetworkCallback()); - fail(); - } catch (IllegalArgumentException expected) {} - try { - mCm.requestNetwork(builder.build(), pendingIntent); - fail(); - } catch (IllegalArgumentException expected) {} - builder = new NetworkRequest.Builder(); - builder.addCapability(NET_CAPABILITY_CAPTIVE_PORTAL); - try { - mCm.requestNetwork(builder.build(), new NetworkCallback()); - fail(); - } catch (IllegalArgumentException expected) {} - try { - mCm.requestNetwork(builder.build(), pendingIntent); - fail(); - } catch (IllegalArgumentException expected) {} + NetworkRequest request1 = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_VALIDATED) + .build(); + NetworkRequest request2 = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_CAPTIVE_PORTAL) + .build(); + + Class expected = IllegalArgumentException.class; + assertException(() -> { mCm.requestNetwork(request1, new NetworkCallback()); }, expected); + assertException(() -> { mCm.requestNetwork(request1, pendingIntent); }, expected); + assertException(() -> { mCm.requestNetwork(request2, new NetworkCallback()); }, expected); + assertException(() -> { mCm.requestNetwork(request2, pendingIntent); }, expected); } @SmallTest @@ -3255,6 +3288,79 @@ public class ConnectivityServiceTest extends AndroidTestCase { } } + @SmallTest + public void testNetworkInfoOfTypeNone() { + ConditionVariable broadcastCV = waitForConnectivityBroadcasts(1); + + verifyNoNetwork(); + MockNetworkAgent lowpanNetwork = new MockNetworkAgent(TRANSPORT_LOWPAN); + assertNull(mCm.getActiveNetworkInfo()); + + Network[] allNetworks = mCm.getAllNetworks(); + assertEquals(1, allNetworks.length); + Network network = allNetworks[0]; + NetworkCapabilities capabilities = mCm.getNetworkCapabilities(network); + assertTrue(capabilities.hasTransport(TRANSPORT_LOWPAN)); + + final NetworkRequest request = + new NetworkRequest.Builder().addTransportType(TRANSPORT_LOWPAN).build(); + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerNetworkCallback(request, callback); + + // Bring up lowpan. + lowpanNetwork.connect(false, false); + callback.expectAvailableCallbacks(lowpanNetwork); + + assertNull(mCm.getActiveNetworkInfo()); + assertNull(mCm.getActiveNetwork()); + // TODO: getAllNetworkInfo is dirty and returns a non-empty array rght from the start + // of this test. Fix it and uncomment the assert below. + //assertEquals(0, mCm.getAllNetworkInfo().length); + + // Disconnect lowpan. + lowpanNetwork.disconnect(); + callback.expectCallback(CallbackState.LOST, lowpanNetwork); + mCm.unregisterNetworkCallback(callback); + + verifyNoNetwork(); + if (broadcastCV.block(10)) { + fail("expected no broadcast, but got CONNECTIVITY_ACTION broadcast"); + } + } + + @SmallTest + public void testDeprecatedAndUnsupportedOperations() throws Exception { + final int TYPE_NONE = ConnectivityManager.TYPE_NONE; + assertNull(mCm.getNetworkInfo(TYPE_NONE)); + assertNull(mCm.getNetworkForType(TYPE_NONE)); + assertNull(mCm.getLinkProperties(TYPE_NONE)); + assertFalse(mCm.isNetworkSupported(TYPE_NONE)); + + assertException(() -> { mCm.networkCapabilitiesForType(TYPE_NONE); }, + IllegalArgumentException.class); + + Class unsupported = UnsupportedOperationException.class; + assertException(() -> { mCm.startUsingNetworkFeature(TYPE_WIFI, ""); }, unsupported); + assertException(() -> { mCm.stopUsingNetworkFeature(TYPE_WIFI, ""); }, unsupported); + // TODO: let test context have configuration application target sdk version + // and test that pre-M requesting for TYPE_NONE sends back APN_REQUEST_FAILED + assertException(() -> { mCm.startUsingNetworkFeature(TYPE_NONE, ""); }, unsupported); + assertException(() -> { mCm.stopUsingNetworkFeature(TYPE_NONE, ""); }, unsupported); + assertException(() -> { mCm.requestRouteToHostAddress(TYPE_NONE, null); }, unsupported); + } + + private static void assertException(Runnable block, Class expected) { + try { + block.run(); + fail("Expected exception of type " + expected); + } catch (Exception got) { + if (!got.getClass().equals(expected)) { + fail("Expected exception of type " + expected + " but got " + got); + } + return; + } + } + /* test utilities */ // TODO: eliminate all usages of sleepFor and replace by proper timeouts/waitForIdle. static private void sleepFor(int ms) { From a0b7f12eb7c8c2720feaa122419645b7b25572e3 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Mon, 26 Jun 2017 10:06:49 +0900 Subject: [PATCH 2/2] ConnectivityServiceTest: more informative assert failures Bug: 62918393 Test: runtest frameworks-net Change-Id: I90c211dc6d6262475924ecabc2863c47aec5a0b9 --- .../server/ConnectivityServiceTest.java | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 953bb00f6b..dda2601682 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -897,7 +897,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { assertNull(mCm.getActiveNetwork()); assertNull(mCm.getActiveNetworkForUid(Process.myUid())); // Test getAllNetworks() - assertEquals(0, mCm.getAllNetworks().length); + assertEmpty(mCm.getAllNetworks()); } /** @@ -938,7 +938,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { mCellNetworkAgent.connect(true); waitFor(cv); verifyActiveNetwork(TRANSPORT_CELLULAR); - assertEquals(2, mCm.getAllNetworks().length); + assertLength(2, mCm.getAllNetworks()); assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) || mCm.getAllNetworks()[1].equals(mCm.getActiveNetwork())); assertTrue(mCm.getAllNetworks()[0].equals(mWiFiNetworkAgent.getNetwork()) || @@ -948,7 +948,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { mWiFiNetworkAgent.connect(true); waitFor(cv); verifyActiveNetwork(TRANSPORT_WIFI); - assertEquals(2, mCm.getAllNetworks().length); + assertLength(2, mCm.getAllNetworks()); assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) || mCm.getAllNetworks()[1].equals(mCm.getActiveNetwork())); assertTrue(mCm.getAllNetworks()[0].equals(mCellNetworkAgent.getNetwork()) || @@ -956,9 +956,9 @@ public class ConnectivityServiceTest extends AndroidTestCase { // Test cellular linger timeout. waitFor(mCellNetworkAgent.getDisconnectedCV()); waitForIdle(); - assertEquals(1, mCm.getAllNetworks().length); + assertLength(1, mCm.getAllNetworks()); verifyActiveNetwork(TRANSPORT_WIFI); - assertEquals(1, mCm.getAllNetworks().length); + assertLength(1, mCm.getAllNetworks()); assertEquals(mCm.getAllNetworks()[0], mCm.getActiveNetwork()); // Test WiFi disconnect. cv = waitForConnectivityBroadcasts(1); @@ -1898,7 +1898,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { mCellNetworkAgent.connectWithoutInternet(); waitFor(cv); waitForIdle(); - assertEquals(0, mCm.getAllNetworks().length); + assertEmpty(mCm.getAllNetworks()); verifyNoNetwork(); // Test bringing up validated WiFi. @@ -2572,7 +2572,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { assertTrue(testFactory.getMyStartRequested()); // Bring up cell data and check that the factory stops looking. - assertEquals(1, mCm.getAllNetworks().length); + assertLength(1, mCm.getAllNetworks()); mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); testFactory.expectAddRequests(2); // Because the cell request changes score twice. mCellNetworkAgent.connect(true); @@ -2583,7 +2583,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { // Check that cell data stays up. waitForIdle(); verifyActiveNetwork(TRANSPORT_WIFI); - assertEquals(2, mCm.getAllNetworks().length); + assertLength(2, mCm.getAllNetworks()); // Turn off mobile data always on and expect the request to disappear... testFactory.expectRemoveRequests(1); @@ -2592,7 +2592,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { // ... and cell data to be torn down. cellNetworkCallback.expectCallback(CallbackState.LOST, mCellNetworkAgent); - assertEquals(1, mCm.getAllNetworks().length); + assertLength(1, mCm.getAllNetworks()); testFactory.unregister(); mCm.unregisterNetworkCallback(cellNetworkCallback); @@ -3297,7 +3297,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { assertNull(mCm.getActiveNetworkInfo()); Network[] allNetworks = mCm.getAllNetworks(); - assertEquals(1, allNetworks.length); + assertLength(1, allNetworks); Network network = allNetworks[0]; NetworkCapabilities capabilities = mCm.getNetworkCapabilities(network); assertTrue(capabilities.hasTransport(TRANSPORT_LOWPAN)); @@ -3315,7 +3315,7 @@ public class ConnectivityServiceTest extends AndroidTestCase { assertNull(mCm.getActiveNetwork()); // TODO: getAllNetworkInfo is dirty and returns a non-empty array rght from the start // of this test. Fix it and uncomment the assert below. - //assertEquals(0, mCm.getAllNetworkInfo().length); + //assertEmpty(mCm.getAllNetworkInfo()); // Disconnect lowpan. lowpanNetwork.disconnect(); @@ -3361,6 +3361,17 @@ public class ConnectivityServiceTest extends AndroidTestCase { } } + private static void assertEmpty(T[] ts) { + int length = ts.length; + assertEquals("expected empty array, but length was " + length, 0, length); + } + + private static void assertLength(int expected, T[] got) { + int length = got.length; + assertEquals(String.format("expected array of length %s, but length was %s for %s", + expected, length, Arrays.toString(got)), expected, length); + } + /* test utilities */ // TODO: eliminate all usages of sleepFor and replace by proper timeouts/waitForIdle. static private void sleepFor(int ms) {