From 37f46e02024621c5722f1a7bb885c5d8ffed2036 Mon Sep 17 00:00:00 2001 From: Mygod Date: Thu, 11 Jun 2020 06:59:42 +0800 Subject: [PATCH 1/3] Fix matching error in IpMonitor --- .../be/mygod/vpnhotspot/net/monitor/IpMonitor.kt | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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 3154a4ea..783bca70 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 @@ -53,7 +53,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) @@ -102,7 +102,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 }) } @@ -117,15 +117,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) } } From 8d6d4a6c6ebfb4dd243f68854a3d53d348d4004c Mon Sep 17 00:00:00 2001 From: Mygod Date: Fri, 12 Jun 2020 00:26:43 -0400 Subject: [PATCH 2/3] Add synchronization for RoutingManager --- .../be/mygod/vpnhotspot/RoutingManager.kt | 35 ++++++++++++------- .../java/be/mygod/vpnhotspot/net/Routing.kt | 2 +- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt b/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt index 5beb538a..7d1949e2 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/RoutingManager.kt @@ -24,10 +24,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() @@ -36,7 +39,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() } } @@ -52,21 +55,28 @@ 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 - fun start() = when (val other = active.putIfAbsent(downstream, this)) { - null -> { - if (isWifi) WifiDoubleLock.acquire(this) - initRouting() + fun start() = synchronized(RoutingManager) { + started = true + when (val other = active.putIfAbsent(downstream, this)) { + null -> { + if (isWifi) WifiDoubleLock.acquire(this) + initRoutingLocked() + } + 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() @@ -85,7 +95,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 (isWifi) WifiDoubleLock.release(this) routing?.revert() 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 ac36b12e..62fb7ba6 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt @@ -331,6 +331,7 @@ class Routing(private val caller: Any, private val downstream: String, IpNeighbourMonitor.unregisterCallback(this) FallbackUpstreamMonitor.unregisterCallback(fallbackUpstream) UpstreamMonitor.unregisterCallback(upstream) + Timber.i("Stopped routing for $downstream by $caller") } fun commit() { @@ -345,7 +346,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() From 4d3e05ac6be0b99f6ce96606f799cd8e0b36fd2b Mon Sep 17 00:00:00 2001 From: Mygod Date: Fri, 12 Jun 2020 01:16:57 -0400 Subject: [PATCH 3/3] Add stopped check in Routing --- .../src/main/java/be/mygod/vpnhotspot/RepeaterService.kt | 2 +- mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt index 6f0e7c85..8223e573 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt @@ -379,7 +379,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?.asIterable()?.macToString()?.let { lastMac = it } } 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 62fb7ba6..0964e84e 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,6 +334,7 @@ class Routing(private val caller: Any, private val downstream: String, } fun stop() { + stopped = true IpNeighbourMonitor.unregisterCallback(this) FallbackUpstreamMonitor.unregisterCallback(fallbackUpstream) UpstreamMonitor.unregisterCallback(upstream)