Migrate from NoisySu to RootSession

Fix #24. Note that just like before, IpMonitor doesn't use RootSession.

README.md

@@ -109,7 +109,7 @@ Undocumented system binaries are all bundled and executable:
 * `echo`;
 * `ip` (`link monitor neigh rule` with proper output format);
 * `iptables` (with correct version corresponding to API level);
-* `su` (needs to support `-c` argument).
+* `su`.
 
 If some of these are unavailable, you can alternatively install a recent version (v1.28.1 or higher) of Busybox.
 

mobile/build.gradle

@@ -49,6 +49,7 @@ dependencies {
     implementation 'com.android.billingclient:billing:1.1'
     implementation 'com.crashlytics.sdk.android:crashlytics:2.9.5'
     implementation 'com.github.luongvo:BadgeView:1.1.5'
+    implementation 'com.github.topjohnwu:libsu:2.0.1'
     implementation "com.google.android.material:material:$androidxVersion"
     implementation 'com.linkedin.dexmaker:dexmaker-mockito:2.19.1'
     implementation 'com.takisoft.preferencex:preferencex:1.0.0-alpha2'

mobile/src/main/java/be/mygod/vpnhotspot/LocalOnlyInterfaceManager.kt

@@ -7,7 +7,6 @@ import be.mygod.vpnhotspot.widget.SmartSnackbar
 import com.crashlytics.android.Crashlytics
 import java.net.InetAddress
 import java.net.InterfaceAddress
-import java.net.SocketException
 
 class LocalOnlyInterfaceManager(val downstream: String) : UpstreamMonitor.Callback {
     private var routing: Routing? = null
@@ -21,13 +20,13 @@ class LocalOnlyInterfaceManager(val downstream: String) : UpstreamMonitor.Callba
     override fun onAvailable(ifname: String, dns: List<InetAddress>) {
         val routing = routing
         initRouting(ifname, if (routing == null) null else {
-            routing.stop()
+            routing.revert()
             routing.hostAddress
         }, dns)
     }
     override fun onLost() {
         val routing = routing ?: return
-        routing.stop()
+        routing.revert()
         initRouting(null, routing.hostAddress, emptyList())
     }
 
@@ -39,20 +38,26 @@ class LocalOnlyInterfaceManager(val downstream: String) : UpstreamMonitor.Callba
 
     private fun initRouting(upstream: String? = null, owner: InterfaceAddress? = null,
                             dns: List<InetAddress> = this.dns) {
+        this.dns = dns
         try {
-            this.dns = dns
             this.routing = Routing(upstream, downstream, owner).apply {
-                val strict = app.strict
-                if (strict && upstream == null) return@apply    // in this case, nothing to be done
-                if (app.dhcpWorkaround) dhcpWorkaround()
-                ipForward()                                     // local only interfaces need to enable ip_forward
-                rule()
-                forward(strict)
-                if (app.masquerade) masquerade(strict)
-                dnsRedirect(dns)
-                if (!start()) SmartSnackbar.make(R.string.noisy_su_failure).show()
+                try {
+                    val strict = app.strict
+                    if (strict && upstream == null) return@apply    // in this case, nothing to be done
+                    if (app.dhcpWorkaround) dhcpWorkaround()
+                    ipForward()                                     // local only interfaces need to enable ip_forward
+                    rule()
+                    forward(strict)
+                    if (app.masquerade) masquerade(strict)
+                    dnsRedirect(dns)
+                } catch (e: Exception) {
+                    revert()
+                    throw e
+                } finally {
+                    commit()
+                }
             }
-        } catch (e: SocketException) {
+        } catch (e: Exception) {
             SmartSnackbar.make(e.localizedMessage).show()
             Crashlytics.logException(e)
             routing = null
@@ -62,6 +67,6 @@ class LocalOnlyInterfaceManager(val downstream: String) : UpstreamMonitor.Callba
     fun stop() {
         UpstreamMonitor.unregisterCallback(this)
         app.cleanRoutings -= this
-        routing?.stop()
+        routing?.revert()
     }
 }

mobile/src/main/java/be/mygod/vpnhotspot/RepeaterService.kt

@@ -176,10 +176,17 @@ class RepeaterService : Service(), WifiP2pManager.ChannelListener, SharedPrefere
     override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
         if (status != Status.IDLE) return START_NOT_STICKY
         status = Status.STARTING
-        val matcher = WifiP2pManagerHelper.patternNetworkInfo.matcher(
-                loggerSu("exec dumpsys ${Context.WIFI_P2P_SERVICE}") ?: "")
+        val out = try {
+            RootSession.use { it.execOut("dumpsys ${Context.WIFI_P2P_SERVICE}") }
+        } catch (e: RuntimeException) {
+            e.printStackTrace()
+            Crashlytics.logException(e)
+            startFailure(e.localizedMessage)
+            return START_NOT_STICKY
+        }
+        val matcher = WifiP2pManagerHelper.patternNetworkInfo.matcher(out)
         when {
-            !matcher.find() -> startFailure(getString(R.string.root_unavailable))
+            !matcher.find() -> startFailure(out)
             matcher.group(2) == "true" -> {
                 unregisterReceiver()
                 registerReceiver(receiver, intentFilter(WifiP2pManager.WIFI_P2P_STATE_CHANGED_ACTION,

mobile/src/main/java/be/mygod/vpnhotspot/SettingsPreferenceFragment.kt

@@ -14,7 +14,7 @@ import be.mygod.vpnhotspot.net.Routing
 import be.mygod.vpnhotspot.net.UpstreamMonitor
 import be.mygod.vpnhotspot.preference.AlwaysAutoCompleteEditTextPreferenceDialogFragmentCompat
 import be.mygod.vpnhotspot.preference.SharedPreferenceDataStore
-import be.mygod.vpnhotspot.util.loggerSuStream
+import be.mygod.vpnhotspot.util.RootSession
 import be.mygod.vpnhotspot.widget.SmartSnackbar
 import com.crashlytics.android.Crashlytics
 import com.takisoft.preferencex.PreferenceFragmentCompat
@@ -35,8 +35,15 @@ class SettingsPreferenceFragment : PreferenceFragmentCompat() {
         }
         boot.isChecked = BootReceiver.enabled
         findPreference("service.clean").setOnPreferenceClickListener {
-            if (Routing.clean() == null) SmartSnackbar.make(R.string.root_unavailable).show()
-            else app.cleanRoutings()
+            val cleaned = try {
+                Routing.clean()
+                true
+            } catch (e: RuntimeException) {
+                e.printStackTrace()
+                SmartSnackbar.make(e.localizedMessage).show()
+                false
+            }
+            if (cleaned) app.cleanRoutings()
             true
         }
         findPreference("misc.logcat").setOnPreferenceClickListener {
@@ -82,8 +89,9 @@ class SettingsPreferenceFragment : PreferenceFragmentCompat() {
                         |logcat -d
                     """.trimMargin())
                     try {
-                        loggerSuStream(commands.toString())?.use { it.copyTo(out) }
-                    } catch (e: IOException) {
+                        out.write(RootSession.use { it.execQuiet(commands.toString(), true).out }
+                                .joinToString("\n").toByteArray())
+                    } catch (e: Exception) {
                         e.printStackTrace(writer)
                         Crashlytics.logException(e)
                         writer.flush()

mobile/src/main/java/be/mygod/vpnhotspot/TetheringService.kt

@@ -12,7 +12,6 @@ import be.mygod.vpnhotspot.util.broadcastReceiver
 import be.mygod.vpnhotspot.widget.SmartSnackbar
 import com.crashlytics.android.Crashlytics
 import java.net.InetAddress
-import java.net.SocketException
 
 class TetheringService : IpNeighbourMonitoringService(), UpstreamMonitor.Callback {
     companion object {
@@ -34,7 +33,7 @@ class TetheringService : IpNeighbourMonitoringService(), UpstreamMonitor.Callbac
     private val receiver = broadcastReceiver { _, intent ->
         synchronized(routings) {
             for (iface in routings.keys - TetheringManager.getTetheredIfaces(intent.extras!!))
-                routings.remove(iface)?.stop()
+                routings.remove(iface)?.revert()
             updateRoutingsLocked()
         }
     }
@@ -61,33 +60,37 @@ class TetheringService : IpNeighbourMonitoringService(), UpstreamMonitor.Callbac
             val upstream = upstream
             val disableIpv6 = app.pref.getBoolean("service.disableIpv6", false)
             if (upstream != null || app.strict || disableIpv6) {
-                var failed = false
                 val iterator = routings.iterator()
                 while (iterator.hasNext()) {
                     val (downstream, value) = iterator.next()
-                    if (value != null) if (value.upstream == upstream) continue else value.stop()
+                    if (value != null) if (value.upstream == upstream) continue else value.revert()
                     try {
                         routings[downstream] = Routing(upstream, downstream).apply {
-                            if (app.dhcpWorkaround) dhcpWorkaround()
-                            // system tethering already has working forwarding rules
-                            // so it doesn't make sense to add additional forwarding rules
-                            rule()
-                            // here we always enforce strict mode as fallback is handled by system which we disable
-                            forward()
-                            if (app.strict) overrideSystemRules()
-                            if (app.masquerade) masquerade()
-                            if (upstream != null) dnsRedirect(dns)
-                            if (disableIpv6) disableIpv6()
-                            if (!start()) failed = true
+                            try {
+                                if (app.dhcpWorkaround) dhcpWorkaround()
+                                // system tethering already has working forwarding rules
+                                // so it doesn't make sense to add additional forwarding rules
+                                rule()
+                                // here we always enforce strict mode as fallback is handled by system which we disable
+                                forward()
+                                if (app.strict) overrideSystemRules()
+                                if (app.masquerade) masquerade()
+                                if (upstream != null) dnsRedirect(dns)
+                                if (disableIpv6) disableIpv6()
+                            } catch (e: Exception) {
+                                revert()
+                                throw e
+                            } finally {
+                                commit()
+                            }
                         }
-                    } catch (e: SocketException) {
+                    } catch (e: Exception) {
                         e.printStackTrace()
                         Crashlytics.logException(e)
+                        SmartSnackbar.make(e.localizedMessage).show()
                         iterator.remove()
-                        failed = true
                     }
                 }
-                if (failed) SmartSnackbar.make(R.string.noisy_su_failure).show()
             }
             updateNotification()
         }
@@ -101,7 +104,7 @@ class TetheringService : IpNeighbourMonitoringService(), UpstreamMonitor.Callbac
             val iface = intent.getStringExtra(EXTRA_ADD_INTERFACE)
             synchronized(routings) {
                 if (iface != null) routings[iface] = null
-                routings.remove(intent.getStringExtra(EXTRA_REMOVE_INTERFACE))?.stop()
+                routings.remove(intent.getStringExtra(EXTRA_REMOVE_INTERFACE))?.revert()
                 updateRoutingsLocked()
             }
         } else if (routings.isEmpty()) stopSelf(startId)
@@ -120,7 +123,7 @@ class TetheringService : IpNeighbourMonitoringService(), UpstreamMonitor.Callbac
         this.dns = emptyList()
         synchronized(routings) {
             for ((iface, routing) in routings) {
-                routing?.stop()
+                routing?.revert()
                 routings[iface] = null
             }
         }

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

@@ -21,38 +21,40 @@ abstract class IpMonitor : Runnable {
     private var monitor: Process? = null
     private var pool: ScheduledExecutorService? = null
 
-    init {
-        thread("${javaClass.simpleName}-input") {
-            // monitor may get rejected by SELinux
-            val monitor = ProcessBuilder("sh", "-c",
-                    "ip monitor $monitoredObject || exec su -c 'exec ip monitor $monitoredObject'")
-                    .redirectErrorStream(true)
-                    .start()
-            this.monitor = monitor
-            thread("${javaClass.simpleName}-error") {
-                try {
-                    monitor.errorStream.bufferedReader().forEachLine {
-                        Crashlytics.log(Log.ERROR, javaClass.simpleName, it)
-                    }
-                } catch (_: InterruptedIOException) { } catch (e: IOException) {
-                    e.printStackTrace()
-                    Crashlytics.logException(e)
-                }
-            }
+    private fun handleProcess(process: Process): Int {
+        val err = thread("${javaClass.simpleName}-error") {
             try {
-                monitor.inputStream.bufferedReader().forEachLine(this::processLine)
-                monitor.waitFor()
-                if (monitor.exitValue() == 0) return@thread
-                Crashlytics.log(Log.WARN, javaClass.simpleName, "Failed to set up monitor, switching to polling")
-                Crashlytics.logException(MonitorFailure())
-                val pool = Executors.newScheduledThreadPool(1)
-                pool.scheduleAtFixedRate(this, 1, 1, TimeUnit.SECONDS)
-                this.pool = pool
+                process.errorStream.bufferedReader().forEachLine {
+                    Crashlytics.log(Log.ERROR, javaClass.simpleName, it)
+                }
             } catch (_: InterruptedIOException) { } catch (e: IOException) {
                 e.printStackTrace()
                 Crashlytics.logException(e)
             }
         }
+        try {
+            process.inputStream.bufferedReader().forEachLine(this::processLine)
+        } catch (_: InterruptedIOException) { } catch (e: IOException) {
+            e.printStackTrace()
+            Crashlytics.logException(e)
+        }
+        err.join()
+        process.waitFor()
+        return process.exitValue()
+    }
+
+    init {
+        thread("${javaClass.simpleName}-input") {
+            // monitor may get rejected by SELinux
+            if (handleProcess(ProcessBuilder("ip", "monitor", monitoredObject).start()) == 0) return@thread
+            if (handleProcess(ProcessBuilder("su", "-c", "exec ip monitor $monitoredObject").start()) == 0)
+                return@thread
+            Crashlytics.log(Log.WARN, javaClass.simpleName, "Failed to set up monitor, switching to polling")
+            Crashlytics.logException(MonitorFailure())
+            val pool = Executors.newScheduledThreadPool(1)
+            pool.scheduleAtFixedRate(this, 1, 1, TimeUnit.SECONDS)
+            this.pool = pool
+        }
     }
 
     fun flush() = thread("${javaClass.simpleName}-flush") { run() }

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

@@ -3,11 +3,15 @@ package be.mygod.vpnhotspot.net
 import android.os.Build
 import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.R
+import be.mygod.vpnhotspot.util.RootSession
 import be.mygod.vpnhotspot.util.debugLog
-import be.mygod.vpnhotspot.util.noisySu
 import java.net.*
-import java.util.*
 
+/**
+ * A transaction wrapper that helps set up routing environment.
+ *
+ * Once revert is called, this object no longer serves any purpose.
+ */
 class Routing(val upstream: String?, private val downstream: String, ownerAddress: InterfaceAddress? = null) {
     companion object {
         /**
@@ -18,17 +22,22 @@ class Routing(val upstream: String?, private val downstream: String, ownerAddres
          */
         private val IPTABLES = if (Build.VERSION.SDK_INT >= 26) "iptables -w 1" else "iptables -w"
 
-        fun clean() = noisySu(
-                "$IPTABLES -t nat -F PREROUTING",
-                "quiet while $IPTABLES -D FORWARD -j vpnhotspot_fwd; do done",
-                "$IPTABLES -F vpnhotspot_fwd",
-                "$IPTABLES -X vpnhotspot_fwd",
-                "quiet while $IPTABLES -t nat -D POSTROUTING -j vpnhotspot_masquerade; do done",
-                "$IPTABLES -t nat -F vpnhotspot_masquerade",
-                "$IPTABLES -t nat -X vpnhotspot_masquerade",
-                "quiet while ip rule del priority 17900; do done",
-                "quiet while ip rule del iif lo uidrange 0-0 lookup local_network priority 11000; do done",
-                report = false)
+        fun clean() = RootSession.use {
+            it.submit("$IPTABLES -t nat -F PREROUTING")
+            it.submit("while $IPTABLES -D FORWARD -j vpnhotspot_fwd; do done")
+            it.submit("$IPTABLES -F vpnhotspot_fwd")
+            it.submit("$IPTABLES -X vpnhotspot_fwd")
+            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 iif lo uidrange 0-0 lookup local_network priority 11000; do done")
+        }
+
+        fun RootSession.Transaction.iptablesAdd(content: String, table: String = "filter") =
+                exec("$IPTABLES -t $table -A $content", "$IPTABLES -t $table -D $content")
+        fun RootSession.Transaction.iptablesInsert(content: String, table: String = "filter") =
+                exec("$IPTABLES -t $table -I $content", "$IPTABLES -t $table -D $content")
     }
 
     class InterfaceNotFoundException : SocketException() {
@@ -37,18 +46,13 @@ class Routing(val upstream: String?, private val downstream: String, ownerAddres
 
     val hostAddress = ownerAddress ?: NetworkInterface.getByName(downstream)?.interfaceAddresses?.asSequence()
             ?.singleOrNull { it.address is Inet4Address } ?: throw InterfaceNotFoundException()
-    private val startScript = LinkedList<String>()
-    private val stopScript = LinkedList<String>()
+    private val transaction = RootSession.beginTransaction()
     var started = false
 
-    fun ipForward() {
-        startScript.add("echo 1 >/proc/sys/net/ipv4/ip_forward")
-    }
+    fun ipForward() = transaction.exec("echo 1 >/proc/sys/net/ipv4/ip_forward")
 
-    fun disableIpv6() {
-        startScript.add("echo 1 >/proc/sys/net/ipv6/conf/$downstream/disable_ipv6")
-        stopScript.add("echo 0 >/proc/sys/net/ipv6/conf/$downstream/disable_ipv6")
-    }
+    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.
@@ -58,63 +62,50 @@ class Routing(val upstream: String?, private val downstream: String, ownerAddres
      */
     fun rule() {
         if (upstream != null) {
-            startScript.add("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
-            stopScript.addFirst("ip rule del from all iif $downstream priority 17900")
+            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(strict: Boolean = true) {
-        startScript.add("quiet $IPTABLES -N vpnhotspot_fwd 2>/dev/null")
+        transaction.execQuiet("$IPTABLES -N vpnhotspot_fwd")
+        transaction.iptablesInsert("FORWARD -j vpnhotspot_fwd")
         if (strict) {
             if (upstream != null) {
-                startScript.add("$IPTABLES -A vpnhotspot_fwd -i $upstream -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
-                startScript.add("$IPTABLES -A vpnhotspot_fwd -i $downstream -o $upstream -j ACCEPT")
-                stopScript.addFirst("$IPTABLES -D vpnhotspot_fwd -i $upstream -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
-                stopScript.addFirst("$IPTABLES -D vpnhotspot_fwd -i $downstream -o $upstream -j ACCEPT")
+                transaction.iptablesAdd("vpnhotspot_fwd -i $upstream -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
+                transaction.iptablesAdd("vpnhotspot_fwd -i $downstream -o $upstream -j ACCEPT")
             }   // else nothing needs to be done
         } else {
             // for not strict mode, allow downstream packets to be redirected to anywhere
             // because we don't wanna keep track of default network changes
-            startScript.add("$IPTABLES -A vpnhotspot_fwd -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
-            startScript.add("$IPTABLES -A vpnhotspot_fwd -i $downstream -j ACCEPT")
-            stopScript.addFirst("$IPTABLES -D vpnhotspot_fwd -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
-            stopScript.addFirst("$IPTABLES -D vpnhotspot_fwd -i $downstream -j ACCEPT")
+            transaction.iptablesAdd("vpnhotspot_fwd -o $downstream -m state --state ESTABLISHED,RELATED -j ACCEPT")
+            transaction.iptablesAdd("vpnhotspot_fwd -i $downstream -j ACCEPT")
         }
-        startScript.add("$IPTABLES -I FORWARD -j vpnhotspot_fwd")
-        stopScript.addFirst("$IPTABLES -D FORWARD -j vpnhotspot_fwd")
     }
 
-    fun overrideSystemRules() {
-        startScript.add("$IPTABLES -A vpnhotspot_fwd -i $downstream -j DROP")
-        stopScript.addFirst("$IPTABLES -D vpnhotspot_fwd -i $downstream -j DROP")
-    }
+    fun overrideSystemRules() = transaction.iptablesAdd("vpnhotspot_fwd -i $downstream -j DROP")
 
     fun masquerade(strict: Boolean = true) {
         val hostSubnet = "${hostAddress.address.hostAddress}/${hostAddress.networkPrefixLength}"
-        startScript.add("quiet $IPTABLES -t nat -N vpnhotspot_masquerade 2>/dev/null")
+        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) {
-                startScript.add("$IPTABLES -t nat -A vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE")
-                stopScript.addFirst("$IPTABLES -t nat -D vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE")
+                transaction.iptablesAdd("vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE", "nat")
             }   // else nothing needs to be done
         } else {
-            startScript.add("$IPTABLES -t nat -A vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE")
-            stopScript.addFirst("$IPTABLES -t nat -D vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE")
+            transaction.iptablesAdd("vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE", "nat")
         }
-        startScript.add("$IPTABLES -t nat -I POSTROUTING -j vpnhotspot_masquerade")
-        stopScript.addFirst("$IPTABLES -t nat -D POSTROUTING -j vpnhotspot_masquerade")
     }
 
     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)")
-        startScript.add("$IPTABLES -t nat -A PREROUTING -i $downstream -p tcp -d $hostAddress --dport 53 -j DNAT --to-destination $dns")
-        startScript.add("$IPTABLES -t nat -A PREROUTING -i $downstream -p udp -d $hostAddress --dport 53 -j DNAT --to-destination $dns")
-        stopScript.addFirst("$IPTABLES -t nat -D PREROUTING -i $downstream -p tcp -d $hostAddress --dport 53 -j DNAT --to-destination $dns")
-        stopScript.addFirst("$IPTABLES -t nat -D PREROUTING -i $downstream -p udp -d $hostAddress --dport 53 -j DNAT --to-destination $dns")
+        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")
     }
 
     /**
@@ -125,20 +116,9 @@ class Routing(val upstream: String?, private val downstream: String, ownerAddres
      *
      * Source: https://android.googlesource.com/platform/system/netd/+/b9baf26/server/RouteController.cpp#57
      */
-    fun dhcpWorkaround() {
-        startScript.add("ip rule add iif lo uidrange 0-0 lookup local_network priority 11000")
-        stopScript.addFirst("ip rule del iif lo uidrange 0-0 lookup local_network priority 11000")
-    }
+    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 start(): Boolean {
-        if (started) return true
-        started = true
-        if (noisySu(startScript) != true) stop()
-        return started
-    }
-    fun stop(): Boolean {
-        if (!started) return true
-        started = false
-        return noisySu(stopScript, false) == true
-    }
+    fun commit() = transaction.commit()
+    fun revert() = transaction.revert()
 }

mobile/src/main/java/be/mygod/vpnhotspot/net/wifi/P2pSupplicantConfiguration.kt

@@ -6,8 +6,7 @@ import android.os.Parcel
 import android.os.Parcelable
 import android.util.Log
 import be.mygod.vpnhotspot.App.Companion.app
-import be.mygod.vpnhotspot.util.loggerSu
-import be.mygod.vpnhotspot.util.noisySu
+import be.mygod.vpnhotspot.util.RootSession
 import com.crashlytics.android.Crashlytics
 import java.io.File
 import java.io.IOException
@@ -29,6 +28,7 @@ class P2pSupplicantConfiguration(private val initContent: String? = null) : Parc
          * PSK parser can be found here: https://android.googlesource.com/platform/external/wpa_supplicant_8/+/d2986c2/wpa_supplicant/config.c#488
          */
         private val pskParser = "^[\\r\\t ]*psk=(ext:|\"(.*)\"|\"(.*)|[0-9a-fA-F]{64}\$)".toRegex(RegexOption.MULTILINE)
+        private val whitespaceMatcher = "\\s+".toRegex()
 
         private val confPath = if (Build.VERSION.SDK_INT >= 28)
             "/data/vendor/wifi/wpa/p2p_supplicant.conf" else "/data/misc/wifi/p2p_supplicant.conf"
@@ -40,12 +40,12 @@ class P2pSupplicantConfiguration(private val initContent: String? = null) : Parc
     }
     override fun describeContents() = 0
 
-    private val contentDelegate = lazy { initContent ?: loggerSu("exec cat $confPath") }
+    private val contentDelegate = lazy { initContent ?: RootSession.use { it.execOut("cat $confPath") } }
     private val content by contentDelegate
 
     fun readPsk(handler: ((RuntimeException) -> Unit)? = null): String? {
         return try {
-            val match = pskParser.findAll(content ?: return null).single()
+            val match = pskParser.findAll(content).single()
             if (match.groups[2] == null && match.groups[3] == null) "" else {
                 // only one will match and hold non-empty value
                 val result = match.groupValues[2] + match.groupValues[3]
@@ -64,8 +64,7 @@ class P2pSupplicantConfiguration(private val initContent: String? = null) : Parc
         }
     }
 
-    fun update(config: WifiConfiguration): Boolean? {
-        val content = content ?: return null
+    fun update(config: WifiConfiguration) {
         val tempFile = File.createTempFile("vpnhotspot-", ".conf", app.cacheDir)
         try {
             var ssidFound = 0
@@ -86,13 +85,18 @@ class P2pSupplicantConfiguration(private val initContent: String? = null) : Parc
             }
             if (ssidFound != 1 || pskFound != 1) {
                 Crashlytics.log(Log.WARN, TAG, "Invalid conf ($ssidFound, $pskFound): $content")
-                Crashlytics.logException(InvalidConfigurationError())
+                if (ssidFound == 0 || pskFound == 0) throw InvalidConfigurationError()
+                else Crashlytics.logException(InvalidConfigurationError())
             }
-            if (ssidFound == 0 || pskFound == 0) return false
             // pkill not available on Lollipop. Source: https://android.googlesource.com/platform/system/core/+/master/shell_and_utilities/README.md
-            return noisySu("cat ${tempFile.absolutePath} > $confPath",
-                    if (Build.VERSION.SDK_INT >= 23) "pkill wpa_supplicant"
-                    else "set `ps | grep wpa_supplicant`; kill \$2")
+            RootSession.use {
+                it.exec("cat ${tempFile.absolutePath} > $confPath")
+                if (Build.VERSION.SDK_INT >= 23) it.exec("pkill wpa_supplicant") else {
+                    val result = it.execOut("ps | grep wpa_supplicant").split(whitespaceMatcher)
+                    check(result.size >= 2) { "wpa_supplicant not found, please toggle Airplane mode manually" }
+                    it.exec("kill ${result[1]}")
+                }
+            }
         } finally {
             if (!tempFile.delete()) tempFile.deleteOnExit()
         }

mobile/src/main/java/be/mygod/vpnhotspot/net/wifi/WifiP2pDialogFragment.kt

@@ -15,6 +15,7 @@ import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.R
 import be.mygod.vpnhotspot.manage.TetheringFragment
 import be.mygod.vpnhotspot.widget.SmartSnackbar
+import com.crashlytics.android.Crashlytics
 import com.google.android.material.textfield.TextInputLayout
 import java.nio.charset.Charset
 
@@ -85,13 +86,14 @@ class WifiP2pDialogFragment : DialogFragment(), TextWatcher, DialogInterface.OnC
 
     override fun onClick(dialog: DialogInterface?, which: Int) {
         when (which) {
-            DialogInterface.BUTTON_POSITIVE -> when (configurer.update(config!!)) {
-                true -> {
-                    app.handler.postDelayed((targetFragment as TetheringFragment).adapter.repeaterManager
-                            .binder!!::requestGroupUpdate, 1000)
-                }
-                false -> SmartSnackbar.make(R.string.noisy_su_failure).show()
-                null -> SmartSnackbar.make(R.string.root_unavailable).show()
+            DialogInterface.BUTTON_POSITIVE -> try {
+                configurer.update(config!!)
+                app.handler.postDelayed((targetFragment as TetheringFragment).adapter.repeaterManager
+                        .binder!!::requestGroupUpdate, 1000)
+            } catch (e: RuntimeException) {
+                e.printStackTrace()
+                Crashlytics.logException(e)
+                SmartSnackbar.make(e.localizedMessage).show()
             }
             DialogInterface.BUTTON_NEUTRAL ->
                 (targetFragment as TetheringFragment).adapter.repeaterManager.binder!!.resetCredentials()

mobile/src/main/java/be/mygod/vpnhotspot/util/NoisySu.kt

@@ -1,51 +0,0 @@
-package be.mygod.vpnhotspot.util
-
-import android.util.Log
-import be.mygod.vpnhotspot.App.Companion.app
-import com.crashlytics.android.Crashlytics
-import java.io.IOException
-import java.io.InputStream
-
-private const val NOISYSU_TAG = "NoisySU"
-private const val NOISYSU_SUFFIX = "SUCCESS\n"
-
-private class SuFailure(msg: String?) : RuntimeException(msg)
-
-fun loggerSuStream(command: String): InputStream? {
-    val process = try {
-        ProcessBuilder("su", "-c", command)
-                .directory(app.deviceStorage.cacheDir)
-                .redirectErrorStream(true)
-                .start()
-    } catch (e: IOException) {
-        e.printStackTrace()
-        Crashlytics.logException(e)
-        return null
-    }
-    return process.inputStream
-}
-
-fun loggerSu(command: String): String? {
-    val stream = loggerSuStream(command) ?: return null
-    return try {
-        stream.bufferedReader().readText()
-    } catch (e: IOException) {
-        e.printStackTrace()
-        Crashlytics.logException(e)
-        null
-    }
-}
-
-fun noisySu(commands: Iterable<String>, report: Boolean = true): Boolean? {
-    var out = loggerSu("""function noisy() { "$@" || echo "$@" exited with $?; }
-${commands.joinToString("\n") { if (it.startsWith("quiet ")) it.substring(6) else "noisy $it" }}
-echo $NOISYSU_SUFFIX""")
-    val result = if (out == null) null else out == NOISYSU_SUFFIX
-    out = out?.removeSuffix(NOISYSU_SUFFIX)
-    if (!out.isNullOrBlank()) {
-        Crashlytics.log(Log.INFO, NOISYSU_TAG, out)
-        if (report) Crashlytics.logException(SuFailure(out))
-    }
-    return result
-}
-fun noisySu(vararg commands: String, report: Boolean = true) = noisySu(commands.asIterable(), report)

mobile/src/main/java/be/mygod/vpnhotspot/util/RootSession.kt

@@ -0,0 +1,107 @@
+package be.mygod.vpnhotspot.util
+
+import android.util.Log
+import com.crashlytics.android.Crashlytics
+import com.topjohnwu.superuser.Shell
+import java.util.*
+import java.util.concurrent.locks.ReentrantLock
+import kotlin.collections.ArrayList
+import kotlin.concurrent.withLock
+
+class RootSession {
+    companion object {
+        const val TAG = "RootSession"
+        const val INIT_CHECKPOINT = "$TAG initialized successfully"
+
+        private val monitor = ReentrantLock()
+        private var instance: RootSession? = null
+        private fun ensureInstance(): RootSession {
+            var instance = instance
+            if (instance == null) instance = RootSession().also { RootSession.instance = it }
+            return instance
+        }
+        fun <T> use(operation: (RootSession) -> T) = monitor.withLock { operation(ensureInstance()) }
+        fun beginTransaction(): Transaction {
+            monitor.lock()
+            return try {
+                ensureInstance()
+            } catch (e: RuntimeException) {
+                monitor.unlock()
+                throw e
+            }.Transaction()
+        }
+    }
+
+    class UnexpectedOutputException(msg: String) : RuntimeException(msg)
+    private fun checkOutput(command: String, result: Shell.Result, out: Boolean = result.out.isNotEmpty(),
+                            err: Boolean = stderr.isNotEmpty()) {
+        if (result.isSuccess && !out && !err) return
+        val msg = StringBuilder("$command exited with ${result.code}")
+        if (out) result.out.forEach { msg.append("\n$it") }
+        // TODO bug: https://github.com/topjohnwu/libsu/pull/23
+        if (err) stderr.forEach { msg.append("\nE $it") }
+        throw UnexpectedOutputException(msg.toString())
+    }
+
+    private val shell = Shell.newInstance("su")
+    private val stdout = ArrayList<String>()
+    private val stderr = ArrayList<String>()
+
+    init {
+        // check basic shell functionality very basically
+        val result = execQuiet("echo $INIT_CHECKPOINT")
+        checkOutput("echo", result, result.out.joinToString("\n").trim() != INIT_CHECKPOINT)
+    }
+
+    /**
+     * Don't care about the results, but still sync.
+     */
+    fun submit(command: String) {
+        val result = execQuiet(command)
+        if (result.code != 0) Crashlytics.log(Log.VERBOSE, TAG, "$command exited with ${result.code}")
+        var msg = stderr.joinToString("\n").trim()
+        if (msg.isNotEmpty()) Crashlytics.log(Log.VERBOSE, TAG, msg)
+        msg = result.out.joinToString("\n").trim()
+        if (msg.isNotEmpty()) Crashlytics.log(Log.VERBOSE, TAG, msg)
+    }
+
+    fun execQuiet(command: String, redirect: Boolean = false): Shell.Result {
+        stdout.clear()
+        return shell.newJob().add(command).to(stdout, if (redirect) stdout else {
+            stderr.clear()
+            stderr
+        }).exec()
+    }
+    fun exec(command: String) = checkOutput(command, execQuiet(command))
+    fun execOut(command: String): String {
+        val result = execQuiet(command)
+        checkOutput(command, result, false)
+        return result.out.joinToString("\n")
+    }
+
+    /**
+     * This transaction is different from what you may have in mind since you can revert it after committing it.
+     */
+    inner class Transaction {
+        private val revertCommands = LinkedList<String>()
+
+        fun exec(command: String, revert: String? = null) {
+            if (revert != null) revertCommands.addFirst(revert) // add first just in case exec fails
+            this@RootSession.exec(command)
+        }
+        fun execQuiet(command: String) = this@RootSession.execQuiet(command)
+
+        fun commit() = monitor.unlock()
+
+        fun revert() {
+            if (revertCommands.isEmpty()) return
+            val shell = if (monitor.isHeldByCurrentThread) this@RootSession else {
+                monitor.lock()
+                ensureInstance()
+            }
+            revertCommands.forEach { shell.submit(it) }
+            revertCommands.clear()
+            monitor.unlock()    // commit
+        }
+    }
+}