From 283e1ebcd8ad2671565fe8efdd1bcc3020f163e9 Mon Sep 17 00:00:00 2001 From: kari-ts <135075563+kari-ts@users.noreply.github.com> Date: Wed, 11 Sep 2024 13:10:02 -0700 Subject: [PATCH] android: fix network callback race (#493) ConnectivityManager doesn't make guarantees about the order of network updates. Only use network updates for currently active network. Also, use registerDefaultNetworkCallback so that we are only listening for default networks. Updates tailscale/tailscale#13173 Signed-off-by: kari-ts --- .../tailscale/ipn/NetworkChangeCallback.kt | 97 +++++++++++++------ 1 file changed, 69 insertions(+), 28 deletions(-) diff --git a/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt b/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt index 622df91..9ae0194 100644 --- a/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt +++ b/android/src/main/java/com/tailscale/ipn/NetworkChangeCallback.kt @@ -2,48 +2,73 @@ // SPDX-License-Identifier: BSD-3-Clause package com.tailscale.ipn -import android.content.Context import android.net.ConnectivityManager import android.net.LinkProperties import android.net.Network import android.net.NetworkCapabilities -import android.net.NetworkRequest import android.util.Log import libtailscale.Libtailscale import java.net.InetAddress -import java.net.NetworkInterface object NetworkChangeCallback { - // requestNetwork attempts to find the best network that matches the passed NetworkRequest. It is - // possible that this might return an unusuable network, eg a captive portal. + private const val TAG = "NetworkChangeCallback" + + // Cache LinkProperties and NetworkCapabilities since synchronous ConnectivityManager calls are + // prone to races. + // Since there is no guarantee for which update might come first, maybe update DNS configs on + // both. + val networkCapabilitiesCache = mutableMapOf() + val linkPropertiesCache = mutableMapOf() + + // requestDefaultNetworkCallback receives notifications about the default network. Listen for + // changes to the capabilities, which are guaranteed to come after a network becomes available per + // https://developer.android.com/reference/android/net/ConnectivityManager.NetworkCallback#onAvailable(android.net.Network), + // in order to filter on non-VPN networks. fun monitorDnsChanges(connectivityManager: ConnectivityManager, dns: DnsConfig) { - val networkConnectivityRequest = - NetworkRequest.Builder() - .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) - .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) - .build() - - connectivityManager.registerNetworkCallback( - networkConnectivityRequest, + + connectivityManager.registerDefaultNetworkCallback( object : ConnectivityManager.NetworkCallback() { - override fun onAvailable(network: Network) { - super.onAvailable(network) - - val sb = StringBuilder() - val linkProperties: LinkProperties? = connectivityManager.getLinkProperties(network) - val dnsList: MutableList = linkProperties?.dnsServers ?: mutableListOf() - for (ip in dnsList) { - sb.append(ip.hostAddress).append(" ") - } - val searchDomains: String? = linkProperties?.domains - if (searchDomains != null) { - sb.append("\n") - sb.append(searchDomains) + override fun onCapabilitiesChanged(network: Network, capabilities: NetworkCapabilities) { + super.onCapabilitiesChanged(network, capabilities) + networkCapabilitiesCache[network] = capabilities + val linkProperties = linkPropertiesCache[network] + if (linkProperties != null && + capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) && + capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) { + maybeUpdateDNSConfig(linkProperties, dns) + } else { + if (!capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) || + !capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) { + Log.d( + TAG, + "Capabilities changed for network $network.toString(), but not updating DNS config because either this is a VPN network or non-internet network") + } else { + Log.d( + TAG, + "Capabilities changed for network $network.toString(), but not updating DNS config, because the LinkProperties hasn't been gotten yet") + } } + } - if (dns.updateDNSFromNetwork(sb.toString())) { - Libtailscale.onDNSConfigChanged(linkProperties?.interfaceName) + override fun onLinkPropertiesChanged(network: Network, linkProperties: LinkProperties) { + super.onLinkPropertiesChanged(network, linkProperties) + linkPropertiesCache[network] = linkProperties + val capabilities = networkCapabilitiesCache[network] + if (capabilities != null && + capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN) && + capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) { + maybeUpdateDNSConfig(linkProperties, dns) + } else { + if (capabilities == null) { + Log.d( + TAG, + "Capabilities changed for network $network.toString(), but not updating DNS config because capabilities haven't been gotten for this network yet") + } else { + Log.d( + TAG, + "Capabilities changed for network $network.toString(), but not updating DNS config, because this is a VPN network or non-Internet network") + } } } @@ -55,4 +80,20 @@ object NetworkChangeCallback { } }) } + + fun maybeUpdateDNSConfig(linkProperties: LinkProperties, dns: DnsConfig) { + val sb = StringBuilder() + val dnsList: MutableList = linkProperties.dnsServers ?: mutableListOf() + for (ip in dnsList) { + sb.append(ip.hostAddress).append(" ") + } + val searchDomains: String? = linkProperties.domains + if (searchDomains != null) { + sb.append("\n") + sb.append(searchDomains) + } + if (dns.updateDNSFromNetwork(sb.toString())) { + Libtailscale.onDNSConfigChanged(linkProperties.interfaceName) + } + } }