Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 78caa258 authored by Hugo Benichi's avatar Hugo Benichi
Browse files

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
Change-Id: I915aac527fc8828b32bf35fee870add2dfb11d8d
parent 1b2ed439

services/core/java/com/android/server/ConnectivityService.java

+30 −17
Original line number Diff line number Diff line
@@ -487,8 +487,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>[])
@@ -508,12 +516,13 @@ public class ConnectivityService extends IConnectivityManager.Stub 
        }



        public NetworkAgentInfo getNetworkForType(int type) {

            synchronized (mTypeLists) {

                if (isTypeSupported(type) && !mTypeLists[type].isEmpty()) {

                    return mTypeLists[type].get(0);

            } else {

                return null;

                }

            }

            return null;

        }



        private void maybeLogBroadcast(NetworkAgentInfo nai, DetailedState state, int type,

                boolean isDefaultNetwork) {
@@ -535,12 +544,13 @@ public class ConnectivityService extends IConnectivityManager.Stub 
            if (list.contains(nai)) {

                return;

            }



            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);

            }
@@ -552,12 +562,13 @@ public class ConnectivityService extends IConnectivityManager.Stub 
            if (list == null || list.isEmpty()) {

                return;

            }



            final boolean wasFirstNetwork = list.get(0).equals(nai);



            synchronized (mTypeLists) {

                if (!list.remove(nai)) {

                    return;

                }

            }



            final DetailedState state = DetailedState.DISCONNECTED;
@@ -591,8 +602,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);

                }
@@ -617,12 +628,14 @@ public class ConnectivityService extends IConnectivityManager.Stub 
            pw.println();

            pw.println("Current state:");

            pw.increaseIndent();

            synchronized (mTypeLists) {

                for (int type = 0; type < mTypeLists.length; type++) {

                if (mTypeLists[type] == null|| mTypeLists[type].size() == 0) continue;

                    if (mTypeLists[type] == null || mTypeLists[type].isEmpty()) continue;

                    for (NetworkAgentInfo nai : mTypeLists[type]) {

                        pw.println(type + " " + naiToString(nai));

                    }

                }

            }

            pw.decreaseIndent();

            pw.decreaseIndent();

            pw.println();