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 <kari@tailscale.com>

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<Network, NetworkCapabilities>()
+  val linkPropertiesCache = mutableMapOf<Network, LinkProperties>()
+
+  // 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<InetAddress> = 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<InetAddress> = 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)
+    }
+  }
 }