From 57af1a02a622d8b6e1f327467bc1913379b72b1c Mon Sep 17 00:00:00 2001
From: Hugo Benichi <hugobenichi@google.com>
Date: Thu, 6 Apr 2017 16:01:44 +0900
Subject: [PATCH] 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() {