From 57af1a02a622d8b6e1f327467bc1913379b72b1c Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Thu, 6 Apr 2017 16:01:44 +0900 Subject: [PATCH 1/2] DO NOT MERGE ANYWHERE ConnectivityService: safer locking This path changes a dangerous lock path in reportNetworkConnectivity(). This methods is called outside of the main ConnectivityService handler and takes a lock on a specific NetworkAgentInfo whose connectivity status is being reported. While this lock is held, reportNetworkConnectivity() goes on and query the network policy state for that network, which may ends into NetworkPolicyManagerService. Instead, the lock on NetworkAgentInfo is only held long enough to make a copy of LinkProperties, which is then passed to NetworkPolicyManagerService without that lock. Bug: 36902662 Test: could not repro b/36902662, reportNetworkConnectivity() works. $ runtest frameworks-net Change-Id: Iac4b75bcecbdddb0ac695c8b1a87ae755f62f47f --- .../android/server/ConnectivityService.java | 39 +++++++++++-------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 614a94fa1e..6f032afcdd 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1327,13 +1327,16 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public LinkProperties getLinkProperties(Network network) { enforceAccessPermission(); - NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network); - if (nai != null) { - synchronized (nai) { - return new LinkProperties(nai.linkProperties); - } + return getLinkProperties(getNetworkAgentInfoForNetwork(network)); + } + + private LinkProperties getLinkProperties(NetworkAgentInfo nai) { + if (nai == null) { + return null; + } + synchronized (nai) { + return new LinkProperties(nai.linkProperties); } - return null; } private NetworkCapabilities getNetworkCapabilitiesInternal(NetworkAgentInfo nai) { @@ -3169,7 +3172,8 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceAccessPermission(); enforceInternetPermission(); - NetworkAgentInfo nai; + // TODO: execute this logic on ConnectivityService handler. + final NetworkAgentInfo nai; if (network == null) { nai = getDefaultNetwork(); } else { @@ -3180,21 +3184,24 @@ public class ConnectivityService extends IConnectivityManager.Stub return; } // Revalidate if the app report does not match our current validated state. - if (hasConnectivity == nai.lastValidated) return; + if (hasConnectivity == nai.lastValidated) { + return; + } final int uid = Binder.getCallingUid(); if (DBG) { log("reportNetworkConnectivity(" + nai.network.netId + ", " + hasConnectivity + ") by " + uid); } - synchronized (nai) { - // Validating a network that has not yet connected could result in a call to - // rematchNetworkAndRequests() which is not meant to work on such networks. - if (!nai.everConnected) return; - - if (isNetworkWithLinkPropertiesBlocked(nai.linkProperties, uid, false)) return; - - nai.networkMonitor.sendMessage(NetworkMonitor.CMD_FORCE_REEVALUATION, uid); + // Validating a network that has not yet connected could result in a call to + // rematchNetworkAndRequests() which is not meant to work on such networks. + if (!nai.everConnected) { + return; } + LinkProperties lp = getLinkProperties(nai); + if (isNetworkWithLinkPropertiesBlocked(lp, uid, false)) { + return; + } + nai.networkMonitor.sendMessage(NetworkMonitor.CMD_FORCE_REEVALUATION, uid); } private ProxyInfo getDefaultProxy() { From 1681f064ffd2d76a23054c8222771e1d6efea52d Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Thu, 6 Apr 2017 17:22:18 +0900 Subject: [PATCH 2/2] DO NOT MERGE ANYWHERE ConnectivityService: move reportNetworkConnectivity to handler This patch moves reportNetworkConnectivity onto the handler of ConnectivityService. This allows: - to inspect NetworkAgentInfo on the ConnectivityService handler, which is always more correct than doing so on a Binder thread. - to improve locking policies around NetworkAgentInfo. Test: $ runtest frameworks-net Bug: 37119619, 36902662 Change-Id: I49a765826e65c29a1995242290e5e7544112c94e --- .../android/server/ConnectivityService.java | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 6f032afcdd..a0707d5ce1 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -372,11 +372,6 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private static final int EVENT_SET_ACCEPT_UNVALIDATED = 28; - /** - * used to specify whether a network should not be penalized when it becomes unvalidated. - */ - private static final int EVENT_SET_AVOID_UNVALIDATED = 35; - /** * used to ask the user to confirm a connection to an unvalidated network. * obj = network @@ -404,6 +399,16 @@ public class ConnectivityService extends IConnectivityManager.Stub private static final int EVENT_REQUEST_LINKPROPERTIES = 32; private static final int EVENT_REQUEST_NETCAPABILITIES = 33; + /* + * used to specify whether a network should not be penalized when it becomes unvalidated. + */ + private static final int EVENT_SET_AVOID_UNVALIDATED = 35; + + /** + * used to trigger revalidation of a network. + */ + private static final int EVENT_REVALIDATE_NETWORK = 36; + /** Handler thread used for both of the handlers below. */ @VisibleForTesting protected final HandlerThread mHandlerThread; @@ -2999,6 +3004,11 @@ public class ConnectivityService extends IConnectivityManager.Stub } break; } + case EVENT_REVALIDATE_NETWORK: { + boolean hasConnectivity = (msg.arg2 == 1); + handleReportNetworkConnectivity((Network) msg.obj, msg.arg1, hasConnectivity); + break; + } } } } @@ -3171,8 +3181,14 @@ public class ConnectivityService extends IConnectivityManager.Stub public void reportNetworkConnectivity(Network network, boolean hasConnectivity) { enforceAccessPermission(); enforceInternetPermission(); + final int uid = Binder.getCallingUid(); + final int connectivityInfo = hasConnectivity ? 1 : 0; + mHandler.sendMessage( + mHandler.obtainMessage(EVENT_REVALIDATE_NETWORK, uid, connectivityInfo, network)); + } - // TODO: execute this logic on ConnectivityService handler. + private void handleReportNetworkConnectivity( + Network network, int uid, boolean hasConnectivity) { final NetworkAgentInfo nai; if (network == null) { nai = getDefaultNetwork(); @@ -3187,10 +3203,9 @@ public class ConnectivityService extends IConnectivityManager.Stub if (hasConnectivity == nai.lastValidated) { return; } - final int uid = Binder.getCallingUid(); if (DBG) { - log("reportNetworkConnectivity(" + nai.network.netId + ", " + hasConnectivity + - ") by " + uid); + int netid = nai.network.netId; + log("reportNetworkConnectivity(" + netid + ", " + hasConnectivity + ") by " + uid); } // Validating a network that has not yet connected could result in a call to // rematchNetworkAndRequests() which is not meant to work on such networks.