Replace strict mode with fallback upstream interface

Fixes #40. Apparently we can no longer take advantage of default network rules set by Android system since Android 9.0 thanks to this commit: 758627c4d9

mobile/src/main/java/be/mygod/vpnhotspot/net/IpNeighbour.kt

@@ -1,4 +1,4 @@
-package be.mygod.vpnhotspot.net.monitor
+package be.mygod.vpnhotspot.net
 
 import be.mygod.vpnhotspot.util.parseNumericAddressNoThrow
 import timber.log.Timber

mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt

@@ -1,22 +1,15 @@
 package be.mygod.vpnhotspot.net
 
-import android.content.ComponentName
-import android.content.Context
-import android.content.Intent
-import android.content.ServiceConnection
 import android.os.Build
-import android.os.IBinder
 import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.R
-import be.mygod.vpnhotspot.client.Client
-import be.mygod.vpnhotspot.client.ClientMonitorService
-import be.mygod.vpnhotspot.debugLog
+import be.mygod.vpnhotspot.net.monitor.FallbackUpstreamMonitor
 import be.mygod.vpnhotspot.net.monitor.TrafficRecorder
+import be.mygod.vpnhotspot.net.monitor.UpstreamMonitor
 import be.mygod.vpnhotspot.util.RootSession
-import be.mygod.vpnhotspot.util.computeIfAbsentCompat
-import be.mygod.vpnhotspot.util.stopAndUnbind
 import be.mygod.vpnhotspot.widget.SmartSnackbar
 import timber.log.Timber
+import java.lang.RuntimeException
 import java.net.*
 
 /**
@@ -24,9 +17,17 @@ import java.net.*
  *
  * Once revert is called, this object no longer serves any purpose.
  */
-class Routing(private val owner: Context, val upstream: String?, private val downstream: String,
-              ownerAddress: InterfaceAddress? = null, private val strict: Boolean = true) : ServiceConnection {
+class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
     companion object {
+        /**
+         * Since Android 5.0, RULE_PRIORITY_TETHERING = 18000.
+         * This also works for Wi-Fi direct where there's no rule at 18000.
+         *
+         * Source: https://android.googlesource.com/platform/system/netd/+/b9baf26/server/RouteController.cpp#65
+         */
+        private const val RULE_PRIORITY_UPSTREAM = 17800
+        private const val RULE_PRIORITY_UPSTREAM_FALLBACK = 17900
+
         /**
          * -w <seconds> is not supported on 7.1-.
          * Fortunately there also isn't a time limit for starting a foreground service back in 7.1-.
@@ -45,7 +46,8 @@ class Routing(private val owner: Context, val upstream: String?, private val dow
                 it.submit("while $IPTABLES -t nat -D POSTROUTING -j vpnhotspot_masquerade; do done")
                 it.submit("$IPTABLES -t nat -F vpnhotspot_masquerade")
                 it.submit("$IPTABLES -t nat -X vpnhotspot_masquerade")
-                it.submit("while ip rule del priority 17900; do done")
+                it.submit("while ip rule del priority $RULE_PRIORITY_UPSTREAM; do done")
+                it.submit("while ip rule del priority $RULE_PRIORITY_UPSTREAM_FALLBACK; do done")
                 it.submit("while ip rule del iif lo uidrange 0-0 lookup local_network priority 11000; do done")
             }
         }
@@ -62,56 +64,107 @@ class Routing(private val owner: Context, val upstream: String?, private val dow
 
     val hostAddress = ownerAddress ?: NetworkInterface.getByName(downstream)?.interfaceAddresses?.asSequence()
             ?.singleOrNull { it.address is Inet4Address } ?: throw InterfaceNotFoundException()
+    val hostSubnet = "${hostAddress.address.hostAddress}/${hostAddress.networkPrefixLength}"
     private val transaction = RootSession.beginTransaction()
-    private val subroutes = HashMap<InetAddress, Subroute>()
-    private var clients: ClientMonitorService.Binder? = null
+
+    var hasMasquerade = false
+
+    private abstract inner class Upstream : UpstreamMonitor.Callback {
+        var subrouting: Subrouting? = null
+        var dns: List<InetAddress> = emptyList()
+
+        override fun onAvailable(ifname: String, dns: List<InetAddress>) {
+            this.dns = dns
+            updateDnsRoute()
+        }
+
+        override fun onLost() {
+            val subrouting = subrouting ?: return
+            subrouting.close()
+            TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
+            subrouting.revert()
+            this.subrouting = null
+        }
+    }
+    private val fallbackUpstream = object : Upstream() {
+        override fun onAvailable(ifname: String, dns: List<InetAddress>) {
+            val subrouting = subrouting
+            if (subrouting == null) this.subrouting = try {
+                Subrouting(this@Routing, RULE_PRIORITY_UPSTREAM_FALLBACK, ifname)
+            } catch (e: Exception) {
+                SmartSnackbar.make(e.localizedMessage).show()
+                Timber.w(e)
+                null
+            } else check(subrouting.upstream == ifname)
+            super.onAvailable(ifname, dns)
+        }
+
+        override fun onFallback() {
+            check(subrouting == null)
+            subrouting = try {
+                Subrouting(this@Routing, RULE_PRIORITY_UPSTREAM_FALLBACK)
+            } catch (e: Exception) {
+                SmartSnackbar.make(e.localizedMessage).show()
+                Timber.w(e)
+                null
+            }
+            updateDnsRoute()
+        }
+    }
+    private val upstream = object : Upstream() {
+        override fun onAvailable(ifname: String, dns: List<InetAddress>) {
+            val subrouting = subrouting
+            if (subrouting == null) this.subrouting = try {
+                Subrouting(this@Routing, RULE_PRIORITY_UPSTREAM, ifname)
+            } catch (e: Exception) {
+                SmartSnackbar.make(e.localizedMessage).show()
+                Timber.w(e)
+                null
+            } else check(subrouting.upstream == ifname)
+            super.onAvailable(ifname, dns)
+        }
+    }
 
     fun ipForward() = transaction.exec("echo 1 >/proc/sys/net/ipv4/ip_forward")
 
     fun disableIpv6() = transaction.exec("echo 1 >/proc/sys/net/ipv6/conf/$downstream/disable_ipv6",
             "echo 0 >/proc/sys/net/ipv6/conf/$downstream/disable_ipv6")
 
-    /**
-     * Since Android 5.0, RULE_PRIORITY_TETHERING = 18000.
-     * This also works for Wi-Fi direct where there's no rule at 18000.
-     *
-     * Source: https://android.googlesource.com/platform/system/netd/+/b9baf26/server/RouteController.cpp#65
-     */
-    fun rule() {
-        if (upstream != null) {
-            transaction.exec("ip rule add from all iif $downstream lookup $upstream priority 17900",
-                    // by the time stopScript is called, table entry for upstream may already get removed
-                    "ip rule del from all iif $downstream priority 17900")
-        }
-    }
-
     fun forward() {
         transaction.execQuiet("$IPTABLES -N vpnhotspot_fwd")
         transaction.iptablesInsert("FORWARD -j vpnhotspot_fwd")
         transaction.iptablesAdd("vpnhotspot_fwd -i $downstream ! -o $downstream -j DROP")   // ensure blocking works
-        // the real forwarding filters will be added in Subroute when clients are connected
+        // the real forwarding filters will be added in Subrouting when clients are connected
     }
 
     fun masquerade() {
-        val hostSubnet = "${hostAddress.address.hostAddress}/${hostAddress.networkPrefixLength}"
         transaction.execQuiet("$IPTABLES -t nat -N vpnhotspot_masquerade")
         transaction.iptablesInsert("POSTROUTING -j vpnhotspot_masquerade", "nat")
-        // note: specifying -i wouldn't work for POSTROUTING
-        if (strict) {
-            if (upstream != null) {
-                transaction.iptablesAdd("vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE", "nat")
-            }   // else nothing needs to be done
-        } else {
-            transaction.iptablesAdd("vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE", "nat")
-        }
+        hasMasquerade = true
+        // further rules are added when upstreams are found
     }
 
-    fun dnsRedirect(dnses: List<InetAddress>) {
-        val hostAddress = hostAddress.address.hostAddress
-        val dns = dnses.firstOrNull { it is Inet4Address }?.hostAddress ?: app.pref.getString("service.dns", "8.8.8.8")
-        debugLog("Routing", "Using $dns from ($dnses)")
-        transaction.iptablesAdd("PREROUTING -i $downstream -p tcp -d $hostAddress --dport 53 -j DNAT --to-destination $dns", "nat")
-        transaction.iptablesAdd("PREROUTING -i $downstream -p udp -d $hostAddress --dport 53 -j DNAT --to-destination $dns", "nat")
+    private inner class DnsRoute(val dns: String) {
+        val transaction = RootSession.beginTransaction().safeguard {
+            val hostAddress = hostAddress.address.hostAddress
+            iptablesAdd("PREROUTING -i $downstream -p tcp -d $hostAddress --dport 53 -j DNAT --to-destination $dns", "nat")
+            iptablesAdd("PREROUTING -i $downstream -p udp -d $hostAddress --dport 53 -j DNAT --to-destination $dns", "nat")
+        }
+    }
+    private var currentDns: DnsRoute? = null
+    private fun updateDnsRoute() {
+        val dns = (upstream.dns + fallbackUpstream.dns).firstOrNull { it is Inet4Address }?.hostAddress
+                ?: app.pref.getString("service.dns", "8.8.8.8")
+        if (dns != currentDns?.dns) {
+            currentDns?.transaction?.revert()
+            currentDns = try {
+                DnsRoute(dns)
+            } catch (e: RuntimeException) {
+                Timber.w(e)
+                SmartSnackbar.make(e.localizedMessage).show()
+                null
+            }
+        }
     }
 
     /**
@@ -125,80 +178,23 @@ class Routing(private val owner: Context, val upstream: String?, private val dow
     fun dhcpWorkaround() = transaction.exec("ip rule add iif lo uidrange 0-0 lookup local_network priority 11000",
             "ip rule del iif lo uidrange 0-0 lookup local_network priority 11000")
 
+    fun stop() {
+        FallbackUpstreamMonitor.unregisterCallback(fallbackUpstream)
+        fallbackUpstream.subrouting?.close()
+        UpstreamMonitor.unregisterCallback(upstream)
+        upstream.subrouting?.close()
+    }
+
     fun commit() {
         transaction.commit()
-        owner.bindService(Intent(owner, ClientMonitorService::class.java), this, Context.BIND_AUTO_CREATE)
+        FallbackUpstreamMonitor.registerCallback(fallbackUpstream)
+        UpstreamMonitor.registerCallback(upstream)
     }
     fun revert() {
         stop()
         TrafficRecorder.update()    // record stats before exiting to prevent stats losing
-        synchronized(subroutes) { subroutes.forEach { (_, subroute) -> subroute.close() } }
+        fallbackUpstream.subrouting?.revert()
+        upstream.subrouting?.revert()
         transaction.revert()
     }
-
-    /**
-     * Only unregister client listener. This should only be used when a clean has just performed.
-     */
-    fun stop() = owner.stopAndUnbind(this)
-
-    override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
-        clients = service as ClientMonitorService.Binder
-        service.clientsChanged[this] = {
-            synchronized(subroutes) {
-                val toRemove = HashSet(subroutes.keys)
-                for (client in it) if (!client.record.blocked) updateForClient(client, toRemove)
-                if (toRemove.isNotEmpty()) {
-                    TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
-                    for (address in toRemove) subroutes.remove(address)!!.close()
-                }
-            }
-        }
-    }
-    override fun onServiceDisconnected(name: ComponentName?) {
-        val clients = clients ?: return
-        clients.clientsChanged -= this
-        this.clients = null
-    }
-
-    private fun updateForClient(client: Client, toRemove: HashSet<InetAddress>? = null) {
-        for ((ip, _) in client.ip) if (ip is Inet4Address) {
-            toRemove?.remove(ip)
-            try {
-                subroutes.computeIfAbsentCompat(ip) { Subroute(ip, client) }
-            } catch (e: Exception) {
-                Timber.w(e)
-                SmartSnackbar.make(e.localizedMessage).show()
-            }
-        }
-    }
-
-    private inner class Subroute(private val ip: Inet4Address, client: Client) : AutoCloseable {
-        private val transaction = RootSession.beginTransaction().apply {
-            try {
-                val address by lazy { ip.hostAddress }
-                if (!strict) {
-                    // otw allow downstream packets to be redirected to anywhere
-                    // because we don't wanna keep track of default network changes
-                    iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -j ACCEPT")
-                    iptablesInsert("vpnhotspot_fwd -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
-                } else if (upstream != null) {
-                    iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -o $upstream -j ACCEPT")
-                    iptablesInsert("vpnhotspot_fwd -i $upstream -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
-                }   // else nothing needs to be done
-                commit()
-            } catch (e: Exception) {
-                revert()
-                throw e
-            }
-        }
-
-        init {
-            TrafficRecorder.register(ip, if (strict) upstream else null, downstream, client.mac)
-        }
-
-        override fun close() {
-            TrafficRecorder.unregister(ip, downstream)
-            transaction.revert()
-        }
-    }
 }

mobile/src/main/java/be/mygod/vpnhotspot/net/Subrouting.kt

@@ -0,0 +1,97 @@
+package be.mygod.vpnhotspot.net
+
+import be.mygod.vpnhotspot.net.Routing.Companion.iptablesAdd
+import be.mygod.vpnhotspot.net.Routing.Companion.iptablesInsert
+import be.mygod.vpnhotspot.net.monitor.IpNeighbourMonitor
+import be.mygod.vpnhotspot.net.monitor.TrafficRecorder
+import be.mygod.vpnhotspot.room.AppDatabase
+import be.mygod.vpnhotspot.room.lookup
+import be.mygod.vpnhotspot.room.macToLong
+import be.mygod.vpnhotspot.util.RootSession
+import be.mygod.vpnhotspot.util.computeIfAbsentCompat
+import be.mygod.vpnhotspot.widget.SmartSnackbar
+import timber.log.Timber
+import java.net.Inet4Address
+import java.net.InetAddress
+
+/**
+ * The only case when upstream is null is on API 23- and we are using system default rules.
+ */
+class Subrouting(private val parent: Routing, priority: Int, val upstream: String? = null) :
+        IpNeighbourMonitor.Callback, AutoCloseable {
+    private inner class Subroute(private val ip: Inet4Address, mac: String) : AutoCloseable {
+        private val transaction = RootSession.beginTransaction().safeguard {
+            val downstream = parent.downstream
+            val address = ip.hostAddress
+            if (upstream == null) {
+                // otw allow downstream packets to be redirected to anywhere
+                // because we don't wanna keep track of default network changes
+                iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -j ACCEPT")
+                iptablesInsert("vpnhotspot_fwd -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
+            } else {
+                iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -o $upstream -j ACCEPT")
+                iptablesInsert("vpnhotspot_fwd -i $upstream -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
+            }
+        }
+
+        init {
+            TrafficRecorder.register(ip, upstream, parent.downstream, mac)
+        }
+
+        override fun close() {
+            TrafficRecorder.unregister(ip, upstream, parent.downstream)
+            transaction.revert()
+        }
+    }
+
+    private val transaction = RootSession.beginTransaction().safeguard {
+        if (upstream != null) {
+            val downstream = parent.downstream
+            exec("ip rule add from all iif $downstream lookup $upstream priority $priority",
+                    // by the time stopScript is called, table entry for upstream may already get removed
+                    "ip rule del from all iif $downstream priority $priority")
+        }
+        // note: specifying -i wouldn't work for POSTROUTING
+        if (parent.hasMasquerade) {
+            val hostSubnet = parent.hostSubnet
+            iptablesAdd(if (upstream == null) "vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE" else
+                "vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE", "nat")
+        }
+    }
+    private val subroutes = HashMap<InetAddress, Subroute>()
+
+    init {
+        IpNeighbourMonitor.registerCallback(this)
+    }
+
+    /**
+     * Unregister client listener. This should be always called even after clean.
+     */
+    override fun close() = IpNeighbourMonitor.unregisterCallback(this)
+
+    override fun onIpNeighbourAvailable(neighbours: List<IpNeighbour>) {
+        synchronized(parent) {
+            val toRemove = HashSet(subroutes.keys)
+            for (neighbour in neighbours) {
+                if (neighbour.dev != parent.downstream || neighbour.ip !is Inet4Address ||
+                        AppDatabase.instance.clientRecordDao.lookup(neighbour.lladdr.macToLong()).blocked) continue
+                toRemove.remove(neighbour.ip)
+                try {
+                    subroutes.computeIfAbsentCompat(neighbour.ip) { Subroute(neighbour.ip, neighbour.lladdr) }
+                } catch (e: Exception) {
+                    Timber.w(e)
+                    SmartSnackbar.make(e.localizedMessage).show()
+                }
+            }
+            if (toRemove.isNotEmpty()) {
+                TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
+                for (address in toRemove) subroutes.remove(address)!!.close()
+            }
+        }
+    }
+
+    fun revert() {
+        subroutes.forEach { (_, subroute) -> subroute.close() }
+        transaction.revert()
+    }
+}

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/DefaultNetworkMonitor.kt

@@ -0,0 +1,86 @@
+package be.mygod.vpnhotspot.net.monitor
+
+import android.annotation.TargetApi
+import android.net.*
+import android.os.Build
+import be.mygod.vpnhotspot.App.Companion.app
+import timber.log.Timber
+
+object DefaultNetworkMonitor : UpstreamMonitor() {
+    private var registered = false
+    private var currentNetwork: Network? = null
+    override var currentLinkProperties: LinkProperties? = null
+        private set
+    /**
+     * Unfortunately registerDefaultNetworkCallback is going to return VPN interface since Android P DP1:
+     * https://android.googlesource.com/platform/frameworks/base/+/dda156ab0c5d66ad82bdcf76cda07cbc0a9c8a2e
+     */
+    private val networkRequest = NetworkRequest.Builder()
+            .addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
+            .addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED)
+            .build()
+    private val networkCallback = object : ConnectivityManager.NetworkCallback() {
+        override fun onAvailable(network: Network) {
+            val properties = app.connectivity.getLinkProperties(network)
+            val ifname = properties?.interfaceName ?: return
+            when (currentNetwork) {
+                null -> currentNetwork = network
+                network -> {
+                    val oldProperties = currentLinkProperties!!
+                    check(ifname == oldProperties.interfaceName)
+                    if (properties.dnsServers == oldProperties.dnsServers) return
+                }
+                else -> check(false)
+            }
+            currentLinkProperties = properties
+            callbacks.forEach { it.onAvailable(ifname, properties.dnsServers) }
+        }
+
+        override fun onLinkPropertiesChanged(network: Network, properties: LinkProperties) {
+            check(currentNetwork == network)
+            val oldProperties = currentLinkProperties!!
+            currentLinkProperties = properties
+            val ifname = properties.interfaceName!!
+            check(ifname == oldProperties.interfaceName)
+            if (properties.dnsServers != oldProperties.dnsServers)
+                callbacks.forEach { it.onAvailable(ifname, properties.dnsServers) }
+        }
+
+        override fun onLost(network: Network) {
+            check(currentNetwork == network)
+            callbacks.forEach { it.onLost() }
+            currentNetwork = null
+            currentLinkProperties = null
+        }
+    }
+
+    override fun registerCallbackLocked(callback: Callback) {
+        if (registered) {
+            val currentLinkProperties = currentLinkProperties
+            if (currentLinkProperties != null) {
+                callback.onAvailable(currentLinkProperties.interfaceName!!, currentLinkProperties.dnsServers)
+            }
+        } else {
+            if (Build.VERSION.SDK_INT in 24..27) @TargetApi(24) {
+                app.connectivity.registerDefaultNetworkCallback(networkCallback)
+            } else try {
+                app.connectivity.requestNetwork(networkRequest, networkCallback)
+            } catch (e: SecurityException) {
+                if (Build.VERSION.SDK_INT != 23) throw e
+                // SecurityException would be thrown in requestNetwork on Android 6.0 thanks to Google's stupid bug
+                Timber.w(e)
+                callback.onFallback()
+                return
+            }
+            registered = true
+        }
+    }
+
+    override fun destroyLocked() {
+        if (!registered) return
+        app.connectivity.unregisterNetworkCallback(networkCallback)
+        registered = false
+        currentNetwork = null
+        currentLinkProperties = null
+    }
+}

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/FallbackUpstreamMonitor.kt

@@ -0,0 +1,42 @@
+package be.mygod.vpnhotspot.net.monitor
+
+import android.content.SharedPreferences
+import be.mygod.vpnhotspot.App.Companion.app
+
+abstract class FallbackUpstreamMonitor private constructor() : UpstreamMonitor() {
+    companion object : SharedPreferences.OnSharedPreferenceChangeListener {
+        const val KEY = "service.upstream.fallback"
+
+        init {
+            app.pref.registerOnSharedPreferenceChangeListener(this)
+        }
+
+        private fun generateMonitor(): UpstreamMonitor {
+            val upstream = app.pref.getString(KEY, null)
+            return if (upstream.isNullOrEmpty()) DefaultNetworkMonitor else InterfaceMonitor(upstream!!)
+        }
+        private var monitor = generateMonitor()
+
+        fun registerCallback(callback: Callback) = synchronized(this) { monitor.registerCallback(callback) }
+        fun unregisterCallback(callback: Callback) = synchronized(this) { monitor.unregisterCallback(callback) }
+
+        override fun onSharedPreferenceChanged(sharedPreferences: SharedPreferences?, key: String?) {
+            if (key == KEY) synchronized(this) {
+                val old = monitor
+                val (active, callbacks) = synchronized(old) {
+                    val active = old.currentIface != null
+                    val callbacks = old.callbacks.toList()
+                    old.callbacks.clear()
+                    old.destroyLocked()
+                    Pair(active, callbacks)
+                }
+                val new = generateMonitor()
+                monitor = new
+                for (callback in callbacks) {
+                    if (active) callback.onLost()
+                    new.registerCallback(callback)
+                }
+            }
+        }
+    }
+}

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/InterfaceMonitor.kt

@@ -23,42 +23,30 @@ class InterfaceMonitor(val iface: String) : UpstreamMonitor() {
                 setPresent(lines.any { parser.find(it)?.groupValues?.get(2) == iface })
     }
 
-    private fun setPresent(present: Boolean) = if (initializing) {
-        initializedPresent = present
-        currentIface = if (present) iface else null
-    } else synchronized(this) {
+    private fun setPresent(present: Boolean) = synchronized(this) {
         val old = currentIface != null
         if (present == old) return
         currentIface = if (present) iface else null
         if (present) {
-            val dns = dns
+            val dns = currentDns
             callbacks.forEach { it.onAvailable(iface, dns) }
         } else callbacks.forEach { it.onLost() }
     }
 
     private var monitor: IpLinkMonitor? = null
-    private var initializing = false
-    private var initializedPresent: Boolean? = null
     override var currentIface: String? = null
         private set
-    private val dns get() = app.connectivity.allNetworks
+    override val currentLinkProperties get() = app.connectivity.allNetworks
             .map { app.connectivity.getLinkProperties(it) }
             .singleOrNull { it?.interfaceName == iface }
-            ?.dnsServers ?: emptyList()
 
-    override fun registerCallbackLocked(callback: Callback): Boolean {
+    override fun registerCallbackLocked(callback: Callback) {
         var monitor = monitor
-        val present = if (monitor == null) {
-            initializing = true
-            initializedPresent = null
+        if (monitor == null) {
             monitor = IpLinkMonitor()
             this.monitor = monitor
             monitor.run()
-            initializing = false
-            initializedPresent!!
-        } else currentIface != null
-        if (present) callback.onAvailable(iface, dns)
-        return !present
+        } else if (currentIface != null) callback.onAvailable(iface, currentDns)
     }
 
     override fun destroyLocked() {

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/IpNeighbourMonitor.kt

@@ -2,6 +2,7 @@ package be.mygod.vpnhotspot.net.monitor
 
 import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.debugLog
+import be.mygod.vpnhotspot.net.IpNeighbour
 import java.net.InetAddress
 
 class IpNeighbourMonitor private constructor() : IpMonitor() {

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/TrafficRecorder.kt

@@ -9,6 +9,7 @@ import be.mygod.vpnhotspot.room.macToLong
 import be.mygod.vpnhotspot.util.Event2
 import be.mygod.vpnhotspot.util.RootSession
 import be.mygod.vpnhotspot.util.parseNumericAddress
+import be.mygod.vpnhotspot.widget.SmartSnackbar
 import timber.log.Timber
 import java.net.InetAddress
 import java.util.concurrent.TimeUnit
@@ -17,7 +18,8 @@ object TrafficRecorder {
     private const val ANYWHERE = "0.0.0.0/0"
 
     private var scheduled = false
-    private val records = HashMap<Pair<InetAddress, String>, TrafficRecord>()
+    private var lastUpdate = 0L
+    private val records = HashMap<Triple<InetAddress, String?, String>, TrafficRecord>()
     val foregroundListeners = Event2<Collection<TrafficRecord>, LongSparseArray<TrafficRecord>>()
 
     fun register(ip: InetAddress, upstream: String?, downstream: String, mac: String) {
@@ -28,13 +30,13 @@ object TrafficRecorder {
                 downstream = downstream)
         AppDatabase.instance.trafficRecordDao.insert(record)
         synchronized(this) {
-            check(records.put(Pair(ip, downstream), record) == null)
+            check(records.put(Triple(ip, upstream, downstream), record) == null)
             scheduleUpdateLocked()
         }
     }
-    fun unregister(ip: InetAddress, downstream: String) = synchronized(this) {
+    fun unregister(ip: InetAddress, upstream: String?, downstream: String) = synchronized(this) {
         update()    // flush stats before removing
-        check(records.remove(Pair(ip, downstream)) != null)
+        check(records.remove(Triple(ip, upstream, downstream)) != null)
     }
 
     private fun unscheduleUpdateLocked() {
@@ -56,72 +58,85 @@ object TrafficRecorder {
         scheduleUpdateLocked()
     }
 
+    private fun doUpdate(timestamp: Long) {
+        val oldRecords = LongSparseArray<TrafficRecord>()
+        for (line in RootSession.use { it.execOutUnjoined("iptables -nvx -L vpnhotspot_fwd") }.asSequence().drop(2)) {
+            val columns = line.split("\\s+".toRegex()).filter { it.isNotEmpty() }
+            try {
+                check(columns.size >= 9)
+                when (columns[2]) {
+                    "DROP" -> { }
+                    "ACCEPT" -> {
+                        val isReceive = columns[7] == ANYWHERE
+                        val isSend = columns[8] == ANYWHERE
+                        check(isReceive != isSend)
+                        val ip = parseNumericAddress(columns[if (isReceive) 8 else 7])
+                        val downstream = columns[if (isReceive) 6 else 5]
+                        var upstream: String? = columns[if (isReceive) 5 else 6]
+                        if (upstream == "*") upstream = null
+                        val key = Triple(ip, upstream, downstream)
+                        val oldRecord = records[key]!!
+                        val record = if (oldRecord.id == null) oldRecord else TrafficRecord(
+                                timestamp = timestamp,
+                                mac = oldRecord.mac,
+                                ip = ip,
+                                upstream = upstream,
+                                downstream = downstream,
+                                sentPackets = -1,
+                                sentBytes = -1,
+                                receivedPackets = -1,
+                                receivedBytes = -1,
+                                previousId = oldRecord.id)
+                        if (isReceive) {
+                            if (record.receivedPackets == -1L && record.receivedBytes == -1L) {
+                                record.receivedPackets = columns[0].toLong()
+                                record.receivedBytes = columns[1].toLong()
+                            }
+                        } else {
+                            if (record.sentPackets == -1L && record.sentBytes == -1L) {
+                                record.sentPackets = columns[0].toLong()
+                                record.sentBytes = columns[1].toLong()
+                            }
+                        }
+                        if (oldRecord.id != null) {
+                            check(records.put(key, record) == oldRecord)
+                            oldRecords.put(oldRecord.id!!, oldRecord)
+                        }
+                    }
+                    else -> check(false)
+                }
+            } catch (e: RuntimeException) {
+                Timber.w(line)
+                Timber.w(e)
+            }
+        }
+        for ((_, record) in records) if (record.id == null) {
+            check(record.sentPackets >= 0)
+            check(record.sentBytes >= 0)
+            check(record.receivedPackets >= 0)
+            check(record.receivedBytes >= 0)
+            AppDatabase.instance.trafficRecordDao.insert(record)
+        }
+        foregroundListeners(records.values, oldRecords)
+    }
     fun update() {
         synchronized(this) {
+            val wasScheduled = scheduled
             scheduled = false
             if (records.isEmpty()) return
             val timestamp = System.currentTimeMillis()
-            val oldRecords = LongSparseArray<TrafficRecord>()
-            for (line in RootSession.use { it.execOutUnjoined("iptables -nvx -L vpnhotspot_fwd") }
-                    .asSequence().drop(2)) {
-                val columns = line.split("\\s+".toRegex()).filter { it.isNotEmpty() }
+            if (timestamp - lastUpdate > 100) {
                 try {
-                    check(columns.size >= 9)
-                    when (columns[2]) {
-                        "DROP" -> { }
-                        "ACCEPT" -> {
-                            val isReceive = columns[7] == ANYWHERE
-                            val isSend = columns[8] == ANYWHERE
-                            check(isReceive != isSend)
-                            val ip = parseNumericAddress(columns[if (isReceive) 8 else 7])
-                            val downstream = columns[if (isReceive) 6 else 5]
-                            var upstream: String? = columns[if (isReceive) 5 else 6]
-                            if (upstream == "*") upstream = null
-                            val key = Pair(ip, downstream)
-                            val oldRecord = records[key]!!
-                            check(upstream == oldRecord.upstream)
-                            val record = if (oldRecord.id == null) oldRecord else TrafficRecord(
-                                    timestamp = timestamp,
-                                    mac = oldRecord.mac,
-                                    ip = ip,
-                                    upstream = upstream,
-                                    downstream = downstream,
-                                    sentPackets = -1,
-                                    sentBytes = -1,
-                                    receivedPackets = -1,
-                                    receivedBytes = -1,
-                                    previousId = oldRecord.id)
-                            if (isReceive) {
-                                if (record.receivedPackets == -1L && record.receivedBytes == -1L) {
-                                    record.receivedPackets = columns[0].toLong()
-                                    record.receivedBytes = columns[1].toLong()
-                                }
-                            } else {
-                                if (record.sentPackets == -1L && record.sentBytes == -1L) {
-                                    record.sentPackets = columns[0].toLong()
-                                    record.sentBytes = columns[1].toLong()
-                                }
-                            }
-                            if (oldRecord.id != null) {
-                                check(records.put(key, record) == oldRecord)
-                                oldRecords.put(oldRecord.id!!, oldRecord)
-                            }
-                        }
-                        else -> check(false)
-                    }
+                    doUpdate(timestamp)
                 } catch (e: RuntimeException) {
-                    Timber.w(line)
                     Timber.w(e)
+                    SmartSnackbar.make(e.localizedMessage)
                 }
+                lastUpdate = timestamp
+            } else if (wasScheduled) {
+                scheduled = true
+                return
             }
-            for ((_, record) in records) if (record.id == null) {
-                check(record.sentPackets >= 0)
-                check(record.sentBytes >= 0)
-                check(record.receivedPackets >= 0)
-                check(record.receivedBytes >= 0)
-                AppDatabase.instance.trafficRecordDao.insert(record)
-            }
-            foregroundListeners(records.values, oldRecords)
             scheduleUpdateLocked()
         }
     }

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/UpstreamMonitor.kt

@@ -1,6 +1,7 @@
 package be.mygod.vpnhotspot.net.monitor
 
 import android.content.SharedPreferences
+import android.net.LinkProperties
 import be.mygod.vpnhotspot.App.Companion.app
 import java.net.InetAddress
 
@@ -18,9 +19,7 @@ abstract class UpstreamMonitor {
         }
         private var monitor = generateMonitor()
 
-        fun registerCallback(callback: Callback, failfast: (() -> Unit)? = null) = synchronized(this) {
-            monitor.registerCallback(callback, failfast)
-        }
+        fun registerCallback(callback: Callback) = synchronized(this) { monitor.registerCallback(callback) }
         fun unregisterCallback(callback: Callback) = synchronized(this) { monitor.unregisterCallback(callback) }
 
         override fun onSharedPreferenceChanged(sharedPreferences: SharedPreferences?, key: String?) {
@@ -35,7 +34,10 @@ abstract class UpstreamMonitor {
                 }
                 val new = generateMonitor()
                 monitor = new
-                callbacks.forEach { new.registerCallback(it) { if (active) it.onLost() } }
+                for (callback in callbacks) {
+                    if (active) callback.onLost()
+                    new.registerCallback(callback)
+                }
             }
         }
     }
@@ -43,24 +45,39 @@ abstract class UpstreamMonitor {
     interface Callback {
         /**
          * Called if some interface is available. This might be called on different ifname without having called onLost.
+         * This might also be called on the same ifname but with updated DNS list.
          */
         fun onAvailable(ifname: String, dns: List<InetAddress>)
         /**
          * Called if no interface is available.
          */
         fun onLost()
+        /**
+         * Called on API 23- from DefaultNetworkMonitor. This indicates that there isn't a good way of telling the
+         * default network (see DefaultNetworkMonitor) and we are using rules at priority 22000
+         * (RULE_PRIORITY_DEFAULT_NETWORK) as our fallback rules, which would work fine until Android 9.0 broke it in
+         * commit: https://android.googlesource.com/platform/system/netd/+/758627c4d93392190b08e9aaea3bbbfb92a5f364
+         */
+        fun onFallback() {
+            throw NotImplementedError()
+        }
     }
 
-    protected val callbacks = HashSet<Callback>()
-    abstract val currentIface: String?
-    protected abstract fun registerCallbackLocked(callback: Callback): Boolean
-    protected abstract fun destroyLocked()
+    val callbacks = HashSet<Callback>()
+    protected abstract val currentLinkProperties: LinkProperties?
+    open val currentIface: String? get() = currentLinkProperties?.interfaceName
+    /**
+     * There's no need for overriding currentDns for now.
+     */
+    val currentDns: List<InetAddress> get() = currentLinkProperties?.dnsServers ?: emptyList()
+    protected abstract fun registerCallbackLocked(callback: Callback)
+    abstract fun destroyLocked()
 
-    fun registerCallback(callback: Callback, failfast: (() -> Unit)? = null) {
-        if (synchronized(this) {
+    fun registerCallback(callback: Callback) {
+        synchronized(this) {
             if (!callbacks.add(callback)) return
             registerCallbackLocked(callback)
-        }) failfast?.invoke()
+        }
     }
     fun unregisterCallback(callback: Callback) = synchronized(this) {
         if (callbacks.remove(callback) && callbacks.isEmpty()) destroyLocked()

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/VpnMonitor.kt

@@ -1,9 +1,6 @@
 package be.mygod.vpnhotspot.net.monitor
 
-import android.net.ConnectivityManager
-import android.net.Network
-import android.net.NetworkCapabilities
-import android.net.NetworkRequest
+import android.net.*
 import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.debugLog
 
@@ -16,12 +13,9 @@ object VpnMonitor : UpstreamMonitor() {
             .build()
     private var registered = false
 
-    /**
-     * Obtaining ifname in onLost doesn't work so we need to cache it in onAvailable.
-     */
-    private val available = HashMap<Network, String>()
+    private val available = HashMap<Network, LinkProperties>()
     private var currentNetwork: Network? = null
-    override val currentIface: String? get() {
+    override val currentLinkProperties: LinkProperties? get() {
         val currentNetwork = currentNetwork
         return if (currentNetwork == null) null else available[currentNetwork]
     }
@@ -30,49 +24,56 @@ object VpnMonitor : UpstreamMonitor() {
             val properties = app.connectivity.getLinkProperties(network)
             val ifname = properties?.interfaceName ?: return
             synchronized(this@VpnMonitor) {
-                if (available.put(network, ifname) != null) return
-                debugLog(TAG, "onAvailable: $ifname, ${properties.dnsServers.joinToString()}")
                 val old = currentNetwork
-                if (old != null) debugLog(TAG, "Assuming old VPN interface ${available[old]} is dying")
-                currentNetwork = network
+                val oldProperties = available.put(network, properties)
+                if (old != network) {
+                    if (old != null) {
+                        debugLog(TAG, "Assuming old VPN interface ${available[old]} is dying")
+                        callbacks.forEach { it.onLost() }
+                    }
+                    currentNetwork = network
+                } else {
+                    check(ifname == oldProperties!!.interfaceName)
+                    if (properties.dnsServers == oldProperties.dnsServers) return
+                }
                 callbacks.forEach { it.onAvailable(ifname, properties.dnsServers) }
             }
         }
 
+        override fun onLinkPropertiesChanged(network: Network, properties: LinkProperties) {
+            synchronized(this@VpnMonitor) {
+                if (currentNetwork != network) return
+                val oldProperties = available.put(network, properties)!!
+                val ifname = properties.interfaceName!!
+                check(ifname == oldProperties.interfaceName)
+                if (properties.dnsServers != oldProperties.dnsServers)
+                    callbacks.forEach { it.onAvailable(ifname, properties.dnsServers) }
+            }
+        }
+
         override fun onLost(network: Network) = synchronized(this@VpnMonitor) {
-            val ifname = available.remove(network) ?: return
-            debugLog(TAG, "onLost: $ifname")
-            if (currentNetwork != network) return
-            while (available.isNotEmpty()) {
+            if (available.remove(network) == null || currentNetwork != network) return
+            if (available.isNotEmpty()) {
                 val next = available.entries.first()
                 currentNetwork = next.key
-                val properties = app.connectivity.getLinkProperties(next.key)
-                if (properties != null) {
-                    debugLog(TAG, "Switching to ${next.value} as VPN interface")
-                    callbacks.forEach { it.onAvailable(next.value, properties.dnsServers) }
-                    return
-                }
-                available.remove(next.key)
+                debugLog(TAG, "Switching to ${next.value.interfaceName} as VPN interface")
+                callbacks.forEach { it.onAvailable(next.value.interfaceName!!, next.value.dnsServers) }
+            } else {
+                callbacks.forEach { it.onLost() }
+                currentNetwork = null
             }
-            callbacks.forEach { it.onLost() }
-            currentNetwork = null
         }
     }
 
-    override fun registerCallbackLocked(callback: Callback) = if (registered) {
-        val currentNetwork = currentNetwork
-        if (currentNetwork == null) true else {
-            callback.onAvailable(available[currentNetwork]!!,
-                    app.connectivity.getLinkProperties(currentNetwork)?.dnsServers ?: emptyList())
-            false
-        }
-    } else {
-        app.connectivity.registerNetworkCallback(request, networkCallback)
-        registered = true
-        app.connectivity.allNetworks.all {
-            val cap = app.connectivity.getNetworkCapabilities(it)
-            cap == null || !cap.hasTransport(NetworkCapabilities.TRANSPORT_VPN) ||
-                    cap.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN)
+    override fun registerCallbackLocked(callback: Callback) {
+        if (registered) {
+            val currentLinkProperties = currentLinkProperties
+            if (currentLinkProperties != null) {
+                callback.onAvailable(currentLinkProperties.interfaceName!!, currentLinkProperties.dnsServers)
+            }
+        } else {
+            app.connectivity.registerNetworkCallback(request, networkCallback)
+            registered = true
         }
     }