From 998a02f226a75c9e68e34455bc53f09cfbce5b7e Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 2 Mar 2021 23:28:49 +0900 Subject: [PATCH 1/2] Isolate an ad-hoc legacy API codepath. The legacy API getNetworkInfo(int type) is expected to return NetworkInfo objects for all network types supported by the device even when those network types are not connected. This requires a lot of fabrication because all the data structures in ConnectivityService store information about connected networks, not networks that don't exist. Worse, the current behaviour is to return BLOCKED instead of DISCONNECTED if background data is restricted. This obviously makes no sense, because a disconnected network cannot be blocked, and because if that network type did connect and was unmetered (e.g., Wi-Fi), it would no longer be BLOCKED. This complicates the code, forcing several methods to deal with a special case of null NetworkCapabilities, no NetworkAgentInfo, etc. Fix this by isolating this outlandish behaviour to its own method. This allows the main codepaths not to have to support this unusual and not very useful edge case. Bug: 174123988 Test: pure refactoring, passes existing tests Change-Id: Ia52b24c59024c8f6e63e584b864e0225cb572090 --- .../android/server/ConnectivityService.java | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 542d527177..ebd483c25e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1551,24 +1551,33 @@ public class ConnectivityService extends IConnectivityManager.Stub return state.networkInfo; } + /** Returns a NetworkInfo object for a network that doesn't exist. */ + private NetworkInfo makeFakeNetworkInfo(int networkType, int uid) { + final NetworkInfo info = new NetworkInfo(networkType, 0 /* subtype */, + getNetworkTypeName(networkType), "" /* subtypeName */); + info.setIsAvailable(true); + // For compatibility with legacy code, return BLOCKED instead of DISCONNECTED when + // background data is restricted. + final NetworkCapabilities nc = new NetworkCapabilities(); // Metered. + final DetailedState state = isNetworkWithCapabilitiesBlocked(nc, uid, false) + ? DetailedState.BLOCKED + : DetailedState.DISCONNECTED; + info.setDetailedState(getLegacyLockdownState(state), + "" /* reason */, null /* extraInfo */); + return info; + } + 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(); + if (nai == null) { + return makeFakeNetworkInfo(networkType, uid); } - filterNetworkInfo(info, nc, uid, false); + final NetworkInfo info = new NetworkInfo(nai.networkInfo); + info.setType(networkType); + filterNetworkInfo(info, nai.networkCapabilities, uid, false); return info; } From cb1d6a17bc1c54cd0cacbcfff572e3335ded6653 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 19 Jan 2021 01:33:05 +0900 Subject: [PATCH 2/2] Stop using NetworkState internally in ConnectivityService. NetworkState is used by many methods that take or return legacy network types. It is used because it contains most of the state related to a network. This code is not easy to follow and is more expensive than it needs to be: most of the methods that construct a NetworkState only really need one of its members (e.g., LinkProperties, or NetworkInfo), but constructing a NetworkState requires making defensive copies of all its other members as well. - Instead of using NetworkState, use NetworkAgentInfo, which already contains all the same data. - Replace calls to getUnfilteredActiveNetworkState with calls to getNetworkAgentInfoForUid. When getUnfilteredActiveNetworkState returned NetworkState.EMPTY, return a null nai, which causes any caller to see return null LinkProperties/NetworkInfo/etc. - Rename filterNetworkStateForUid to getFilteredNetworkInfo, because that's the only thing it actually filters. - Rename getFilteredNetworkInfo to getFilteredNetworkInfoForType to avoid having two methods with the same name perform two different operations. That method was only introduced recently in aosp/1552503, so it's probably fine to rename it. Bug: 174123988 Test: passes existing ConnectivityServiceTest Change-Id: Idfb5e149967266a442b268de6f13a521884dbb8f --- .../android/server/ConnectivityService.java | 98 +++++++++---------- 1 file changed, 46 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index ebd483c25e..93fab6e459 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1403,7 +1403,7 @@ public class ConnectivityService extends IConnectivityManager.Stub return null; } - private NetworkState getUnfilteredActiveNetworkState(int uid) { + private NetworkAgentInfo getNetworkAgentInfoForUid(int uid) { NetworkAgentInfo nai = getDefaultNetworkForUid(uid); final Network[] networks = getVpnUnderlyingNetworks(uid); @@ -1419,12 +1419,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nai = null; } } - - if (nai != null) { - return nai.getNetworkState(); - } else { - return NetworkState.EMPTY; - } + return nai; } /** @@ -1477,24 +1472,28 @@ 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); - } - networkInfo.setDetailedState( - getLegacyLockdownState(networkInfo.getDetailedState()), - "" /* reason */, null /* extraInfo */); - } - /** - * Apply any relevant filters to {@link NetworkState} for the given UID. For + * Apply any relevant filters to the specified {@link NetworkInfo} for the given UID. For * example, this may mark the network as {@link DetailedState#BLOCKED} based * on {@link #isNetworkWithCapabilitiesBlocked}. */ - private void filterNetworkStateForUid(NetworkState state, int uid, boolean ignoreBlocked) { - if (state == null || state.networkInfo == null || state.linkProperties == null) return; - filterNetworkInfo(state.networkInfo, state.networkCapabilities, uid, ignoreBlocked); + @NonNull + private NetworkInfo filterNetworkInfo(@NonNull NetworkInfo networkInfo, int type, + @NonNull NetworkCapabilities nc, int uid, boolean ignoreBlocked) { + NetworkInfo filtered = new NetworkInfo(networkInfo); + filtered.setType(type); + final DetailedState state = isNetworkWithCapabilitiesBlocked(nc, uid, ignoreBlocked) + ? DetailedState.BLOCKED + : filtered.getDetailedState(); + filtered.setDetailedState(getLegacyLockdownState(state), + "" /* reason */, null /* extraInfo */); + return filtered; + } + + private NetworkInfo getFilteredNetworkInfo(NetworkAgentInfo nai, int uid, + boolean ignoreBlocked) { + return filterNetworkInfo(nai.networkInfo, nai.networkInfo.getType(), + nai.networkCapabilities, uid, ignoreBlocked); } /** @@ -1508,10 +1507,11 @@ public class ConnectivityService extends IConnectivityManager.Stub public NetworkInfo getActiveNetworkInfo() { enforceAccessPermission(); final int uid = mDeps.getCallingUid(); - final NetworkState state = getUnfilteredActiveNetworkState(uid); - filterNetworkStateForUid(state, uid, false); - maybeLogBlockedNetworkInfo(state.networkInfo, uid); - return state.networkInfo; + final NetworkAgentInfo nai = getNetworkAgentInfoForUid(uid); + if (nai == null) return null; + final NetworkInfo networkInfo = getFilteredNetworkInfo(nai, uid, false); + maybeLogBlockedNetworkInfo(networkInfo, uid); + return networkInfo; } @Override @@ -1546,9 +1546,9 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public NetworkInfo getActiveNetworkInfoForUid(int uid, boolean ignoreBlocked) { PermissionUtils.enforceNetworkStackPermission(mContext); - final NetworkState state = getUnfilteredActiveNetworkState(uid); - filterNetworkStateForUid(state, uid, ignoreBlocked); - return state.networkInfo; + final NetworkAgentInfo nai = getNetworkAgentInfoForUid(uid); + if (nai == null) return null; + return getFilteredNetworkInfo(nai, uid, ignoreBlocked); } /** Returns a NetworkInfo object for a network that doesn't exist. */ @@ -1567,7 +1567,7 @@ public class ConnectivityService extends IConnectivityManager.Stub return info; } - private NetworkInfo getFilteredNetworkInfo(int networkType, int uid) { + private NetworkInfo getFilteredNetworkInfoForType(int networkType, int uid) { if (!mLegacyTypeTracker.isTypeSupported(networkType)) { return null; } @@ -1575,10 +1575,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (nai == null) { return makeFakeNetworkInfo(networkType, uid); } - final NetworkInfo info = new NetworkInfo(nai.networkInfo); - info.setType(networkType); - filterNetworkInfo(info, nai.networkCapabilities, uid, false); - return info; + return filterNetworkInfo(nai.networkInfo, networkType, nai.networkCapabilities, uid, + false); } @Override @@ -1588,27 +1586,23 @@ public class ConnectivityService extends IConnectivityManager.Stub if (getVpnUnderlyingNetworks(uid) != null) { // A VPN is active, so we may need to return one of its underlying networks. This // information is not available in LegacyTypeTracker, so we have to get it from - // getUnfilteredActiveNetworkState. - final NetworkState state = getUnfilteredActiveNetworkState(uid); - if (state.networkInfo != null && state.networkInfo.getType() == networkType) { - filterNetworkStateForUid(state, uid, false); - return state.networkInfo; + // getNetworkAgentInfoForUid. + final NetworkAgentInfo nai = getNetworkAgentInfoForUid(uid); + if (nai == null) return null; + final NetworkInfo networkInfo = getFilteredNetworkInfo(nai, uid, false); + if (networkInfo.getType() == networkType) { + return networkInfo; } } - return getFilteredNetworkInfo(networkType, uid); + return getFilteredNetworkInfoForType(networkType, uid); } @Override public NetworkInfo getNetworkInfoForUid(Network network, int uid, boolean ignoreBlocked) { enforceAccessPermission(); final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network); - if (nai != null) { - final NetworkState state = nai.getNetworkState(); - filterNetworkStateForUid(state, uid, ignoreBlocked); - return state.networkInfo; - } else { - return null; - } + if (nai == null) return null; + return getFilteredNetworkInfo(nai, uid, ignoreBlocked); } @Override @@ -1636,10 +1630,10 @@ public class ConnectivityService extends IConnectivityManager.Stub return null; } final int uid = mDeps.getCallingUid(); - if (!isNetworkWithCapabilitiesBlocked(nai.networkCapabilities, uid, false)) { - return nai.network; + if (isNetworkWithCapabilitiesBlocked(nai.networkCapabilities, uid, false)) { + return null; } - return null; + return nai.network; } @Override @@ -1728,9 +1722,9 @@ public class ConnectivityService extends IConnectivityManager.Stub public LinkProperties getActiveLinkProperties() { enforceAccessPermission(); final int uid = mDeps.getCallingUid(); - NetworkState state = getUnfilteredActiveNetworkState(uid); - if (state.linkProperties == null) return null; - return linkPropertiesRestrictedForCallerPermissions(state.linkProperties, + NetworkAgentInfo nai = getNetworkAgentInfoForUid(uid); + if (nai == null) return null; + return linkPropertiesRestrictedForCallerPermissions(nai.linkProperties, Binder.getCallingPid(), uid); }