From f61f694d5f00be07fd58fb0fa2c2dfcb4ec4fa8f Mon Sep 17 00:00:00 2001 From: Mygod Date: Tue, 16 Jul 2019 10:23:21 +0800 Subject: [PATCH] Prevent initiailizing su in main thread This should hopefully fix #113. --- .../be/mygod/vpnhotspot/RepeaterService.kt | 35 ++-- .../vpnhotspot/SettingsPreferenceFragment.kt | 154 ++++++++++-------- .../be/mygod/vpnhotspot/TetheringService.kt | 76 ++++----- .../be/mygod/vpnhotspot/net/DhcpWorkaround.kt | 6 +- .../vpnhotspot/net/TetherOffloadManager.kt | 1 - .../be/mygod/vpnhotspot/util/RootSession.kt | 7 + 6 files changed, 157 insertions(+), 122 deletions(-) diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt index 04356743..f478836c 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt @@ -23,13 +23,18 @@ import be.mygod.vpnhotspot.net.wifi.WifiP2pManagerHelper.startWps import be.mygod.vpnhotspot.net.wifi.configuration.channelToFrequency import be.mygod.vpnhotspot.util.* import be.mygod.vpnhotspot.widget.SmartSnackbar +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch +import kotlinx.coroutines.newSingleThreadContext import timber.log.Timber import java.lang.reflect.InvocationTargetException /** * Service for handling Wi-Fi P2P. `supported` must be checked before this service is started otherwise it would crash. */ -class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPreferences.OnSharedPreferenceChangeListener { +class RepeaterService : Service(), CoroutineScope, WifiP2pManager.ChannelListener, + SharedPreferences.OnSharedPreferenceChangeListener { companion object { private const val TAG = "RepeaterService" private const val KEY_NETWORK_NAME = "service.repeater.networkName" @@ -121,7 +126,7 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere when (intent.action) { WifiP2pManager.WIFI_P2P_STATE_CHANGED_ACTION -> if (intent.getIntExtra(WifiP2pManager.EXTRA_WIFI_STATE, 0) == - WifiP2pManager.WIFI_P2P_STATE_DISABLED) clean() // ignore P2P enabled + WifiP2pManager.WIFI_P2P_STATE_DISABLED) launch { cleanLocked() } // ignore P2P enabled WifiP2pManager.WIFI_P2P_CONNECTION_CHANGED_ACTION -> onP2pConnectionChanged( intent.getParcelableExtra(WifiP2pManager.EXTRA_WIFI_P2P_INFO)!!, intent.getParcelableExtra(WifiP2pManager.EXTRA_WIFI_P2P_GROUP)!!) @@ -136,6 +141,10 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere WifiP2pManagerHelper.WIFI_P2P_PERSISTENT_GROUPS_CHANGED_ACTION -> onPersistentGroupsChanged() } } + /** + * Writes and critical reads to routingManager should be protected with this context. + */ + override val coroutineContext = newSingleThreadContext("TetheringService") + Job() private var routingManager: RoutingManager? = null private var persistNextGroup = false @@ -250,7 +259,7 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere p2pManager.requestGroupInfo(channel) { when { it == null -> doStart() - it.isGroupOwner -> if (routingManager == null) doStart(it) + it.isGroupOwner -> launch { if (routingManager == null) doStartLocked(it) } else -> { Timber.i("Removing old group ($it)") p2pManager.removeGroup(channel, object : WifiP2pManager.ActionListener { @@ -324,23 +333,23 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere /** * Used during step 2, also called when connection changed */ - private fun onP2pConnectionChanged(info: WifiP2pInfo, group: WifiP2pGroup) { + private fun onP2pConnectionChanged(info: WifiP2pInfo, group: WifiP2pGroup) = launch { DebugHelper.log(TAG, "P2P connection changed: $info\n$group") when { !info.groupFormed || !info.isGroupOwner || !group.isGroupOwner -> { - if (routingManager != null) clean() // P2P shutdown, else other groups changing before start, ignore + if (routingManager != null) cleanLocked() // P2P shutdown, else other groups changing before start, ignore } routingManager != null -> { binder.group = group showNotification(group) } - else -> doStart(group) + else -> doStartLocked(group) } } /** * startService Step 3 */ - private fun doStart(group: WifiP2pGroup) { + private fun doStartLocked(group: WifiP2pGroup) { binder.group = group if (persistNextGroup) { networkName = group.networkName @@ -355,7 +364,7 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere private fun startFailure(msg: CharSequence, group: WifiP2pGroup? = null) { SmartSnackbar.make(msg).show() showNotification() - if (group != null) removeGroup() else clean() + if (group != null) removeGroup() else cleanLocked() } private fun showNotification(group: WifiP2pGroup? = null) = ServiceNotification.startForeground(this, @@ -363,12 +372,14 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere private fun removeGroup() { p2pManager.removeGroup(channel, object : WifiP2pManager.ActionListener { - override fun onSuccess() = clean() + override fun onSuccess() { + launch { cleanLocked() } + } override fun onFailure(reason: Int) { if (reason != WifiP2pManager.BUSY) { SmartSnackbar.make(formatReason(R.string.repeater_remove_group_failure, reason)).show() } // else assuming it's already gone - clean() + launch { cleanLocked() } } }) } @@ -378,7 +389,7 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere receiverRegistered = false } } - private fun clean() { + private fun cleanLocked() { unregisterReceiver() routingManager?.destroy() routingManager = null @@ -390,7 +401,7 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere override fun onDestroy() { handler.removeCallbacksAndMessages(null) if (status != Status.IDLE) binder.shutdown() - clean() // force clean to prevent leakage + launch { cleanLocked() } // force clean to prevent leakage if (Build.VERSION.SDK_INT < 29) @Suppress("DEPRECATION") { app.pref.unregisterOnSharedPreferenceChangeListener(this) unregisterReceiver(deviceListener) diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/SettingsPreferenceFragment.kt b/mobile/src/main/java/be/mygod/vpnhotspot/SettingsPreferenceFragment.kt index a6dd90bf..49abf9e6 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/SettingsPreferenceFragment.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/SettingsPreferenceFragment.kt @@ -21,6 +21,10 @@ import be.mygod.vpnhotspot.util.RootSession import be.mygod.vpnhotspot.util.launchUrl import be.mygod.vpnhotspot.widget.SmartSnackbar import com.google.android.material.snackbar.Snackbar +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import timber.log.Timber import java.io.File import java.io.IOException @@ -40,14 +44,22 @@ class SettingsPreferenceFragment : PreferenceFragmentCompat() { if (Build.VERSION.SDK_INT >= 27) { isChecked = TetherOffloadManager.enabled setOnPreferenceChangeListener { _, newValue -> - try { - TetherOffloadManager.enabled = newValue as Boolean - TetherOffloadManager.enabled == newValue - } catch (e: Exception) { - Timber.d(e) - SmartSnackbar.make(e).show() - false + if (TetherOffloadManager.enabled != newValue) { + isEnabled = false + GlobalScope.launch { + try { + TetherOffloadManager.enabled = newValue as Boolean + } catch (e: Exception) { + Timber.d(e) + SmartSnackbar.make(e).show() + } + withContext(Dispatchers.Main) { + isChecked = TetherOffloadManager.enabled + isEnabled = true + } + } } + false } } else parent!!.removePreference(this) } @@ -60,82 +72,86 @@ class SettingsPreferenceFragment : PreferenceFragmentCompat() { boot.isChecked = BootReceiver.enabled } else boot.parent!!.removePreference(boot) findPreference("service.clean")!!.setOnPreferenceClickListener { - RoutingManager.clean() + GlobalScope.launch { RoutingManager.clean() } true } findPreference(IpMonitor.KEY)!!.setOnPreferenceChangeListener { _, _ -> Snackbar.make(requireView(), R.string.settings_restart_required, Snackbar.LENGTH_LONG).apply { setAction(R.string.settings_exit_app) { - RoutingManager.clean(false) - RootSession.trimMemory() - exitProcess(0) + GlobalScope.launch { + RoutingManager.clean(false) + RootSession.trimMemory() + exitProcess(0) + } } }.show() true } findPreference("misc.logcat")!!.setOnPreferenceClickListener { - val context = requireContext() - val logDir = File(context.cacheDir, "log") - logDir.mkdir() - val logFile = File.createTempFile("vpnhotspot-", ".log", logDir) - logFile.outputStream().use { out -> - PrintWriter(out.bufferedWriter()).use { writer -> - writer.println("${BuildConfig.VERSION_CODE} is running on API ${Build.VERSION.SDK_INT}\n") - writer.flush() - try { - Runtime.getRuntime().exec(arrayOf("logcat", "-d")).inputStream.use { it.copyTo(out) } - } catch (e: IOException) { - Timber.w(e) - } - writer.println() - val commands = StringBuilder() - // https://android.googlesource.com/platform/external/iptables/+/android-7.0.0_r1/iptables/Android.mk#34 - val iptablesSave = if (Build.VERSION.SDK_INT >= 24) "iptables-save" else - File(app.deviceStorage.cacheDir, "iptables-save").absolutePath.also { - commands.appendln("ln -sf /system/bin/iptables $it") + GlobalScope.launch(Dispatchers.IO) { + val context = requireContext() + val logDir = File(context.cacheDir, "log") + logDir.mkdir() + val logFile = File.createTempFile("vpnhotspot-", ".log", logDir) + logFile.outputStream().use { out -> + PrintWriter(out.bufferedWriter()).use { writer -> + writer.println("${BuildConfig.VERSION_CODE} is running on API ${Build.VERSION.SDK_INT}\n") + writer.flush() + try { + Runtime.getRuntime().exec(arrayOf("logcat", "-d")).inputStream.use { it.copyTo(out) } + } catch (e: IOException) { + Timber.w(e) + } + writer.println() + val commands = StringBuilder() + // https://android.googlesource.com/platform/external/iptables/+/android-7.0.0_r1/iptables/Android.mk#34 + val iptablesSave = if (Build.VERSION.SDK_INT >= 24) "iptables-save" else + File(app.deviceStorage.cacheDir, "iptables-save").absolutePath.also { + commands.appendln("ln -sf /system/bin/iptables $it") + } + commands.append(""" + |echo dumpsys ${Context.WIFI_P2P_SERVICE} + |dumpsys ${Context.WIFI_P2P_SERVICE} + |echo + |echo dumpsys ${Context.CONNECTIVITY_SERVICE} tethering + |dumpsys ${Context.CONNECTIVITY_SERVICE} tethering + |echo + |echo iptables -t filter + |$iptablesSave -t filter + |echo + |echo iptables -t nat + |$iptablesSave -t nat + |echo + |echo ip rule + |ip rule + |echo + |echo ip neigh + |ip neigh + |echo + |echo iptables -nvx -L vpnhotspot_fwd + |$IPTABLES -nvx -L vpnhotspot_fwd + |echo + |echo iptables -nvx -L vpnhotspot_acl + |$IPTABLES -nvx -L vpnhotspot_acl + |echo + |echo logcat-su + |logcat -d + """.trimMargin()) + try { + RootSession.use { it.execQuiet(commands.toString(), true).out.forEach(writer::println) } + } catch (e: Exception) { + e.printStackTrace(writer) + Timber.i(e) } - commands.append(""" - |echo dumpsys ${Context.WIFI_P2P_SERVICE} - |dumpsys ${Context.WIFI_P2P_SERVICE} - |echo - |echo dumpsys ${Context.CONNECTIVITY_SERVICE} tethering - |dumpsys ${Context.CONNECTIVITY_SERVICE} tethering - |echo - |echo iptables -t filter - |$iptablesSave -t filter - |echo - |echo iptables -t nat - |$iptablesSave -t nat - |echo - |echo ip rule - |ip rule - |echo - |echo ip neigh - |ip neigh - |echo - |echo iptables -nvx -L vpnhotspot_fwd - |$IPTABLES -nvx -L vpnhotspot_fwd - |echo - |echo iptables -nvx -L vpnhotspot_acl - |$IPTABLES -nvx -L vpnhotspot_acl - |echo - |echo logcat-su - |logcat -d - """.trimMargin()) - try { - RootSession.use { it.execQuiet(commands.toString(), true).out.forEach(writer::println) } - } catch (e: Exception) { - e.printStackTrace(writer) - Timber.i(e) } } + context.startActivity(Intent.createChooser(Intent(Intent.ACTION_SEND) + .setType("text/x-log") + .setFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION) + .putExtra(Intent.EXTRA_STREAM, + FileProvider.getUriForFile(context, "be.mygod.vpnhotspot.log", logFile)), + context.getString(R.string.abc_shareactionprovider_share_with))) } - startActivity(Intent.createChooser(Intent(Intent.ACTION_SEND) - .setType("text/x-log") - .setFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION) - .putExtra(Intent.EXTRA_STREAM, - FileProvider.getUriForFile(context, "be.mygod.vpnhotspot.log", logFile)), - getString(R.string.abc_shareactionprovider_share_with))) true } findPreference("misc.source")!!.setOnPreferenceClickListener { diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/TetheringService.kt b/mobile/src/main/java/be/mygod/vpnhotspot/TetheringService.kt index 7c0b7552..a476e6b2 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/TetheringService.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/TetheringService.kt @@ -10,11 +10,10 @@ import be.mygod.vpnhotspot.net.TetheringManager.tetheredIfaces import be.mygod.vpnhotspot.net.monitor.IpNeighbourMonitor import be.mygod.vpnhotspot.util.Event0 import be.mygod.vpnhotspot.util.broadcastReceiver -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.GlobalScope -import kotlinx.coroutines.launch +import kotlinx.coroutines.* +import java.util.concurrent.ConcurrentHashMap -class TetheringService : IpNeighbourMonitoringService() { +class TetheringService : IpNeighbourMonitoringService(), CoroutineScope { companion object { const val EXTRA_ADD_INTERFACES = "interface.add" const val EXTRA_ADD_INTERFACE_MONITOR = "interface.add.monitor" @@ -23,13 +22,11 @@ class TetheringService : IpNeighbourMonitoringService() { inner class Binder : android.os.Binder() { val routingsChanged = Event0() - val monitoredIfaces get() = synchronized(downstreams) { - downstreams.values.filter { it.monitor }.map { it.downstream } - } + val monitoredIfaces get() = downstreams.values.filter { it.monitor }.map { it.downstream } - fun isActive(iface: String) = synchronized(downstreams) { downstreams.containsKey(iface) } - fun isInactive(iface: String) = synchronized(downstreams) { downstreams[iface] }?.run { !started && monitor } - fun monitored(iface: String) = synchronized(downstreams) { downstreams[iface] }?.monitor + fun isActive(iface: String) = downstreams.containsKey(iface) + fun isInactive(iface: String) = downstreams[iface]?.run { !started && monitor } + fun monitored(iface: String) = downstreams[iface]?.monitor } private inner class Downstream(caller: Any, downstream: String, var monitor: Boolean = false) : @@ -42,13 +39,17 @@ class TetheringService : IpNeighbourMonitoringService() { } } + /** + * Writes and critical reads to downstreams should be protected with this context. + */ + override val coroutineContext = newSingleThreadContext("TetheringService") + Job() private val binder = Binder() - private val downstreams = mutableMapOf() + private val downstreams = ConcurrentHashMap() private var receiverRegistered = false private val receiver = broadcastReceiver { _, intent -> - synchronized(downstreams) { + launch { val toRemove = downstreams.toMutableMap() // make a copy - for (iface in intent.tetheredIfaces ?: return@synchronized) { + for (iface in intent.tetheredIfaces ?: return@launch) { val downstream = toRemove.remove(iface) ?: continue if (downstream.monitor) downstream.start() } @@ -58,12 +59,8 @@ class TetheringService : IpNeighbourMonitoringService() { onDownstreamsChangedLocked() } } - override val activeIfaces get() = synchronized(downstreams) { - downstreams.values.filter { it.started }.map { it.downstream } - } - override val inactiveIfaces get() = synchronized(downstreams) { - downstreams.values.filter { !it.started }.map { it.downstream } - } + override val activeIfaces get() = downstreams.values.filter { it.started }.map { it.downstream } + override val inactiveIfaces get() = downstreams.values.filter { !it.started }.map { it.downstream } private fun onDownstreamsChangedLocked() { if (downstreams.isEmpty()) { @@ -78,37 +75,40 @@ class TetheringService : IpNeighbourMonitoringService() { } updateNotification() } - GlobalScope.launch(Dispatchers.Main) { binder.routingsChanged() } + launch(Dispatchers.Main) { binder.routingsChanged() } } override fun onBind(intent: Intent?) = binder override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { - if (intent != null) synchronized(downstreams) { - for (iface in intent.getStringArrayExtra(EXTRA_ADD_INTERFACES) ?: emptyArray()) { - if (downstreams[iface] == null) Downstream(this, iface).apply { - if (start()) check(downstreams.put(iface, this) == null) else destroy() + launch { + if (intent != null) { + for (iface in intent.getStringArrayExtra(EXTRA_ADD_INTERFACES) ?: emptyArray()) { + if (downstreams[iface] == null) Downstream(this@TetheringService, iface).apply { + if (start()) check(downstreams.put(iface, this) == null) else destroy() + } } - } - intent.getStringExtra(EXTRA_ADD_INTERFACE_MONITOR)?.also { iface -> - val downstream = downstreams[iface] - if (downstream == null) Downstream(this, iface, true).apply { - start() - check(downstreams.put(iface, this) == null) - downstreams[iface] = this - } else downstream.monitor = true - } - intent.getStringExtra(EXTRA_REMOVE_INTERFACE)?.also { downstreams.remove(it)?.destroy() } - updateNotification() // call this first just in case we are shutting down immediately - onDownstreamsChangedLocked() - } else if (downstreams.isEmpty()) stopSelf(startId) + intent.getStringExtra(EXTRA_ADD_INTERFACE_MONITOR)?.also { iface -> + val downstream = downstreams[iface] + if (downstream == null) Downstream(this@TetheringService, iface, true).apply { + start() + check(downstreams.put(iface, this) == null) + downstreams[iface] = this + } else downstream.monitor = true + } + intent.getStringExtra(EXTRA_REMOVE_INTERFACE)?.also { downstreams.remove(it)?.destroy() } + updateNotification() // call this first just in case we are shutting down immediately + onDownstreamsChangedLocked() + } else if (downstreams.isEmpty()) stopSelf(startId) + } return START_NOT_STICKY } override fun onDestroy() { - synchronized(downstreams) { + launch { downstreams.values.forEach { it.destroy() } // force clean to prevent leakage unregisterReceiver() + cancel() } super.onDestroy() } diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/net/DhcpWorkaround.kt b/mobile/src/main/java/be/mygod/vpnhotspot/net/DhcpWorkaround.kt index 9e304b01..59520207 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/net/DhcpWorkaround.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/net/DhcpWorkaround.kt @@ -4,6 +4,8 @@ import android.content.SharedPreferences import be.mygod.vpnhotspot.App.Companion.app import be.mygod.vpnhotspot.util.RootSession import be.mygod.vpnhotspot.widget.SmartSnackbar +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.launch import timber.log.Timber import java.io.IOException @@ -24,7 +26,7 @@ object DhcpWorkaround : SharedPreferences.OnSharedPreferenceChangeListener { } val shouldEnable get() = app.pref.getBoolean(KEY_ENABLED, false) - fun enable(enabled: Boolean) { + fun enable(enabled: Boolean) = GlobalScope.launch { val action = if (enabled) "add" else "del" try { RootSession.use { it.exec("ip rule $action iif lo uidrange 0-0 lookup local_network priority 11000") } @@ -33,7 +35,7 @@ object DhcpWorkaround : SharedPreferences.OnSharedPreferenceChangeListener { e.result.err.joinToString("\n") == "RTNETLINK answers: File exists" } else { e.result.err.joinToString("\n") == "RTNETLINK answers: No such file or directory" - }) return + }) return@launch Timber.w(IOException("Failed to tweak dhcp workaround rule", e)) SmartSnackbar.make(e).show() } catch (e: Exception) { diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/net/TetherOffloadManager.kt b/mobile/src/main/java/be/mygod/vpnhotspot/net/TetherOffloadManager.kt index 3f0b899f..c1dc9e54 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/net/TetherOffloadManager.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/net/TetherOffloadManager.kt @@ -16,7 +16,6 @@ import be.mygod.vpnhotspot.util.RootSession @RequiresApi(27) object TetherOffloadManager { private const val TETHER_OFFLOAD_DISABLED = "tether_offload_disabled" - @JvmStatic var enabled: Boolean get() = Settings.Global.getInt(app.contentResolver, TETHER_OFFLOAD_DISABLED, 0) == 0 set(value) { diff --git a/mobile/src/main/java/be/mygod/vpnhotspot/util/RootSession.kt b/mobile/src/main/java/be/mygod/vpnhotspot/util/RootSession.kt index 382cc85a..7c779174 100644 --- a/mobile/src/main/java/be/mygod/vpnhotspot/util/RootSession.kt +++ b/mobile/src/main/java/be/mygod/vpnhotspot/util/RootSession.kt @@ -2,6 +2,7 @@ package be.mygod.vpnhotspot.util import android.os.Handler import android.os.HandlerThread +import android.os.Looper import androidx.core.os.postDelayed import com.topjohnwu.superuser.Shell import timber.log.Timber @@ -67,6 +68,12 @@ class RootSession : AutoCloseable { class UnexpectedOutputException(msg: String, val result: Shell.Result) : RuntimeException(msg) + init { + check(Looper.getMainLooper().thread != Thread.currentThread()) { + "Unable to initialize shell in main thread" // https://github.com/topjohnwu/libsu/issues/33 + } + } + private val shell = Shell.newInstance("su") private val stdout = ArrayList() private val stderr = ArrayList()