diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt index 7e454314..43bba2c3 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt @@ -394,7 +394,7 @@ class RepeaterService : Service(), CoroutineScope, WifiP2pManager.ChannelListene persistNextGroup = false } check(routingManager == null) - routingManager = object : RoutingManager.LocalOnly(this, group.`interface`!!) { + routingManager = object : RoutingManager.LocalOnly(this@RepeaterService, group.`interface`!!) { override fun ifaceHandler(iface: NetworkInterface) { iface.hardwareAddress?.let { lastMac = MacAddressCompat.bytesToString(it) } } diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt b/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt index 9a7ca3f7..e8c7f6cb 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt @@ -25,10 +25,13 @@ abstract class RoutingManager(private val caller: Any, val downstream: String, p } set(value) = app.pref.edit().putString(KEY_MASQUERADE_MODE, value.name).apply() + /** + * Thread safety: needs protection by companion object! + */ private val active = mutableMapOf() - fun clean(reinit: Boolean = true) { - if (!reinit && active.isEmpty()) return + fun clean(reinit: Boolean = true) = synchronized(this) { + if (!reinit && active.isEmpty()) return@synchronized for (manager in active.values) manager.routing?.stop() try { Routing.clean() @@ -37,7 +40,7 @@ abstract class RoutingManager(private val caller: Any, val downstream: String, p SmartSnackbar.make(e).show() return } - if (reinit) for (manager in active.values) manager.initRouting() + if (reinit) for (manager in active.values) manager.initRoutingLocked() } } @@ -53,29 +56,36 @@ abstract class RoutingManager(private val caller: Any, val downstream: String, p } } - val started get() = active[downstream] === this + var started = false + private set + /** + * Thread safety: needs protection by companion object! + */ private var routing: Routing? = null private var isWifi = forceWifi || TetherType.ofInterface(downstream).isWifi - fun start() = when (val other = active.putIfAbsent(downstream, this)) { - null -> { - if (isWifi) WifiDoubleLock.acquire(this) - if (!forceWifi && Build.VERSION.SDK_INT >= 30) TetherType.listener[this] = { - val isWifiNow = TetherType.ofInterface(downstream).isWifi - if (isWifi != isWifiNow) { - if (isWifi) WifiDoubleLock.release(this) else WifiDoubleLock.acquire(this) - isWifi = isWifiNow + fun start() = synchronized(RoutingManager) { + started = true + when (val other = active.putIfAbsent(downstream, this)) { + null -> { + if (isWifi) WifiDoubleLock.acquire(this) + if (!forceWifi && Build.VERSION.SDK_INT >= 30) TetherType.listener[this] = { + val isWifiNow = TetherType.ofInterface(downstream).isWifi + if (isWifi != isWifiNow) { + if (isWifi) WifiDoubleLock.release(this) else WifiDoubleLock.acquire(this) + isWifi = isWifiNow + } } + initRoutingLocked() } - initRouting() + this -> true // already started + else -> error("Double routing detected for $downstream from $caller != ${other.caller}") } - this -> true // already started - else -> error("Double routing detected for $downstream from $caller != ${other.caller}") } open fun ifaceHandler(iface: NetworkInterface) { } - private fun initRouting() = try { + private fun initRoutingLocked() = try { routing = Routing(caller, downstream, this::ifaceHandler).apply { try { configure() @@ -94,7 +104,8 @@ abstract class RoutingManager(private val caller: Any, val downstream: String, p protected abstract fun Routing.configure() - fun stop() { + fun stop() = synchronized(RoutingManager) { + started = false if (active.remove(downstream, this)) { if (!forceWifi && Build.VERSION.SDK_INT >= 30) TetherType.listener -= this if (isWifi) WifiDoubleLock.release(this) diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt b/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt index d3ebc721..47dd8c5b 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt @@ -129,6 +129,8 @@ class Routing(private val caller: Any, private val downstream: String, private val hostSubnet = "${hostAddress.address.hostAddress}/${hostAddress.networkPrefixLength}" private val transaction = RootSession.beginTransaction() + @Volatile + private var stopped = false private var masqueradeMode = MasqueradeMode.None private val upstreams = HashSet() @@ -164,6 +166,7 @@ class Routing(private val caller: Any, private val downstream: String, var dns: List = emptyList() override fun onAvailable(ifname: String, properties: LinkProperties) = synchronized(this@Routing) { + if (stopped) return val subrouting = subrouting when { subrouting != null -> check(subrouting.upstream == ifname) { "${subrouting.upstream} != $ifname" } @@ -181,6 +184,7 @@ class Routing(private val caller: Any, private val downstream: String, } override fun onLost() = synchronized(this@Routing) { + if (stopped) return val subrouting = subrouting ?: return // we could be removing fallback subrouting which no collision could ever happen, check before removing subrouting.upstream?.let { check(upstreams.remove(it)) } @@ -193,6 +197,7 @@ class Routing(private val caller: Any, private val downstream: String, private val fallbackUpstream = object : Upstream(RULE_PRIORITY_UPSTREAM_FALLBACK) { var fallbackInactive = true override fun onFallback() = synchronized(this@Routing) { + if (stopped) return fallbackInactive = false check(subrouting == null) subrouting = try { @@ -231,6 +236,7 @@ class Routing(private val caller: Any, private val downstream: String, } private val clients = mutableMapOf() override fun onIpNeighbourAvailable(neighbours: Collection) = synchronized(this) { + if (stopped) return val toRemove = HashSet(clients.keys) for (neighbour in neighbours) { if (neighbour.dev != downstream || neighbour.ip !is Inet4Address || @@ -328,9 +334,11 @@ class Routing(private val caller: Any, private val downstream: String, } fun stop() { + stopped = true IpNeighbourMonitor.unregisterCallback(this) FallbackUpstreamMonitor.unregisterCallback(fallbackUpstream) UpstreamMonitor.unregisterCallback(upstream) + Timber.i("Stopped routing for $downstream by $caller") } fun commit() { @@ -345,7 +353,6 @@ class Routing(private val caller: Any, private val downstream: String, } fun revert() { stop() - Timber.i("Stopped routing for $downstream by $caller") TrafficRecorder.update() // record stats before exiting to prevent stats losing synchronized(this) { clients.values.forEach { it.close() } } currentDns?.transaction?.revert() diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/IpMonitor.kt b/mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/IpMonitor.kt index 28ea9977..5c5a2494 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/IpMonitor.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/IpMonitor.kt @@ -70,7 +70,7 @@ abstract class IpMonitor : Runnable { } try { process.inputStream.bufferedReader().forEachLine { - if (errorMatcher.matches(it)) { + if (errorMatcher.containsMatchIn(it)) { Timber.w(it) process.destroy() // move on to next mode } else processLine(it) @@ -119,7 +119,7 @@ abstract class IpMonitor : Runnable { } process.inputStream.bufferedReader().useLines { processLines(it.map { line -> - if (errorMatcher.matches(line)) throw IOException(line) + if (errorMatcher.containsMatchIn(line)) throw IOException(line) line }) } @@ -134,15 +134,17 @@ abstract class IpMonitor : Runnable { } try { val command = "ip $monitoredObject" - RootSession.use { - val result = it.execQuiet(command) + RootSession.use { shell -> + val result = shell.execQuiet(command) RootSession.checkOutput(command, result, false) - if (result.out.any { errorMatcher.matches(it) }) throw IOException(result.out.joinToString("\n")) + if (result.out.any { errorMatcher.containsMatchIn(it) }) { + throw IOException(result.out.joinToString("\n")) + } processLines(result.out.asSequence()) } } catch (e: RuntimeException) { - app.logEvent("ip_su_poll_failure") - Timber.w(e) + app.logEvent("ip_su_poll_failure") { param("cause", e.message.toString()) } + Timber.d(e) } }