From dc6e640623cf1fe5290ee9ce1cf7d0ecae2cbeb4 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Wed, 27 Mar 2019 15:42:53 +0900 Subject: 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 --- src/com/android/server/NetworkStackService.java | 14 ++-- .../server/connectivity/NetworkMonitor.java | 82 +++++++++++----------- 2 files changed, 48 insertions(+), 48 deletions(-) (limited to 'src') diff --git a/src/com/android/server/NetworkStackService.java b/src/com/android/server/NetworkStackService.java index 19e9108..63f057c 100644 --- a/src/com/android/server/NetworkStackService.java +++ b/src/com/android/server/NetworkStackService.java @@ -35,7 +35,9 @@ import android.net.INetd; import android.net.INetworkMonitor; import android.net.INetworkMonitorCallbacks; import android.net.INetworkStackConnector; +import android.net.LinkProperties; import android.net.Network; +import android.net.NetworkCapabilities; import android.net.PrivateDnsConfigParcel; import android.net.dhcp.DhcpServer; import android.net.dhcp.DhcpServingParams; @@ -307,9 +309,9 @@ public class NetworkStackService extends Service { } @Override - public void notifyNetworkConnected() { + public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) { checkNetworkStackCallingPermission(); - mNm.notifyNetworkConnected(); + mNm.notifyNetworkConnected(lp, nc); } @Override @@ -319,15 +321,15 @@ public class NetworkStackService extends Service { } @Override - public void notifyLinkPropertiesChanged() { + public void notifyLinkPropertiesChanged(LinkProperties lp) { checkNetworkStackCallingPermission(); - mNm.notifyLinkPropertiesChanged(); + mNm.notifyLinkPropertiesChanged(lp); } @Override - public void notifyNetworkCapabilitiesChanged() { + public void notifyNetworkCapabilitiesChanged(NetworkCapabilities nc) { checkNetworkStackCallingPermission(); - mNm.notifyNetworkCapabilitiesChanged(); + mNm.notifyNetworkCapabilitiesChanged(nc); } } } diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java index f3476ed..c000fc6 100644 --- a/src/com/android/server/connectivity/NetworkMonitor.java +++ b/src/com/android/server/connectivity/NetworkMonitor.java @@ -78,6 +78,7 @@ import android.telephony.SignalStrength; import android.telephony.TelephonyManager; import android.text.TextUtils; import android.util.Log; +import android.util.Pair; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.RingBufferIndices; @@ -221,19 +222,31 @@ public class NetworkMonitor extends StateMachine { * Message to self indicating captive portal detection is completed. * 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. * 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 * 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. private static final int INITIAL_REEVALUATE_DELAY_MS = 1000; @@ -379,10 +392,8 @@ public class NetworkMonitor extends StateMachine { mDataStallValidDnsTimeThreshold = getDataStallValidDnsTimeThreshold(); mDataStallEvaluationType = getDataStallEvalutionType(); - // mLinkProperties and mNetworkCapbilities must never be null or we will NPE. - // Provide empty objects in case we are started and the network disconnects before - // we can ever fetch them. - // TODO: Delete ASAP. + // Provide empty LinkProperties and NetworkCapabilities to make sure they are never null, + // even before notifyNetworkConnected. mLinkProperties = new LinkProperties(); 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. */ - public void notifyNetworkConnected() { - sendMessage(CMD_NETWORK_CONNECTED); + public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) { + sendMessage(CMD_NETWORK_CONNECTED, new Pair<>( + new LinkProperties(lp), new NetworkCapabilities(nc))); + } + + private void updateConnectedNetworkAttributes(Message connectedMsg) { + final Pair attrs = + (Pair) 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. */ - public void notifyLinkPropertiesChanged() { - getHandler().post(() -> { - 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; - } + public void notifyLinkPropertiesChanged(final LinkProperties lp) { + sendMessage(EVENT_LINK_PROPERTIES_CHANGED, new LinkProperties(lp)); } /** * Send a notification to NetworkMonitor indicating that network capabilities have changed. */ - public void notifyNetworkCapabilitiesChanged() { - getHandler().post(() -> { - 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; - } + public void notifyNetworkCapabilitiesChanged(final NetworkCapabilities nc) { + sendMessage(EVENT_NETWORK_CAPABILITIES_CHANGED, new NetworkCapabilities(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 // does not entail any real state (hence no enter() or exit() routines). private class DefaultState extends State { - @Override - public void enter() { - // TODO: have those passed parceled in start() and remove this - updateLinkProperties(); - updateNetworkCapabilities(); - } - @Override public boolean processMessage(Message message) { switch (message.what) { case CMD_NETWORK_CONNECTED: + updateConnectedNetworkAttributes(message); logNetworkEvent(NetworkEvent.NETWORK_CONNECTED); transitionTo(mEvaluatingState); return HANDLED; @@ -660,6 +651,12 @@ public class NetworkMonitor extends StateMachine { case EVENT_ACCEPT_PARTIAL_CONNECTIVITY: mAcceptPartialConnectivity = true; break; + case EVENT_LINK_PROPERTIES_CHANGED: + mLinkProperties = (LinkProperties) message.obj; + break; + case EVENT_NETWORK_CAPABILITIES_CHANGED: + mNetworkCapabilities = (NetworkCapabilities) message.obj; + break; default: break; } @@ -684,6 +681,7 @@ public class NetworkMonitor extends StateMachine { public boolean processMessage(Message message) { switch (message.what) { case CMD_NETWORK_CONNECTED: + updateConnectedNetworkAttributes(message); transitionTo(mValidatedState); break; case CMD_EVALUATE_PRIVATE_DNS: -- cgit v1.2.3