From 52a6d547b7bd207b0f9c8479fdfd37eb39dfb211 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 18 May 2018 23:48:49 +0900 Subject: [PATCH] Fix setCapabilities. P introduced setSSID, UIDs and unwanted capabilities. None of these exhibit commutative behavior through combineCapabilities because their semantics don't allow it. Therefore NetworkRequest.setCapabilities() is badly broken around any of these. Look at the comments in the new tests to realize the extent of the damage. Clean cherry-pick of ag/4083952 Bug: 79748782 Test: new tests written, old tests pass Change-Id: Iafe074126132a82af37f4bf056c4a7b8d56bdc83 Merged-In: Ia5bebf8a233775367bbf1b788870528934ecbcfb Merged-In: I13d7782a6c0c7b1f94137995bbb0d257a58d89c1 --- .../java/android/net/NetworkCapabilities.java | 28 ++++++--- core/java/android/net/NetworkRequest.java | 3 +- .../android/net/NetworkCapabilitiesTest.java | 61 ++++++++++++++++++- 3 files changed, 78 insertions(+), 14 deletions(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 3f0f334772..0f2bfba86c 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -61,16 +61,7 @@ public final class NetworkCapabilities implements Parcelable { public NetworkCapabilities(NetworkCapabilities nc) { if (nc != null) { - mNetworkCapabilities = nc.mNetworkCapabilities; - mTransportTypes = nc.mTransportTypes; - mLinkUpBandwidthKbps = nc.mLinkUpBandwidthKbps; - mLinkDownBandwidthKbps = nc.mLinkDownBandwidthKbps; - mNetworkSpecifier = nc.mNetworkSpecifier; - mSignalStrength = nc.mSignalStrength; - mUids = nc.mUids; - mEstablishingVpnAppUid = nc.mEstablishingVpnAppUid; - mUnwantedNetworkCapabilities = nc.mUnwantedNetworkCapabilities; - mSSID = nc.mSSID; + set(nc); } } @@ -89,6 +80,23 @@ public final class NetworkCapabilities implements Parcelable { mSSID = null; } + /** + * Set all contents of this object to the contents of a NetworkCapabilities. + * @hide + */ + public void set(NetworkCapabilities nc) { + mNetworkCapabilities = nc.mNetworkCapabilities; + mTransportTypes = nc.mTransportTypes; + mLinkUpBandwidthKbps = nc.mLinkUpBandwidthKbps; + mLinkDownBandwidthKbps = nc.mLinkDownBandwidthKbps; + mNetworkSpecifier = nc.mNetworkSpecifier; + mSignalStrength = nc.mSignalStrength; + setUids(nc.mUids); // Will make the defensive copy + mEstablishingVpnAppUid = nc.mEstablishingVpnAppUid; + mUnwantedNetworkCapabilities = nc.mUnwantedNetworkCapabilities; + mSSID = nc.mSSID; + } + /** * Represents the network's capabilities. If any are specified they will be satisfied * by any Network that matches all of them. diff --git a/core/java/android/net/NetworkRequest.java b/core/java/android/net/NetworkRequest.java index b0b58cd524..f3669fb3e2 100644 --- a/core/java/android/net/NetworkRequest.java +++ b/core/java/android/net/NetworkRequest.java @@ -197,8 +197,7 @@ public class NetworkRequest implements Parcelable { * @hide */ public Builder setCapabilities(NetworkCapabilities nc) { - mNetworkCapabilities.clearAll(); - mNetworkCapabilities.combineCapabilities(nc); + mNetworkCapabilities.set(nc); return this; } diff --git a/tests/net/java/android/net/NetworkCapabilitiesTest.java b/tests/net/java/android/net/NetworkCapabilitiesTest.java index c17f31e701..2f4d69ec78 100644 --- a/tests/net/java/android/net/NetworkCapabilitiesTest.java +++ b/tests/net/java/android/net/NetworkCapabilitiesTest.java @@ -57,6 +57,7 @@ import java.util.Set; @SmallTest public class NetworkCapabilitiesTest { private static final String TEST_SSID = "TEST_SSID"; + private static final String DIFFERENT_TEST_SSID = "DIFFERENT_TEST_SSID"; @Test public void testMaybeMarkCapabilitiesRestricted() { @@ -382,6 +383,12 @@ public class NetworkCapabilitiesTest { assertFalse(nc1.satisfiedByNetworkCapabilities(nc2)); } + private ArraySet uidRange(int from, int to) { + final ArraySet range = new ArraySet<>(1); + range.add(new UidRange(from, to)); + return range; + } + @Test public void testCombineCapabilities() { NetworkCapabilities nc1 = new NetworkCapabilities(); @@ -408,14 +415,30 @@ public class NetworkCapabilitiesTest { nc2.combineCapabilities(nc1); assertTrue(TEST_SSID.equals(nc2.getSSID())); - // Because they now have the same SSID, the folllowing call should not throw + // Because they now have the same SSID, the following call should not throw nc2.combineCapabilities(nc1); - nc1.setSSID("different " + TEST_SSID); + nc1.setSSID(DIFFERENT_TEST_SSID); try { nc2.combineCapabilities(nc1); fail("Expected IllegalStateException: can't combine different SSIDs"); } catch (IllegalStateException expected) {} + nc1.setSSID(TEST_SSID); + + nc1.setUids(uidRange(10, 13)); + assertNotEquals(nc1, nc2); + nc2.combineCapabilities(nc1); // Everything + 10~13 is still everything. + assertNotEquals(nc1, nc2); + nc1.combineCapabilities(nc2); // 10~13 + everything is everything. + assertEquals(nc1, nc2); + nc1.setUids(uidRange(10, 13)); + nc2.setUids(uidRange(20, 23)); + assertNotEquals(nc1, nc2); + nc1.combineCapabilities(nc2); + assertTrue(nc1.appliesToUid(12)); + assertFalse(nc2.appliesToUid(12)); + assertTrue(nc1.appliesToUid(22)); + assertTrue(nc2.appliesToUid(22)); } @Test @@ -454,4 +477,38 @@ public class NetworkCapabilitiesTest { p.setDataPosition(0); assertEquals(NetworkCapabilities.CREATOR.createFromParcel(p), netCap); } + + @Test + public void testSet() { + NetworkCapabilities nc1 = new NetworkCapabilities(); + NetworkCapabilities nc2 = new NetworkCapabilities(); + + nc1.addUnwantedCapability(NET_CAPABILITY_CAPTIVE_PORTAL); + nc1.addCapability(NET_CAPABILITY_NOT_ROAMING); + assertNotEquals(nc1, nc2); + nc2.set(nc1); + assertEquals(nc1, nc2); + assertTrue(nc2.hasCapability(NET_CAPABILITY_NOT_ROAMING)); + assertTrue(nc2.hasUnwantedCapability(NET_CAPABILITY_CAPTIVE_PORTAL)); + + // This will effectively move NOT_ROAMING capability from required to unwanted for nc1. + nc1.addUnwantedCapability(NET_CAPABILITY_NOT_ROAMING); + nc1.setSSID(TEST_SSID); + nc2.set(nc1); + assertEquals(nc1, nc2); + // Contrary to combineCapabilities, set() will have removed the NOT_ROAMING capability + // from nc2. + assertFalse(nc2.hasCapability(NET_CAPABILITY_NOT_ROAMING)); + assertTrue(nc2.hasUnwantedCapability(NET_CAPABILITY_NOT_ROAMING)); + assertTrue(TEST_SSID.equals(nc2.getSSID())); + + nc1.setSSID(DIFFERENT_TEST_SSID); + nc2.set(nc1); + assertEquals(nc1, nc2); + assertTrue(DIFFERENT_TEST_SSID.equals(nc2.getSSID())); + + nc1.setUids(uidRange(10, 13)); + nc2.set(nc1); // Overwrites, as opposed to combineCapabilities + assertEquals(nc1, nc2); + } }