Eliminate race conditions in UID-based network filtering.
The previous code retrieved information from the legacy tracker multiple
times for each user query, leading to race conditions where the info
could've changed between the calls.
Refactors the handling of legacy data types in ConnectivityService and
unifies call paths so that APIs that deal with legacy data types
(NetworkInfo and int/networkType) and newer types (such as Network) go
through common code paths, using NetworkState to hold all the necessary
data pieces. This enables follow-on bug fixes to getActiveNetworkInfo().
The changes are limited to public "query" APIs (those that retrieve some
network information or the other). More details about the specific
changes and their rationale can be found in the code review thread.
Bug: 17460017
Change-Id: I656ee7eddf2b8cace5627036452bb5748043406c
diff --git a/core/java/android/net/NetworkState.java b/core/java/android/net/NetworkState.java
index 2e0e9e4..d26c70d 100644
--- a/core/java/android/net/NetworkState.java
+++ b/core/java/android/net/NetworkState.java
@@ -29,20 +29,23 @@
public final NetworkInfo networkInfo;
public final LinkProperties linkProperties;
public final NetworkCapabilities networkCapabilities;
+ public final Network network;
/** Currently only used by testing. */
public final String subscriberId;
public final String networkId;
public NetworkState(NetworkInfo networkInfo, LinkProperties linkProperties,
- NetworkCapabilities networkCapabilities) {
- this(networkInfo, linkProperties, networkCapabilities, null, null);
+ NetworkCapabilities networkCapabilities, Network network) {
+ this(networkInfo, linkProperties, networkCapabilities, network, null, null);
}
public NetworkState(NetworkInfo networkInfo, LinkProperties linkProperties,
- NetworkCapabilities networkCapabilities, String subscriberId, String networkId) {
+ NetworkCapabilities networkCapabilities, Network network, String subscriberId,
+ String networkId) {
this.networkInfo = networkInfo;
this.linkProperties = linkProperties;
this.networkCapabilities = networkCapabilities;
+ this.network = network;
this.subscriberId = subscriberId;
this.networkId = networkId;
}
@@ -51,6 +54,7 @@
networkInfo = in.readParcelable(null);
linkProperties = in.readParcelable(null);
networkCapabilities = in.readParcelable(null);
+ network = in.readParcelable(null);
subscriberId = in.readString();
networkId = in.readString();
}
@@ -65,6 +69,7 @@
out.writeParcelable(networkInfo, flags);
out.writeParcelable(linkProperties, flags);
out.writeParcelable(networkCapabilities, flags);
+ out.writeParcelable(network, flags);
out.writeString(subscriberId);
out.writeString(networkId);
}
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index fa6d349..ec0e15c 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -819,20 +819,62 @@
}
}
- /**
- * Check if UID should be blocked from using the network represented by the given networkType.
- * @deprecated Uses mLegacyTypeTracker; cannot deal with multiple Networks of the same type.
- */
- private boolean isNetworkBlocked(int networkType, int uid) {
- return isNetworkWithLinkPropertiesBlocked(getLinkPropertiesForType(networkType), uid);
+ private NetworkState getFilteredNetworkState(int networkType, int uid) {
+ NetworkInfo info = null;
+ LinkProperties lp = null;
+ NetworkCapabilities nc = null;
+ Network network = null;
+
+ if (mLegacyTypeTracker.isTypeSupported(networkType)) {
+ NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
+ if (nai != null) {
+ synchronized (nai) {
+ info = new NetworkInfo(nai.networkInfo);
+ lp = new LinkProperties(nai.linkProperties);
+ nc = new NetworkCapabilities(nai.networkCapabilities);
+ network = new Network(nai.network);
+ }
+ info.setType(networkType);
+ } else {
+ info = new NetworkInfo(networkType, 0, getNetworkTypeName(networkType), "");
+ info.setDetailedState(NetworkInfo.DetailedState.DISCONNECTED, null, null);
+ info.setIsAvailable(true);
+ lp = new LinkProperties();
+ nc = new NetworkCapabilities();
+ network = null;
+ }
+ info = getFilteredNetworkInfo(info, lp, uid);
+ }
+
+ return new NetworkState(info, lp, nc, network);
}
- /**
- * Check if UID should be blocked from using the network represented by the given
- * NetworkAgentInfo.
- */
- private boolean isNetworkBlocked(NetworkAgentInfo nai, int uid) {
- return isNetworkWithLinkPropertiesBlocked(nai.linkProperties, uid);
+ private NetworkAgentInfo getNetworkAgentInfoForNetwork(Network network) {
+ if (network == null) {
+ return null;
+ }
+ synchronized (mNetworkForNetId) {
+ return mNetworkForNetId.get(network.netId);
+ }
+ };
+
+ private NetworkState getUnfilteredActiveNetworkState(int uid) {
+ NetworkInfo info = null;
+ LinkProperties lp = null;
+ NetworkCapabilities nc = null;
+ Network network = null;
+
+ NetworkAgentInfo nai = mNetworkForRequestId.get(mDefaultRequest.requestId);
+ if (nai != null) {
+ synchronized (nai) {
+ info = new NetworkInfo(nai.networkInfo);
+ lp = new LinkProperties(nai.linkProperties);
+ nc = new NetworkCapabilities(nai.networkCapabilities);
+ network = new Network(nai.network);
+ }
+ }
+
+ return new NetworkState(info, lp, nc, network);
}
/**
@@ -859,40 +901,16 @@
/**
* Return a filtered {@link NetworkInfo}, potentially marked
* {@link DetailedState#BLOCKED} based on
- * {@link #isNetworkBlocked}.
- * @deprecated Uses mLegacyTypeTracker; cannot deal with multiple Networks of the same type.
+ * {@link #isNetworkWithLinkPropertiesBlocked}.
*/
- private NetworkInfo getFilteredNetworkInfo(int networkType, int uid) {
- NetworkInfo info = getNetworkInfoForType(networkType);
- return getFilteredNetworkInfo(info, networkType, uid);
- }
-
- /*
- * @deprecated Uses mLegacyTypeTracker; cannot deal with multiple Networks of the same type.
- */
- private NetworkInfo getFilteredNetworkInfo(NetworkInfo info, int networkType, int uid) {
- if (isNetworkBlocked(networkType, uid)) {
- // network is blocked; clone and override state
- info = new NetworkInfo(info);
- info.setDetailedState(DetailedState.BLOCKED, null, null);
- if (VDBG) log("returning Blocked NetworkInfo");
- }
- if (mLockdownTracker != null) {
- info = mLockdownTracker.augmentNetworkInfo(info);
- if (VDBG) log("returning Locked NetworkInfo");
- }
- return info;
- }
-
- private NetworkInfo getFilteredNetworkInfo(NetworkAgentInfo nai, int uid) {
- NetworkInfo info = nai.networkInfo;
- if (isNetworkBlocked(nai, uid)) {
+ private NetworkInfo getFilteredNetworkInfo(NetworkInfo info, LinkProperties lp, int uid) {
+ if (info != null && isNetworkWithLinkPropertiesBlocked(lp, uid)) {
// network is blocked; clone and override state
info = new NetworkInfo(info);
info.setDetailedState(DetailedState.BLOCKED, null, null);
if (DBG) log("returning Blocked NetworkInfo");
}
- if (mLockdownTracker != null) {
+ if (info != null && mLockdownTracker != null) {
info = mLockdownTracker.augmentNetworkInfo(info);
if (DBG) log("returning Locked NetworkInfo");
}
@@ -910,7 +928,8 @@
public NetworkInfo getActiveNetworkInfo() {
enforceAccessPermission();
final int uid = Binder.getCallingUid();
- return getNetworkInfo(mActiveDefaultNetwork, uid);
+ NetworkState state = getUnfilteredActiveNetworkState(uid);
+ return getFilteredNetworkInfo(state.networkInfo, state.linkProperties, uid);
}
/**
@@ -945,8 +964,7 @@
NetworkInfo provNi = getProvisioningNetworkInfo();
if (provNi == null) {
- final int uid = Binder.getCallingUid();
- provNi = getNetworkInfo(mActiveDefaultNetwork, uid);
+ provNi = getActiveNetworkInfo();
}
if (DBG) log("getProvisioningOrActiveNetworkInfo: X provNi=" + provNi);
return provNi;
@@ -954,62 +972,50 @@
public NetworkInfo getActiveNetworkInfoUnfiltered() {
enforceAccessPermission();
- if (isNetworkTypeValid(mActiveDefaultNetwork)) {
- return getNetworkInfoForType(mActiveDefaultNetwork);
- }
- return null;
+ final int uid = Binder.getCallingUid();
+ NetworkState state = getUnfilteredActiveNetworkState(uid);
+ return state.networkInfo;
}
@Override
public NetworkInfo getActiveNetworkInfoForUid(int uid) {
enforceConnectivityInternalPermission();
- return getNetworkInfo(mActiveDefaultNetwork, uid);
+ NetworkState state = getUnfilteredActiveNetworkState(uid);
+ return getFilteredNetworkInfo(state.networkInfo, state.linkProperties, uid);
}
@Override
public NetworkInfo getNetworkInfo(int networkType) {
enforceAccessPermission();
final int uid = Binder.getCallingUid();
- return getNetworkInfo(networkType, uid);
+ NetworkState state = getFilteredNetworkState(networkType, uid);
+ return state.networkInfo;
}
- private NetworkInfo getNetworkInfo(int networkType, int uid) {
+ @Override
+ public NetworkInfo getNetworkInfoForNetwork(Network network) {
+ enforceAccessPermission();
+ final int uid = Binder.getCallingUid();
NetworkInfo info = null;
- if (isNetworkTypeValid(networkType)) {
- if (getNetworkInfoForType(networkType) != null) {
- info = getFilteredNetworkInfo(networkType, uid);
+ NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
+ if (nai != null) {
+ synchronized (nai) {
+ info = new NetworkInfo(nai.networkInfo);
+ info = getFilteredNetworkInfo(info, nai.linkProperties, uid);
}
}
return info;
}
@Override
- public NetworkInfo getNetworkInfoForNetwork(Network network) {
- enforceAccessPermission();
- if (network == null) return null;
-
- final int uid = Binder.getCallingUid();
- NetworkAgentInfo nai = null;
- synchronized (mNetworkForNetId) {
- nai = mNetworkForNetId.get(network.netId);
- }
- if (nai == null) return null;
- synchronized (nai) {
- if (nai.networkInfo == null) return null;
-
- return getFilteredNetworkInfo(nai, uid);
- }
- }
-
- @Override
public NetworkInfo[] getAllNetworkInfo() {
enforceAccessPermission();
- final int uid = Binder.getCallingUid();
final ArrayList<NetworkInfo> result = Lists.newArrayList();
for (int networkType = 0; networkType <= ConnectivityManager.MAX_NETWORK_TYPE;
networkType++) {
- if (getNetworkInfoForType(networkType) != null) {
- result.add(getFilteredNetworkInfo(networkType, uid));
+ NetworkInfo info = getNetworkInfo(networkType);
+ if (info != null) {
+ result.add(info);
}
}
return result.toArray(new NetworkInfo[result.size()]);
@@ -1019,11 +1025,11 @@
public Network getNetworkForType(int networkType) {
enforceAccessPermission();
final int uid = Binder.getCallingUid();
- if (isNetworkBlocked(networkType, uid)) {
- return null;
+ NetworkState state = getFilteredNetworkState(networkType, uid);
+ if (!isNetworkWithLinkPropertiesBlocked(state.linkProperties, uid)) {
+ return state.network;
}
- NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
- return (nai == null) ? null : nai.network;
+ return null;
}
@Override
@@ -1041,7 +1047,7 @@
@Override
public boolean isNetworkSupported(int networkType) {
enforceAccessPermission();
- return (isNetworkTypeValid(networkType) && (getNetworkInfoForType(networkType) != null));
+ return mLegacyTypeTracker.isTypeSupported(networkType);
}
/**
@@ -1054,14 +1060,20 @@
*/
@Override
public LinkProperties getActiveLinkProperties() {
- return getLinkPropertiesForType(mActiveDefaultNetwork);
+ enforceAccessPermission();
+ final int uid = Binder.getCallingUid();
+ NetworkState state = getUnfilteredActiveNetworkState(uid);
+ return state.linkProperties;
}
@Override
public LinkProperties getLinkPropertiesForType(int networkType) {
enforceAccessPermission();
- if (isNetworkTypeValid(networkType)) {
- return getLinkPropertiesForTypeInternal(networkType);
+ NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
+ if (nai != null) {
+ synchronized (nai) {
+ return new LinkProperties(nai.linkProperties);
+ }
}
return null;
}
@@ -1070,11 +1082,7 @@
@Override
public LinkProperties getLinkProperties(Network network) {
enforceAccessPermission();
- NetworkAgentInfo nai = null;
- synchronized (mNetworkForNetId) {
- nai = mNetworkForNetId.get(network.netId);
- }
-
+ NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
if (nai != null) {
synchronized (nai) {
return new LinkProperties(nai.linkProperties);
@@ -1086,10 +1094,7 @@
@Override
public NetworkCapabilities getNetworkCapabilities(Network network) {
enforceAccessPermission();
- NetworkAgentInfo nai = null;
- synchronized (mNetworkForNetId) {
- nai = mNetworkForNetId.get(network.netId);
- }
+ NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
if (nai != null) {
synchronized (nai) {
return new NetworkCapabilities(nai.networkCapabilities);
@@ -1105,36 +1110,22 @@
final ArrayList<NetworkState> result = Lists.newArrayList();
for (int networkType = 0; networkType <= ConnectivityManager.MAX_NETWORK_TYPE;
networkType++) {
- if (getNetworkInfoForType(networkType) != null) {
- final NetworkInfo info = getFilteredNetworkInfo(networkType, uid);
- final LinkProperties lp = getLinkPropertiesForTypeInternal(networkType);
- final NetworkCapabilities netcap = getNetworkCapabilitiesForType(networkType);
- result.add(new NetworkState(info, lp, netcap));
+ NetworkState state = getFilteredNetworkState(networkType, uid);
+ if (state.networkInfo != null) {
+ result.add(state);
}
}
return result.toArray(new NetworkState[result.size()]);
}
- private NetworkState getNetworkStateUnchecked(int networkType) {
- if (isNetworkTypeValid(networkType)) {
- NetworkInfo info = getNetworkInfoForType(networkType);
- if (info != null) {
- return new NetworkState(info,
- getLinkPropertiesForTypeInternal(networkType),
- getNetworkCapabilitiesForType(networkType));
- }
- }
- return null;
- }
-
@Override
public NetworkQuotaInfo getActiveNetworkQuotaInfo() {
enforceAccessPermission();
-
+ final int uid = Binder.getCallingUid();
final long token = Binder.clearCallingIdentity();
try {
- final NetworkState state = getNetworkStateUnchecked(mActiveDefaultNetwork);
- if (state != null) {
+ final NetworkState state = getUnfilteredActiveNetworkState(uid);
+ if (state.networkInfo != null) {
try {
return mPolicyManager.getNetworkQuotaInfo(state);
} catch (RemoteException e) {
@@ -1149,17 +1140,18 @@
@Override
public boolean isActiveNetworkMetered() {
enforceAccessPermission();
+ final int uid = Binder.getCallingUid();
final long token = Binder.clearCallingIdentity();
try {
- return isNetworkMeteredUnchecked(mActiveDefaultNetwork);
+ return isActiveNetworkMeteredUnchecked(uid);
} finally {
Binder.restoreCallingIdentity(token);
}
}
- private boolean isNetworkMeteredUnchecked(int networkType) {
- final NetworkState state = getNetworkStateUnchecked(networkType);
- if (state != null) {
+ private boolean isActiveNetworkMeteredUnchecked(int uid) {
+ final NetworkState state = getUnfilteredActiveNetworkState(uid);
+ if (state.networkInfo != null) {
try {
return mPolicyManager.isNetworkMetered(state);
} catch (RemoteException e) {
@@ -1330,16 +1322,18 @@
// kick off connectivity change broadcast for active network, since
// global background policy change is radical.
- final int networkType = mActiveDefaultNetwork;
- if (isNetworkTypeValid(networkType)) {
- final NetworkStateTracker tracker = mNetTrackers[networkType];
- if (tracker != null) {
- final NetworkInfo info = tracker.getNetworkInfo();
- if (info != null && info.isConnected()) {
- sendConnectedBroadcast(info);
- }
- }
- }
+ // TODO: Dead code; remove.
+ //
+ // final int networkType = mActiveDefaultNetwork;
+ // if (isNetworkTypeValid(networkType)) {
+ // final NetworkStateTracker tracker = mNetTrackers[networkType];
+ // if (tracker != null) {
+ // final NetworkInfo info = tracker.getNetworkInfo();
+ // if (info != null && info.isConnected()) {
+ // sendConnectedBroadcast(info);
+ // }
+ // }
+ // }
}
};
@@ -1804,10 +1798,7 @@
private boolean isLiveNetworkAgent(NetworkAgentInfo nai, String msg) {
if (nai.network == null) return false;
- final NetworkAgentInfo officialNai;
- synchronized (mNetworkForNetId) {
- officialNai = mNetworkForNetId.get(nai.network.netId);
- }
+ final NetworkAgentInfo officialNai = getNetworkAgentInfoForNetwork(nai.network);
if (officialNai != null && officialNai.equals(nai)) return true;
if (officialNai != null || VDBG) {
loge(msg + " - isLiveNetworkAgent found mismatched netId: " + officialNai +
@@ -2003,7 +1994,7 @@
* to the link that may have incorrectly setup by the lower
* levels.
*/
- LinkProperties lp = getLinkPropertiesForTypeInternal(info.getType());
+ LinkProperties lp = getLinkPropertiesForType(info.getType());
if (DBG) {
log("EVENT_STATE_CHANGED: connected to provisioning network, lp=" + lp);
}
@@ -2556,10 +2547,7 @@
if (network == null) return;
final int uid = Binder.getCallingUid();
- NetworkAgentInfo nai = null;
- synchronized (mNetworkForNetId) {
- nai = mNetworkForNetId.get(network.netId);
- }
+ NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
if (nai == null) return;
if (DBG) log("reportBadNetwork(" + nai.name() + ") by " + uid);
synchronized (nai) {
@@ -2567,7 +2555,7 @@
// which isn't meant to work on uncreated networks.
if (!nai.created) return;
- if (isNetworkBlocked(nai, uid)) return;
+ if (isNetworkWithLinkPropertiesBlocked(nai.linkProperties, uid)) return;
nai.networkMonitor.sendMessage(NetworkMonitor.CMD_FORCE_REEVALUATION, uid);
}
@@ -4371,44 +4359,6 @@
return "UNKNOWN";
}
- private LinkProperties getLinkPropertiesForTypeInternal(int networkType) {
- NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
- if (nai != null) {
- synchronized (nai) {
- return new LinkProperties(nai.linkProperties);
- }
- }
- return new LinkProperties();
- }
-
- private NetworkInfo getNetworkInfoForType(int networkType) {
- if (!mLegacyTypeTracker.isTypeSupported(networkType))
- return null;
-
- NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
- if (nai != null) {
- NetworkInfo result = new NetworkInfo(nai.networkInfo);
- result.setType(networkType);
- return result;
- } else {
- NetworkInfo result = new NetworkInfo(
- networkType, 0, ConnectivityManager.getNetworkTypeName(networkType), "");
- result.setDetailedState(NetworkInfo.DetailedState.DISCONNECTED, null, null);
- result.setIsAvailable(true);
- return result;
- }
- }
-
- private NetworkCapabilities getNetworkCapabilitiesForType(int networkType) {
- NetworkAgentInfo nai = mLegacyTypeTracker.getNetworkForType(networkType);
- if (nai != null) {
- synchronized (nai) {
- return new NetworkCapabilities(nai.networkCapabilities);
- }
- }
- return new NetworkCapabilities();
- }
-
@Override
public boolean addVpnAddress(String address, int prefixLength) {
throwIfLockdownEnabled();
diff --git a/services/tests/servicestests/src/com/android/server/NetworkStatsServiceTest.java b/services/tests/servicestests/src/com/android/server/NetworkStatsServiceTest.java
index c115339..f9a03fc 100644
--- a/services/tests/servicestests/src/com/android/server/NetworkStatsServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/NetworkStatsServiceTest.java
@@ -1006,7 +1006,7 @@
info.setDetailedState(DetailedState.CONNECTED, null, null);
final LinkProperties prop = new LinkProperties();
prop.setInterfaceName(TEST_IFACE);
- return new NetworkState(info, prop, null, null, TEST_SSID);
+ return new NetworkState(info, prop, null, null, null, TEST_SSID);
}
private static NetworkState buildMobile3gState(String subscriberId) {
@@ -1015,7 +1015,7 @@
info.setDetailedState(DetailedState.CONNECTED, null, null);
final LinkProperties prop = new LinkProperties();
prop.setInterfaceName(TEST_IFACE);
- return new NetworkState(info, prop, null, subscriberId, null);
+ return new NetworkState(info, prop, null, null, subscriberId, null);
}
private static NetworkState buildMobile4gState(String iface) {
@@ -1023,7 +1023,7 @@
info.setDetailedState(DetailedState.CONNECTED, null, null);
final LinkProperties prop = new LinkProperties();
prop.setInterfaceName(iface);
- return new NetworkState(info, prop, null);
+ return new NetworkState(info, prop, null, null);
}
private NetworkStats buildEmptyStats() {