From 5224f00b5110e37a288692a3449387aa199a1fc3 Mon Sep 17 00:00:00 2001
From: Hugo Benichi <hugobenichi@google.com>
Date: Tue, 21 Jun 2016 09:48:07 +0900
Subject: Fix unsafe concurrent access in LegacyTypeTracker

This patch adds synchronization inside LegacyTypeTracker so that
getNetworkForType() can safely run concurrently with remove().

Without synchronization if remove() removes the last network for a
given type while getNetworkForType() runs for the same type, it is
possible that getNetworkForType tries to access the head of an empty
list, resulting in a runtime exception.

This issue was found by zoran.jovanovic@sonymobile.com who proposed a
fix in AOSP (Change-Id: Ia963662edb9d643790e8d9439e4dbdcac4c2187b).

This patch differs from the fix proposed by the bug reporter and tries
instead to do the minimum amount of locking to make getNetworkForType
safe.

Bug: 29030387

(cherry picked from commit 78caa25870391f676e1edbd448b5ff3e12a99a1e)

Change-Id: I915aac527fc8828b32bf35fee870add2dfb11d8d
---
 .../com/android/server/ConnectivityService.java    | 47 ++++++++++++++--------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 327fb8a7ef2d..7e5cc1231875 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -468,8 +468,16 @@ public class ConnectivityService extends IConnectivityManager.Stub
          *
          * The actual lists are populated when we scan the network types that
          * are supported on this device.
+         *
+         * Threading model:
+         *  - addSupportedType() is only called in the constructor
+         *  - add(), update(), remove() are only called from the ConnectivityService handler thread.
+         *    They are therefore not thread-safe with respect to each other.
+         *  - getNetworkForType() can be called at any time on binder threads. It is synchronized
+         *    on mTypeLists to be thread-safe with respect to a concurrent remove call.
+         *  - dump is thread-safe with respect to concurrent add and remove calls.
          */
-        private ArrayList<NetworkAgentInfo> mTypeLists[];
+        private final ArrayList<NetworkAgentInfo> mTypeLists[];
 
         public LegacyTypeTracker() {
             mTypeLists = (ArrayList<NetworkAgentInfo>[])
@@ -489,11 +497,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
 
         public NetworkAgentInfo getNetworkForType(int type) {
-            if (isTypeSupported(type) && !mTypeLists[type].isEmpty()) {
-                return mTypeLists[type].get(0);
-            } else {
-                return null;
+            synchronized (mTypeLists) {
+                if (isTypeSupported(type) && !mTypeLists[type].isEmpty()) {
+                    return mTypeLists[type].get(0);
+                }
             }
+            return null;
         }
 
         private void maybeLogBroadcast(NetworkAgentInfo nai, DetailedState state, int type,
@@ -516,12 +525,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
             if (list.contains(nai)) {
                 return;
             }
-
-            list.add(nai);
+            synchronized (mTypeLists) {
+                list.add(nai);
+            }
 
             // Send a broadcast if this is the first network of its type or if it's the default.
             final boolean isDefaultNetwork = isDefaultNetwork(nai);
-            if (list.size() == 1 || isDefaultNetwork) {
+            if ((list.size() == 1) || isDefaultNetwork) {
                 maybeLogBroadcast(nai, DetailedState.CONNECTED, type, isDefaultNetwork);
                 sendLegacyNetworkBroadcast(nai, DetailedState.CONNECTED, type);
             }
@@ -533,11 +543,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
             if (list == null || list.isEmpty()) {
                 return;
             }
-
             final boolean wasFirstNetwork = list.get(0).equals(nai);
 
-            if (!list.remove(nai)) {
-                return;
+            synchronized (mTypeLists) {
+                if (!list.remove(nai)) {
+                    return;
+                }
             }
 
             final DetailedState state = DetailedState.DISCONNECTED;
@@ -572,8 +583,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
             for (int type = 0; type < mTypeLists.length; type++) {
                 final ArrayList<NetworkAgentInfo> list = mTypeLists[type];
                 final boolean contains = (list != null && list.contains(nai));
-                final boolean isFirst = (list != null && list.size() > 0 && nai == list.get(0));
-                if (isFirst || (contains && isDefault)) {
+                final boolean isFirst = contains && (nai == list.get(0));
+                if (isFirst || contains && isDefault) {
                     maybeLogBroadcast(nai, state, type, isDefault);
                     sendLegacyNetworkBroadcast(nai, state, type);
                 }
@@ -598,10 +609,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
             pw.println();
             pw.println("Current state:");
             pw.increaseIndent();
-            for (int type = 0; type < mTypeLists.length; type++) {
-                if (mTypeLists[type] == null|| mTypeLists[type].size() == 0) continue;
-                for (NetworkAgentInfo nai : mTypeLists[type]) {
-                    pw.println(type + " " + naiToString(nai));
+            synchronized (mTypeLists) {
+                for (int type = 0; type < mTypeLists.length; type++) {
+                    if (mTypeLists[type] == null || mTypeLists[type].isEmpty()) continue;
+                    for (NetworkAgentInfo nai : mTypeLists[type]) {
+                        pw.println(type + " " + naiToString(nai));
+                    }
                 }
             }
             pw.decreaseIndent();
-- 
cgit v1.2.3