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

Commit 3962f678 authored by Remi NGUYEN VAN's avatar Remi NGUYEN VAN
Browse files

Fix race when starting NetworkMonitor 

NetworkMonitor obtained LinkProperties and NetworkCapabilities via
synchronous calls to ConnectivityManager after receiving an asynchronous
notification, which is prone to races: the network could be gone before
the LinkProperties/NetworkCapabilities can be fetched.

Fix the race by passing LinkProperties/NetworkCapabilities directly to
NetworkMonitor in the asynchronous notifications.

Test: atest FrameworksNetTests NetworkStackTests
Test: booted, WiFi works
Bug: 129375892
Change-Id: I200ac7ca6ff79590b11c9be705f650c92fd3cb63
parent 1859ae7e

core/java/android/net/NetworkCapabilities.aidl

+1 −1
Original line number Original line Diff line number Diff line
@@ -17,5 +17,5 @@ 




package android.net;

package android.net;





parcelable NetworkCapabilities;

@JavaOnlyStableParcelable parcelable NetworkCapabilities;

packages/NetworkStack/src/com/android/server/NetworkStackService.java

+8 −6
Original line number Original line Diff line number Diff line
@@ -35,7 +35,9 @@ import android.net.INetd; 
import android.net.INetworkMonitor;

import android.net.INetworkMonitor;

import android.net.INetworkMonitorCallbacks;

import android.net.INetworkMonitorCallbacks;

import android.net.INetworkStackConnector;

import android.net.INetworkStackConnector;

import android.net.LinkProperties;

import android.net.Network;

import android.net.Network;

import android.net.NetworkCapabilities;

import android.net.PrivateDnsConfigParcel;

import android.net.PrivateDnsConfigParcel;

import android.net.dhcp.DhcpServer;

import android.net.dhcp.DhcpServer;

import android.net.dhcp.DhcpServingParams;

import android.net.dhcp.DhcpServingParams;
@@ -307,9 +309,9 @@ public class NetworkStackService extends Service { 
        }

        }





        @Override

        @Override

        public void notifyNetworkConnected() {

        public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) {

            checkNetworkStackCallingPermission();

            checkNetworkStackCallingPermission();

            mNm.notifyNetworkConnected();

            mNm.notifyNetworkConnected(lp, nc);

        }

        }





        @Override

        @Override
@@ -319,15 +321,15 @@ public class NetworkStackService extends Service { 
        }

        }





        @Override

        @Override

        public void notifyLinkPropertiesChanged() {

        public void notifyLinkPropertiesChanged(LinkProperties lp) {

            checkNetworkStackCallingPermission();

            checkNetworkStackCallingPermission();

            mNm.notifyLinkPropertiesChanged();

            mNm.notifyLinkPropertiesChanged(lp);

        }

        }





        @Override

        @Override

        public void notifyNetworkCapabilitiesChanged() {

        public void notifyNetworkCapabilitiesChanged(NetworkCapabilities nc) {

            checkNetworkStackCallingPermission();

            checkNetworkStackCallingPermission();

            mNm.notifyNetworkCapabilitiesChanged();

            mNm.notifyNetworkCapabilitiesChanged(nc);

        }

        }

    }

    }

}
 
}

packages/NetworkStack/src/com/android/server/connectivity/NetworkMonitor.java

+40 −42
Original line number Original line Diff line number Diff line
@@ -78,6 +78,7 @@ import android.telephony.SignalStrength; 
import android.telephony.TelephonyManager;

import android.telephony.TelephonyManager;

import android.text.TextUtils;

import android.text.TextUtils;

import android.util.Log;

import android.util.Log;

import android.util.Pair;





import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.util.RingBufferIndices;

import com.android.internal.util.RingBufferIndices;
@@ -221,19 +222,31 @@ public class NetworkMonitor extends StateMachine { 
     * Message to self indicating captive portal detection is completed.

     * Message to self indicating captive portal detection is completed.

     * obj = CaptivePortalProbeResult for detection result;

     * obj = CaptivePortalProbeResult for detection result;

     */

     */

    public static final int CMD_PROBE_COMPLETE = 16;

    private static final int CMD_PROBE_COMPLETE = 16;





    /**

    /**

     * ConnectivityService notifies NetworkMonitor of DNS query responses event.

     * ConnectivityService notifies NetworkMonitor of DNS query responses event.

     * arg1 = returncode in OnDnsEvent which indicates the response code for the DNS query.

     * arg1 = returncode in OnDnsEvent which indicates the response code for the DNS query.

     */

     */

    public static final int EVENT_DNS_NOTIFICATION = 17;

    private static final int EVENT_DNS_NOTIFICATION = 17;





    /**

    /**

     * ConnectivityService notifies NetworkMonitor that the user accepts partial connectivity and

     * ConnectivityService notifies NetworkMonitor that the user accepts partial connectivity and

     * NetworkMonitor should ignore the https probe.

     * NetworkMonitor should ignore the https probe.

     */

     */

    public static final int EVENT_ACCEPT_PARTIAL_CONNECTIVITY = 18;

    private static final int EVENT_ACCEPT_PARTIAL_CONNECTIVITY = 18;



    /**

     * ConnectivityService notifies NetworkMonitor of changed LinkProperties.

     * obj = new LinkProperties.

     */

    private static final int EVENT_LINK_PROPERTIES_CHANGED = 19;



    /**

     * ConnectivityService notifies NetworkMonitor of changed NetworkCapabilities.

     * obj = new NetworkCapabilities.

     */

    private static final int EVENT_NETWORK_CAPABILITIES_CHANGED = 20;





    // Start mReevaluateDelayMs at this value and double.

    // Start mReevaluateDelayMs at this value and double.

    private static final int INITIAL_REEVALUATE_DELAY_MS = 1000;

    private static final int INITIAL_REEVALUATE_DELAY_MS = 1000;
@@ -379,10 +392,8 @@ public class NetworkMonitor extends StateMachine { 
        mDataStallValidDnsTimeThreshold = getDataStallValidDnsTimeThreshold();

        mDataStallValidDnsTimeThreshold = getDataStallValidDnsTimeThreshold();

        mDataStallEvaluationType = getDataStallEvalutionType();

        mDataStallEvaluationType = getDataStallEvalutionType();





        // mLinkProperties and mNetworkCapbilities must never be null or we will NPE.

        // Provide empty LinkProperties and NetworkCapabilities to make sure they are never null,

        // Provide empty objects in case we are started and the network disconnects before

        // even before notifyNetworkConnected.

        // we can ever fetch them.

        // TODO: Delete ASAP.

        mLinkProperties = new LinkProperties();

        mLinkProperties = new LinkProperties();

        mNetworkCapabilities = new NetworkCapabilities(null);

        mNetworkCapabilities = new NetworkCapabilities(null);

    }

    }
@@ -434,8 +445,16 @@ public class NetworkMonitor extends StateMachine { 
    /**

    /**

     * Send a notification to NetworkMonitor indicating that the network is now connected.

     * Send a notification to NetworkMonitor indicating that the network is now connected.

     */

     */

    public void notifyNetworkConnected() {

    public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) {

        sendMessage(CMD_NETWORK_CONNECTED);

        sendMessage(CMD_NETWORK_CONNECTED, new Pair<>(

                new LinkProperties(lp), new NetworkCapabilities(nc)));

    }



    private void updateConnectedNetworkAttributes(Message connectedMsg) {

        final Pair<LinkProperties, NetworkCapabilities> attrs =

                (Pair<LinkProperties, NetworkCapabilities>) connectedMsg.obj;

        mLinkProperties = attrs.first;

        mNetworkCapabilities = attrs.second;

    }

    }





    /**

    /**
@@ -448,37 +467,15 @@ public class NetworkMonitor extends StateMachine { 
    /**

    /**

     * Send a notification to NetworkMonitor indicating that link properties have changed.

     * Send a notification to NetworkMonitor indicating that link properties have changed.

     */

     */

    public void notifyLinkPropertiesChanged() {

    public void notifyLinkPropertiesChanged(final LinkProperties lp) {

        getHandler().post(() -> {

        sendMessage(EVENT_LINK_PROPERTIES_CHANGED, new LinkProperties(lp));

            updateLinkProperties();

        });

    }



    private void updateLinkProperties() {

        final LinkProperties lp = mCm.getLinkProperties(mNetwork);

        // If null, we should soon get a message that the network was disconnected, and will stop.

        if (lp != null) {

            // TODO: send LinkProperties parceled in notifyLinkPropertiesChanged() and start().

            mLinkProperties = lp;

        }

    }

    }





    /**

    /**

     * Send a notification to NetworkMonitor indicating that network capabilities have changed.

     * Send a notification to NetworkMonitor indicating that network capabilities have changed.

     */

     */

    public void notifyNetworkCapabilitiesChanged() {

    public void notifyNetworkCapabilitiesChanged(final NetworkCapabilities nc) {

        getHandler().post(() -> {

        sendMessage(EVENT_NETWORK_CAPABILITIES_CHANGED, new NetworkCapabilities(nc));

            updateNetworkCapabilities();

        });

    }



    private void updateNetworkCapabilities() {

        final NetworkCapabilities nc = mCm.getNetworkCapabilities(mNetwork);

        // If null, we should soon get a message that the network was disconnected, and will stop.

        if (nc != null) {

            // TODO: send NetworkCapabilities parceled in notifyNetworkCapsChanged() and start().

            mNetworkCapabilities = nc;

        }

    }

    }





    /**

    /**
@@ -546,17 +543,11 @@ public class NetworkMonitor extends StateMachine { 
    // DefaultState is the parent of all States.  It exists only to handle CMD_* messages but

    // DefaultState is the parent of all States.  It exists only to handle CMD_* messages but

    // does not entail any real state (hence no enter() or exit() routines).

    // does not entail any real state (hence no enter() or exit() routines).

    private class DefaultState extends State {

    private class DefaultState extends State {

        @Override

        public void enter() {

            // TODO: have those passed parceled in start() and remove this

            updateLinkProperties();

            updateNetworkCapabilities();

        }



        @Override

        @Override

        public boolean processMessage(Message message) {

        public boolean processMessage(Message message) {

            switch (message.what) {

            switch (message.what) {

                case CMD_NETWORK_CONNECTED:

                case CMD_NETWORK_CONNECTED:

                    updateConnectedNetworkAttributes(message);

                    logNetworkEvent(NetworkEvent.NETWORK_CONNECTED);

                    logNetworkEvent(NetworkEvent.NETWORK_CONNECTED);

                    transitionTo(mEvaluatingState);

                    transitionTo(mEvaluatingState);

                    return HANDLED;

                    return HANDLED;
@@ -660,6 +651,12 @@ public class NetworkMonitor extends StateMachine { 
                case EVENT_ACCEPT_PARTIAL_CONNECTIVITY:

                case EVENT_ACCEPT_PARTIAL_CONNECTIVITY:

                    mAcceptPartialConnectivity = true;

                    mAcceptPartialConnectivity = true;

                    break;

                    break;

                case EVENT_LINK_PROPERTIES_CHANGED:

                    mLinkProperties = (LinkProperties) message.obj;

                    break;

                case EVENT_NETWORK_CAPABILITIES_CHANGED:

                    mNetworkCapabilities = (NetworkCapabilities) message.obj;

                    break;

                default:

                default:

                    break;

                    break;

            }

            }
@@ -684,6 +681,7 @@ public class NetworkMonitor extends StateMachine { 
        public boolean processMessage(Message message) {

        public boolean processMessage(Message message) {

            switch (message.what) {

            switch (message.what) {

                case CMD_NETWORK_CONNECTED:

                case CMD_NETWORK_CONNECTED:

                    updateConnectedNetworkAttributes(message);

                    transitionTo(mValidatedState);

                    transitionTo(mValidatedState);

                    break;

                    break;

                case CMD_EVALUATE_PRIVATE_DNS:

                case CMD_EVALUATE_PRIVATE_DNS:

packages/NetworkStack/tests/src/com/android/server/connectivity/NetworkMonitorTest.java

+84 −96

File changed.

Preview size limit exceeded, changes collapsed.

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

+9 −18
Original line number Original line Diff line number Diff line
@@ -5382,7 +5382,7 @@ public class ConnectivityService extends IConnectivityManager.Stub 
                mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mNMS,

                mContext, mTrackerHandler, new NetworkMisc(networkMisc), this, mNetd, mNMS,

                factorySerialNumber);

                factorySerialNumber);

        // Make sure the network capabilities reflect what the agent info says.

        // Make sure the network capabilities reflect what the agent info says.

        nai.networkCapabilities = mixInCapabilities(nai, nc);

        nai.setNetworkCapabilities(mixInCapabilities(nai, nc));

        final String extraInfo = networkInfo.getExtraInfo();

        final String extraInfo = networkInfo.getExtraInfo();

        final String name = TextUtils.isEmpty(extraInfo)

        final String name = TextUtils.isEmpty(extraInfo)

                ? nai.networkCapabilities.getSSID() : extraInfo;

                ? nai.networkCapabilities.getSSID() : extraInfo;
@@ -5475,12 +5475,12 @@ public class ConnectivityService extends IConnectivityManager.Stub 
            // Start or stop DNS64 detection and 464xlat according to network state.

            // Start or stop DNS64 detection and 464xlat according to network state.

            networkAgent.clatd.update();

            networkAgent.clatd.update();

            notifyIfacesChangedForNetworkStats();

            notifyIfacesChangedForNetworkStats();

            if (networkAgent.everConnected) {

            try {

            try {

                    networkAgent.networkMonitor().notifyLinkPropertiesChanged();

                networkAgent.networkMonitor().notifyLinkPropertiesChanged(newLp);

            } catch (RemoteException e) {

            } catch (RemoteException e) {

                e.rethrowFromSystemServer();

                e.rethrowFromSystemServer();

            }

            }

            if (networkAgent.everConnected) {

                notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_IP_CHANGED);

                notifyNetworkCallbacks(networkAgent, ConnectivityManager.CALLBACK_IP_CHANGED);

            }

            }

        }

        }
@@ -5708,7 +5708,7 @@ public class ConnectivityService extends IConnectivityManager.Stub 
        final NetworkCapabilities prevNc;

        final NetworkCapabilities prevNc;

        synchronized (nai) {

        synchronized (nai) {

            prevNc = nai.networkCapabilities;

            prevNc = nai.networkCapabilities;

            nai.networkCapabilities = newNc;

            nai.setNetworkCapabilities(newNc);

        }

        }





        updateUids(nai, prevNc, newNc);

        updateUids(nai, prevNc, newNc);
@@ -5723,11 +5723,6 @@ public class ConnectivityService extends IConnectivityManager.Stub 
            // If the requestable capabilities have changed or the score changed, we can't have been

            // If the requestable capabilities have changed or the score changed, we can't have been

            // called by rematchNetworkAndRequests, so it's safe to start a rematch.

            // called by rematchNetworkAndRequests, so it's safe to start a rematch.

            rematchAllNetworksAndRequests(nai, oldScore);

            rematchAllNetworksAndRequests(nai, oldScore);

            try {

                nai.networkMonitor().notifyNetworkCapabilitiesChanged();

            } catch (RemoteException e) {

                e.rethrowFromSystemServer();

            }

            notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED);

            notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED);

        }

        }
@@ -5986,11 +5981,6 @@ public class ConnectivityService extends IConnectivityManager.Stub 
        }

        }





        if (capabilitiesChanged) {

        if (capabilitiesChanged) {

            try {

                nai.networkMonitor().notifyNetworkCapabilitiesChanged();

            } catch (RemoteException e) {

                e.rethrowFromSystemServer();

            }

            notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED);

            notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED);

        }

        }
@@ -6399,7 +6389,8 @@ public class ConnectivityService extends IConnectivityManager.Stub 
                if (networkAgent.networkMisc.acceptPartialConnectivity) {

                if (networkAgent.networkMisc.acceptPartialConnectivity) {

                    networkAgent.networkMonitor().setAcceptPartialConnectivity();

                    networkAgent.networkMonitor().setAcceptPartialConnectivity();

                }

                }

                networkAgent.networkMonitor().notifyNetworkConnected();

                networkAgent.networkMonitor().notifyNetworkConnected(

                        networkAgent.linkProperties, networkAgent.networkCapabilities);

            } catch (RemoteException e) {

            } catch (RemoteException e) {

                e.rethrowFromSystemServer();

                e.rethrowFromSystemServer();

            }

            }