From 24de5525f25d9862ab727dee18f5b95fd7a93fcf Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 1 Feb 2021 15:57:44 +0900 Subject: [PATCH 1/2] Add test coverage for get*NetworkInfo on metered networks. Test: test-only change Change-Id: I58769bb768978d0acff1da6d32c2f6942c43508b --- .../server/ConnectivityServiceTest.java | 71 ++++++++++++++++++- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 8b536b2214..6ac707dbf2 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -380,6 +380,10 @@ public class ConnectivityServiceTest { private QosCallbackMockHelper mQosCallbackMockHelper; private QosCallbackTracker mQosCallbackTracker; + // State variables required to emulate NetworkPolicyManagerService behaviour. + private int mUidRules = RULE_NONE; + private boolean mRestrictBackground = false; + @Mock DeviceIdleInternal mDeviceIdleInternal; @Mock INetworkManagementService mNetworkManagementService; @Mock INetworkStatsService mStatsService; @@ -1277,12 +1281,45 @@ public class ConnectivityServiceTest { } } + private void updateUidNetworkingBlocked() { + // Changes the return value of the mock NetworkPolicyManager's isUidNetworkingBlocked method + // based on the current UID rules and restrict background setting. Note that the test never + // pretends to be a foreground app, so always declare no connectivity if background + // networking is not allowed. + switch (mUidRules) { + case RULE_REJECT_ALL: + when(mNetworkPolicyManager.isUidNetworkingBlocked(anyInt(), anyBoolean())) + .thenReturn(true); + break; + + case RULE_REJECT_METERED: + when(mNetworkPolicyManager.isUidNetworkingBlocked(anyInt(), eq(true))) + .thenReturn(true); + when(mNetworkPolicyManager.isUidNetworkingBlocked(anyInt(), eq(false))) + .thenReturn(mRestrictBackground); + break; + + case RULE_ALLOW_METERED: + case RULE_NONE: + when(mNetworkPolicyManager.isUidNetworkingBlocked(anyInt(), anyBoolean())) + .thenReturn(mRestrictBackground); + break; + + default: + fail("Unknown policy rule " + mUidRules); + } + } + private void setUidRulesChanged(int uidRules) throws RemoteException { - mPolicyListener.onUidRulesChanged(Process.myUid(), uidRules); + mUidRules = uidRules; + updateUidNetworkingBlocked(); + mPolicyListener.onUidRulesChanged(Process.myUid(), mUidRules); } private void setRestrictBackgroundChanged(boolean restrictBackground) throws RemoteException { - mPolicyListener.onRestrictBackgroundChanged(restrictBackground); + mRestrictBackground = restrictBackground; + updateUidNetworkingBlocked(); + mPolicyListener.onRestrictBackgroundChanged(mRestrictBackground); } private Nat464Xlat getNat464Xlat(NetworkAgentWrapper mna) { @@ -6839,9 +6876,15 @@ public class ConnectivityServiceTest { mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mCellNetworkAgent.connect(true); cellNetworkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); + assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); setUidRulesChanged(RULE_REJECT_ALL); cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent); + assertNull(mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); // ConnectivityService should cache it not to invoke the callback again. setUidRulesChanged(RULE_REJECT_METERED); @@ -6849,20 +6892,37 @@ public class ConnectivityServiceTest { setUidRulesChanged(RULE_NONE); cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent); + assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); setUidRulesChanged(RULE_REJECT_METERED); cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent); + assertNull(mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); // Restrict the network based on UID rule and NOT_METERED capability change. mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); cellNetworkCallback.expectCapabilitiesWith(NET_CAPABILITY_NOT_METERED, mCellNetworkAgent); cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent); + assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_METERED); cellNetworkCallback.expectCapabilitiesWithout(NET_CAPABILITY_NOT_METERED, mCellNetworkAgent); cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent); + assertEquals(null, mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + setUidRulesChanged(RULE_ALLOW_METERED); cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent); + assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); setUidRulesChanged(RULE_NONE); cellNetworkCallback.assertNoCallback(); @@ -6870,11 +6930,18 @@ public class ConnectivityServiceTest { // Restrict the network based on BackgroundRestricted. setRestrictBackgroundChanged(true); cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent); + assertEquals(null, mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED); + setRestrictBackgroundChanged(true); cellNetworkCallback.assertNoCallback(); setRestrictBackgroundChanged(false); cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent); cellNetworkCallback.assertNoCallback(); + assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); mCm.unregisterNetworkCallback(cellNetworkCallback); } From 63aaccb9fa95a50233043613ba651eef6767b9a4 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 15 Jan 2021 23:35:16 +0900 Subject: [PATCH 2/2] Remove getFilteredNetworkState and add @NonNull in NetworkState. getFilteredNetworkState is only used in two places, both of which use only small parts of the NetworkState that is returned to them. Remove the method and replace it with inline code in the two callers. NetworkState is a fairly expensive object, and this removes the need to perform lots of defensive copies of data that the caller does not need. Also remove the only call to the NetworkState constructor in ConnectivityService. That leaves only one caller to the NetworkState constructor, the one in NetworkAgentInfo. This constructor is called with defensive copies of NetworkInfo, LinkProperties, and NetworkCapabilities, so mark these three parameters as @NonNull. It's also called with a non-null Network, because NetworkAgentInfo is only constructed with non-null Networks, so mark the network parameter @NonNull as well. In order to make the arguments in the NetworkState constructor @NonNull, introduce a new constructor that sets everything to null and make NetworkState.EMPTY call it. Test: atest FrameworksNetTests Change-Id: Idcc9e32c53533b0cf61494517e62d4c184fa7610 --- core/java/android/net/NetworkState.java | 19 ++++- .../android/server/ConnectivityService.java | 84 ++++++++++--------- 2 files changed, 59 insertions(+), 44 deletions(-) diff --git a/core/java/android/net/NetworkState.java b/core/java/android/net/NetworkState.java index 713b688837..e1ef8b5ea5 100644 --- a/core/java/android/net/NetworkState.java +++ b/core/java/android/net/NetworkState.java @@ -16,6 +16,7 @@ package android.net; +import android.annotation.NonNull; import android.compat.annotation.UnsupportedAppUsage; import android.os.Build; import android.os.Parcel; @@ -30,7 +31,8 @@ import android.util.Slog; public class NetworkState implements Parcelable { private static final boolean VALIDATE_ROAMING_STATE = false; - public static final NetworkState EMPTY = new NetworkState(null, null, null, null, null, null); + // TODO: remove and make members @NonNull. + public static final NetworkState EMPTY = new NetworkState(); public final NetworkInfo networkInfo; public final LinkProperties linkProperties; @@ -40,9 +42,18 @@ public class NetworkState implements Parcelable { public final String subscriberId; public final String networkId; - public NetworkState(NetworkInfo networkInfo, LinkProperties linkProperties, - NetworkCapabilities networkCapabilities, Network network, String subscriberId, - String networkId) { + private NetworkState() { + networkInfo = null; + linkProperties = null; + networkCapabilities = null; + network = null; + subscriberId = null; + networkId = null; + } + + public NetworkState(@NonNull NetworkInfo networkInfo, @NonNull LinkProperties linkProperties, + @NonNull NetworkCapabilities networkCapabilities, @NonNull Network network, + String subscriberId, String networkId) { this.networkInfo = networkInfo; this.linkProperties = linkProperties; this.networkCapabilities = networkCapabilities; diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 69e792e623..5d9ed08110 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1329,31 +1329,6 @@ public class ConnectivityService extends IConnectivityManager.Stub return mNextNetworkRequestId++; } - private NetworkState getFilteredNetworkState(int networkType, int uid) { - if (mLegacyTypeTracker.isTypeSupported(networkType)) { - final NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType); - final NetworkState state; - if (nai != null) { - state = nai.getNetworkState(); - state.networkInfo.setType(networkType); - } else { - final NetworkInfo info = new NetworkInfo(networkType, 0, - getNetworkTypeName(networkType), ""); - info.setDetailedState(NetworkInfo.DetailedState.DISCONNECTED, null, null); - info.setIsAvailable(true); - final NetworkCapabilities capabilities = new NetworkCapabilities(); - capabilities.setCapability(NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING, - !info.isRoaming()); - state = new NetworkState(info, new LinkProperties(), capabilities, - null, null, null); - } - filterNetworkStateForUid(state, uid, false); - return state; - } else { - return NetworkState.EMPTY; - } - } - @VisibleForTesting protected NetworkAgentInfo getNetworkAgentInfoForNetwork(Network network) { if (network == null) { @@ -1464,6 +1439,18 @@ public class ConnectivityService extends IConnectivityManager.Stub "%s %d(%d) on netId %d", action, nri.mUid, requestId, net.getNetId())); } + private void filterNetworkInfo(@NonNull NetworkInfo networkInfo, + @NonNull NetworkCapabilities nc, int uid, boolean ignoreBlocked) { + if (isNetworkWithCapabilitiesBlocked(nc, uid, ignoreBlocked)) { + networkInfo.setDetailedState(DetailedState.BLOCKED, null, null); + } + synchronized (mVpns) { + if (mLockdownTracker != null) { + mLockdownTracker.augmentNetworkInfo(networkInfo); + } + } + } + /** * Apply any relevant filters to {@link NetworkState} for the given UID. For * example, this may mark the network as {@link DetailedState#BLOCKED} based @@ -1471,16 +1458,7 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private void filterNetworkStateForUid(NetworkState state, int uid, boolean ignoreBlocked) { if (state == null || state.networkInfo == null || state.linkProperties == null) return; - - if (isNetworkWithCapabilitiesBlocked(state.networkCapabilities, uid, - ignoreBlocked)) { - state.networkInfo.setDetailedState(DetailedState.BLOCKED, null, null); - } - synchronized (mVpns) { - if (mLockdownTracker != null) { - mLockdownTracker.augmentNetworkInfo(state.networkInfo); - } - } + filterNetworkInfo(state.networkInfo, state.networkCapabilities, uid, ignoreBlocked); } /** @@ -1545,6 +1523,27 @@ public class ConnectivityService extends IConnectivityManager.Stub return state.networkInfo; } + private NetworkInfo getFilteredNetworkInfo(int networkType, int uid) { + if (!mLegacyTypeTracker.isTypeSupported(networkType)) { + return null; + } + final NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType); + final NetworkInfo info; + final NetworkCapabilities nc; + if (nai != null) { + info = new NetworkInfo(nai.networkInfo); + info.setType(networkType); + nc = nai.networkCapabilities; + } else { + info = new NetworkInfo(networkType, 0, getNetworkTypeName(networkType), ""); + info.setDetailedState(NetworkInfo.DetailedState.DISCONNECTED, null, null); + info.setIsAvailable(true); + nc = new NetworkCapabilities(); + } + filterNetworkInfo(info, nc, uid, false); + return info; + } + @Override public NetworkInfo getNetworkInfo(int networkType) { enforceAccessPermission(); @@ -1559,8 +1558,7 @@ public class ConnectivityService extends IConnectivityManager.Stub return state.networkInfo; } } - final NetworkState state = getFilteredNetworkState(networkType, uid); - return state.networkInfo; + return getFilteredNetworkInfo(networkType, uid); } @Override @@ -1593,10 +1591,16 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public Network getNetworkForType(int networkType) { enforceAccessPermission(); + if (!mLegacyTypeTracker.isTypeSupported(networkType)) { + return null; + } + final NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType); + if (nai == null) { + return null; + } final int uid = mDeps.getCallingUid(); - NetworkState state = getFilteredNetworkState(networkType, uid); - if (!isNetworkWithCapabilitiesBlocked(state.networkCapabilities, uid, false)) { - return state.network; + if (!isNetworkWithCapabilitiesBlocked(nai.networkCapabilities, uid, false)) { + return nai.network; } return null; }