Prevent initiailizing su in main thread

This should hopefully fix #113.

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)

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 {
+                    if (TetherOffloadManager.enabled != newValue) {
-                        TetherOffloadManager.enabled = newValue as Boolean
+                        isEnabled = false
-                        TetherOffloadManager.enabled == newValue
+                        GlobalScope.launch {
-                    } catch (e: Exception) {
+                            try {
-                        Timber.d(e)
+                                TetherOffloadManager.enabled = newValue as Boolean
-                        SmartSnackbar.make(e).show()
+                            } catch (e: Exception) {
-                        false
+                                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<Preference>("service.clean")!!.setOnPreferenceClickListener {
-            RoutingManager.clean()
+            GlobalScope.launch { RoutingManager.clean() }
             true
         }
         findPreference<Preference>(IpMonitor.KEY)!!.setOnPreferenceChangeListener { _, _ ->
             Snackbar.make(requireView(), R.string.settings_restart_required, Snackbar.LENGTH_LONG).apply {
                 setAction(R.string.settings_exit_app) {
-                    RoutingManager.clean(false)
+                    GlobalScope.launch {
-                    RootSession.trimMemory()
+                        RoutingManager.clean(false)
-                    exitProcess(0)
+                        RootSession.trimMemory()
+                        exitProcess(0)
+                    }
                 }
             }.show()
             true
         }
         findPreference<Preference>("misc.logcat")!!.setOnPreferenceClickListener {
-            val context = requireContext()
+            GlobalScope.launch(Dispatchers.IO) {
-            val logDir = File(context.cacheDir, "log")
+                val context = requireContext()
-            logDir.mkdir()
+                val logDir = File(context.cacheDir, "log")
-            val logFile = File.createTempFile("vpnhotspot-", ".log", logDir)
+                logDir.mkdir()
-            logFile.outputStream().use { out ->
+                val logFile = File.createTempFile("vpnhotspot-", ".log", logDir)
-                PrintWriter(out.bufferedWriter()).use { writer ->
+                logFile.outputStream().use { out ->
-                    writer.println("${BuildConfig.VERSION_CODE} is running on API ${Build.VERSION.SDK_INT}\n")
+                    PrintWriter(out.bufferedWriter()).use { writer ->
-                    writer.flush()
+                        writer.println("${BuildConfig.VERSION_CODE} is running on API ${Build.VERSION.SDK_INT}\n")
-                    try {
+                        writer.flush()
-                        Runtime.getRuntime().exec(arrayOf("logcat", "-d")).inputStream.use { it.copyTo(out) }
+                        try {
-                    } catch (e: IOException) {
+                            Runtime.getRuntime().exec(arrayOf("logcat", "-d")).inputStream.use { it.copyTo(out) }
-                        Timber.w(e)
+                        } catch (e: IOException) {
-                    }
+                            Timber.w(e)
-                    writer.println()
+                        }
-                    val commands = StringBuilder()
+                        writer.println()
-                    // https://android.googlesource.com/platform/external/iptables/+/android-7.0.0_r1/iptables/Android.mk#34
+                        val commands = StringBuilder()
-                    val iptablesSave = if (Build.VERSION.SDK_INT >= 24) "iptables-save" else
+                        // https://android.googlesource.com/platform/external/iptables/+/android-7.0.0_r1/iptables/Android.mk#34
-                        File(app.deviceStorage.cacheDir, "iptables-save").absolutePath.also {
+                        val iptablesSave = if (Build.VERSION.SDK_INT >= 24) "iptables-save" else
-                            commands.appendln("ln -sf /system/bin/iptables $it")
+                            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<Preference>("misc.source")!!.setOnPreferenceClickListener {

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.*
-import kotlinx.coroutines.GlobalScope
+import java.util.concurrent.ConcurrentHashMap
-import kotlinx.coroutines.launch
 
-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) {
+        val monitoredIfaces get() = downstreams.values.filter { it.monitor }.map { it.downstream }
-            downstreams.values.filter { it.monitor }.map { it.downstream }
-        }
 
-        fun isActive(iface: String) = synchronized(downstreams) { downstreams.containsKey(iface) }
+        fun isActive(iface: String) = downstreams.containsKey(iface)
-        fun isInactive(iface: String) = synchronized(downstreams) { downstreams[iface] }?.run { !started && monitor }
+        fun isInactive(iface: String) = downstreams[iface]?.run { !started && monitor }
-        fun monitored(iface: String) = synchronized(downstreams) { downstreams[iface] }?.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<String, Downstream>()
+    private val downstreams = ConcurrentHashMap<String, Downstream>()
     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) {
+    override val activeIfaces get() = downstreams.values.filter { it.started }.map { it.downstream }
-        downstreams.values.filter { it.started }.map { it.downstream }
+    override val inactiveIfaces get() = downstreams.values.filter { !it.started }.map { it.downstream }
-    }
-    override val inactiveIfaces get() = synchronized(downstreams) {
-        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) {
+        launch {
-            for (iface in intent.getStringArrayExtra(EXTRA_ADD_INTERFACES) ?: emptyArray()) {
+            if (intent != null) {
-                if (downstreams[iface] == null) Downstream(this, iface).apply {
+                for (iface in intent.getStringArrayExtra(EXTRA_ADD_INTERFACES) ?: emptyArray()) {
-                    if (start()) check(downstreams.put(iface, this) == null) else destroy()
+                    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 ->
-            intent.getStringExtra(EXTRA_ADD_INTERFACE_MONITOR)?.also { iface ->
+                    val downstream = downstreams[iface]
-                val downstream = downstreams[iface]
+                    if (downstream == null) Downstream(this@TetheringService, iface, true).apply {
-                if (downstream == null) Downstream(this, iface, true).apply {
+                        start()
-                    start()
+                        check(downstreams.put(iface, this) == null)
-                    check(downstreams.put(iface, this) == null)
+                        downstreams[iface] = this
-                    downstreams[iface] = this
+                    } else downstream.monitor = true
-                } else downstream.monitor = true
+                }
-            }
+                intent.getStringExtra(EXTRA_REMOVE_INTERFACE)?.also { downstreams.remove(it)?.destroy() }
-            intent.getStringExtra(EXTRA_REMOVE_INTERFACE)?.also { downstreams.remove(it)?.destroy() }
+                updateNotification()    // call this first just in case we are shutting down immediately
-            updateNotification()    // call this first just in case we are shutting down immediately
+                onDownstreamsChangedLocked()
-            onDownstreamsChangedLocked()
+            } else if (downstreams.isEmpty()) stopSelf(startId)
-        } 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()
     }

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) {

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) {

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<String>()
     private val stderr = ArrayList<String>()