From 475dd68f1117aa86dbbde3a8ff26c131c1c2dd09 Mon Sep 17 00:00:00 2001
From: Hugo Benichi <hugobenichi@google.com>
Date: Thu, 6 Apr 2017 16:01:44 +0900
Subject: [PATCH] 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 34cb6c95c8..76d3262823 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -1304,13 +1304,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) {
@@ -3132,7 +3135,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 {
@@ -3143,21 +3147,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() {