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

1 files 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 25ac008fa586..50c0a124fb8a 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -1265,13 +1265,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) {
@@ -3027,7 +3030,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 {
@@ -3038,21 +3042,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() {