Work out the rendezvous name and hold it across a restart

daemon/src/main/kotlin/org/matrix/vector/daemon/VectorDaemon.kt

@@ -44,24 +44,55 @@ object VectorDaemon {
   val scope = CoroutineScope(Dispatchers.IO + SupervisorJob() + exceptionHandler)
   val bridgeServiceName = "activity"
 
-  var isLateInject = false
-  var proxyServiceName = "serial"
+  /**
+   * Whether a `system_server` was already serving the moment this daemon started, which is how
+   * Zygisk injecting into a booted system rather than into a fresh fork shows up on our side. The
+   * boot it missed then has to be replayed, and it has to be met under another name.
+   *
+   * Set on the main thread during [main] and cleared by the death recipient on a binder thread,
+   * which is what the `@Volatile` is for.
+   */
+  @Volatile var startedAfterSystemServer = false
+
+  /**
+   * The name the module looks the daemon up under while system_server is specialized. `serial` is
+   * free at boot, because system_server registers it only much later, so a binder found there can
+   * only be ours. On a booted system the real service holds it, and a lookup returning that is
+   * indistinguishable from a successful rendezvous, so a name of our own is used instead.
+   *
+   * Computed from [startedAfterSystemServer] rather than stored beside it, so that the single
+   * place a restart changes its mind about the rendezvous is the place the name is read from.
+   */
+  val proxyServiceName: String
+    get() = if (startedAfterSystemServer) "serial_vector" else "serial"
+
+  /**
+   * The `activity` binder this daemon last linked its death recipient through.
+   *
+   * A death notification lives exactly as long as the `BinderProxy` it was registered on. Once
+   * [sendToBridge] returns nothing else refers to that proxy, and when ART collects it the kernel
+   * registration goes with it, silently: the daemon then sleeps through the next system_server
+   * restart, never re-claims the rendezvous, and the module that comes back finds nothing to meet.
+   * Holding the proxy here is what lets a daemon outlive the system_server it was started with.
+   * The link is dropped through this field too, so the reference that keeps the notification
+   * alive is the same one that releases it.
+   */
+  @Volatile private var bridgeBinder: IBinder? = null
 
   @JvmStatic
   fun main(args: Array<String>) {
     if (!FileSystem.tryLock()) kotlin.system.exitProcess(0)
 
-    var systemServerMaxRetry = 1
-    for (arg in args) {
-      if (arg.startsWith("--system-server-max-retry=")) {
-        systemServerMaxRetry = arg.substringAfter('=').toIntOrNull() ?: 1
-      } else if (arg == "--late-inject") {
-        isLateInject = true
-        proxyServiceName = "serial_vector"
-      }
-    }
+    // `activity` is system_server's own, registered long after the specialization in which the
+    // module comes looking for us, so at boot nothing answers it and a binder there means someone
+    // booted without us. checkService is that question by its own name: does servicemanager hold
+    // this one now.
+    startedAfterSystemServer = ServiceManager.checkService(bridgeServiceName) != null
 
-    Log.i(TAG, "Vector daemon started: lateInject=$isLateInject, proxy=$proxyServiceName")
+    Log.i(
+        TAG,
+        "Vector daemon started: afterSystemServer=$startedAfterSystemServer, " +
+            "proxy=$proxyServiceName")
     // The hash is here rather than in the version the manager prints: Home should stay readable,
     // but every saved bug report should say exactly which commit produced the daemon that wrote it.
     Log.i(
@@ -110,7 +141,7 @@ object VectorDaemon {
     val isVerboseLog = ManagerService.isVerboseLogEnabled()
 
     // Setup IPC channel for applications by injecting DaemonService binder
-    sendToBridge(VectorService.asBinder(), false, systemServerMaxRetry)
+    sendToBridge(VectorService.asBinder(), false)
 
     if (!isVerboseLog) {
       LogcatMonitor.stopVerbose()
@@ -129,11 +160,7 @@ object VectorDaemon {
 
   // The bridge is setup in `system_server` via Zygisk API
   @Suppress("DEPRECATION")
-  private fun sendToBridge(
-      binder: IBinder,
-      isRestart: Boolean,
-      restartRetry: Int,
-  ) {
+  private fun sendToBridge(binder: IBinder, isRestart: Boolean) {
     check(Looper.myLooper() == Looper.getMainLooper()) {
       "sendToBridge MUST run on the main thread!"
     }
@@ -156,19 +183,29 @@ object VectorDaemon {
               object : IBinder.DeathRecipient {
                 override fun binderDied() {
                   Log.w(TAG, "System Server died! Clearing caches and re-injecting...")
-                  bridgeService.unlinkToDeath(this, 0)
+                  bridgeBinder?.unlinkToDeath(this, 0)
+                  bridgeBinder = null
                   clearSystemCaches()
                   SystemServerService.binderDied() // Cleanup old references
+                  // Nothing will look the invented name up again, so stop holding it.
+                  if (startedAfterSystemServer) SystemServerService.unregisterProxyService()
+                  // Whatever this daemon was started into, the replacement is forked by a zygote
+                  // that already carries our hooks, so it arrives the way one does at boot: it
+                  // looks us up under `serial`, which its predecessor has just freed by dying,
+                  // and it broadcasts a boot of its own that there is no need to replay.
+                  startedAfterSystemServer = false
+                  // And the question of whether a module reached system_server is about the one
+                  // ahead of us, not the one that died with ours inside it.
+                  SystemServerService.systemServerRequested = false
                   // Re-claim the service name immediately to ensure that when system_server
                   // restarts, our proxy is already there for the Zygisk module to find.
-                  ServiceManager.addService(proxyServiceName, SystemServerService)
+                  SystemServerService.registerProxyService(proxyServiceName)
                   ManagerService.guard = null // Remove dead guard
-                  Handler(Looper.getMainLooper()).post {
-                    sendToBridge(binder, true, restartRetry - 1)
-                  }
+                  Handler(Looper.getMainLooper()).post { sendToBridge(binder, true) }
                 }
               }
           bridgeService.linkToDeath(deathRecipient, 0)
+          bridgeBinder = bridgeService // see [bridgeBinder]: the link is only as alive as this
 
           // Try sending the Binder payload (up to 3 times)
           var success = false
@@ -191,9 +228,29 @@ object VectorDaemon {
 
           if (success) {
             Log.i(TAG, "Successfully injected Vector IPC binder for applications.")
+            // Only the name we invented for a late injection: `serial` is a real service name,
+            // and system_server registering it is what takes our proxy out of servicemanager.
+            if (startedAfterSystemServer) SystemServerService.unregisterProxyService()
           } else {
-            Log.e(TAG, "Failed to inject VectorService into system_server")
-            if (restartRetry > 0) restartSystemServer()
+            // system_server is not restarted here. Restarting it has never been shown to
+            // recover an injection: the module's hooks go in during specialization, long
+            // before the bridge that would be restarted over, so a handshake failing this
+            // late is a bug in the injection or the bootstrap. A restart hides that bug
+            // until someone fixes it, which is the argument against keeping one.
+            //
+            // It is not free to keep around either. Every system_server start is a boot to
+            // PackageWatchdog, which escalates a mitigation count that lives on disk and
+            // never goes down, and far enough along that ladder the mitigation stops being
+            // a reboot: the device is sent to recovery to be asked whether to erase itself.
+            //
+            // The two cases are still worth telling apart for whoever reads the log afterwards.
+            // `systemServerRequested` is latched from inside `postServerSpecialize`, so a live
+            // `activity` with nothing behind it means no module arrived at all.
+            if (!SystemServerService.systemServerRequested) {
+              Log.e(TAG, "No Vector module reached system_server; leaving it running")
+            } else {
+              Log.e(TAG, "Vector reached system_server but its bridge never answered")
+            }
           }
         }
         .onFailure { Log.e(TAG, "Error during injecting DaemonService", it) }
@@ -235,25 +292,39 @@ object VectorDaemon {
    *
    * `system_server` is forked from the *primary* zygote, so restarting that is what restarts the
    * framework — on a 64/32 device the primary init service is still called `zygote` (it runs
-   * app_process64) and `zygote_secondary` is the 32-bit one. Restarting the secondary leaves
-   * system_server running, which is right for [restartSystemServer]'s own purpose and wrong for
-   * this one; they are separate functions for that reason.
+   * app_process64) and `zygote_secondary` is the 32-bit one.
    *
-   * Everything on screen dies with it. The caller is expected to have said so first.
+   * Everything on screen dies with it. The caller is expected to have said so first, which is
+   * also what tells this apart from a boot loop — see [forgetDeliberateBoot].
    */
   fun softReboot() {
     Log.w(TAG, "Soft reboot: restarting the primary zygote")
+    forgetDeliberateBoot()
     SystemProperties.set("ctl.restart", "zygote")
   }
 
-  fun restartSystemServer() {
-    Log.w(TAG, "Restarting system_server...")
-    val restartTarget =
-        if (Build.SUPPORTED_64_BIT_ABIS.isNotEmpty() && Build.SUPPORTED_32_BIT_ABIS.isNotEmpty()) {
-          "zygote_secondary"
-        } else {
-          "zygote"
-        }
-    SystemProperties.set("ctl.restart", restartTarget)
+  /**
+   * Takes the boot we are about to cause back out of PackageWatchdog's count.
+   *
+   * It counts every system_server start alike, having no way to tell one that was asked for from
+   * one a crash loop produced, and each time enough of them land close together it raises a
+   * mitigation count that is kept on disk and never lowered. Far enough along that ladder the
+   * mitigation stops being a reboot: the device goes to recovery to be asked whether to erase
+   * itself. A soft reboot does not clear the window count either, because init and its
+   * properties live through one, so deliberate restarts pile up exactly as failures would.
+   *
+   * Zeroing the window count immediately before restarting cancels the single tick we are about
+   * to add and nothing else. A real loop around us starts counting again from here and still
+   * trips, which is the point: we only vouch for the boot we asked for ourselves.
+   */
+  private fun forgetDeliberateBoot() {
+    // Two names for one counter: RescueParty read `sys.rescue_boot_count` directly until the
+    // CrashRecovery mainline module moved it behind generated accessors and onto the
+    // `crashrecovery.` prefix. Both are cleared because the daemon supports both eras, and
+    // writing the name a platform does not use costs nothing.
+    for (property in arrayOf("crashrecovery.rescue_boot_count", "sys.rescue_boot_count")) {
+      runCatching { SystemProperties.set(property, "0") }
+          .onFailure { Log.w(TAG, "Could not clear `$property` before restarting", it) }
+    }
   }
 }

daemon/src/main/kotlin/org/matrix/vector/daemon/VectorService.kt

@@ -59,8 +59,8 @@ object VectorService : IVectorDaemon.Stub() {
     // Initialize OS Observers using Coroutines for the dispatch blocks
     registerReceivers()
 
-    if (VectorDaemon.isLateInject) {
-      Log.i(TAG, "Late injection detected. Forcing boot completed event.")
+    if (VectorDaemon.startedAfterSystemServer) {
+      Log.i(TAG, "Started after system_server. Forcing boot completed event.")
       dispatchBootCompleted()
     }
   }

daemon/src/main/kotlin/org/matrix/vector/daemon/ipc/SystemServerService.kt

@@ -3,6 +3,7 @@ package org.matrix.vector.daemon.ipc
 import android.os.Binder
 import android.os.Build
 import android.os.IBinder
+import android.os.IClientCallback
 import android.os.IServiceCallback
 import android.os.Parcel
 import android.os.ServiceManager
@@ -27,7 +28,15 @@ private const val TAG = "VectorSystemServer"
  */
 private object ServiceRegistrationWatcher {
 
+  // Written from the main thread at startup and from a binder thread when system_server dies.
+  @Volatile private var watchedName: String? = null
+  @Volatile private var unusedName: String? = null
+
   fun watch(serviceName: String) {
+    // servicemanager keeps every callback it is handed, and a system_server restart brings the
+    // daemon back through here; without this a long-lived daemon stacks up duplicates that all
+    // report the same registration.
+    if (watchedName == serviceName) return
     val callback =
         object : IServiceCallback.Stub() {
           // The IServiceCallback will tell us when the real Android service is ready,
@@ -41,8 +50,40 @@ private object ServiceRegistrationWatcher {
           override fun asBinder(): IBinder = this
         }
     runCatching { getSystemServiceManager().registerForNotifications(serviceName, callback) }
+        .onSuccess { watchedName = serviceName }
         .onFailure { Log.e(TAG, "Failed to register IServiceCallback", it) }
   }
+
+  /**
+   * Asks servicemanager to drop [serviceName] once nothing but itself is holding [binder].
+   *
+   * It will not drop a service on request alone. `tryUnregisterService` refuses outright while
+   * the service is marked as having a client on the way, which handing the binder out is what
+   * sets, and the only code that clears that mark returns early for any name without a client
+   * callback — so for a service registered the ordinary way the mark is permanent and the
+   * refusal is forever. Asking repeatedly changes nothing.
+   *
+   * Registering a callback is what puts the name on servicemanager's interval, which then clears
+   * the mark and, once the last client is gone, says so. Unregistering from inside that is how a
+   * lazy service shuts itself down, and it is the only sequence servicemanager accepts.
+   */
+  fun unregisterWhenUnused(serviceName: String, binder: IBinder) {
+    if (unusedName == serviceName) return
+    val callback =
+        object : IClientCallback.Stub() {
+          override fun onClients(registered: IBinder, hasClients: Boolean) {
+            if (hasClients) return
+            runCatching { getSystemServiceManager().tryUnregisterService(serviceName, registered) }
+                .onSuccess { Log.d(TAG, "Dropped the proxy service `$serviceName`") }
+                .onFailure { Log.d(TAG, "Proxy service `$serviceName` stays registered: $it") }
+          }
+
+          override fun asBinder(): IBinder = this
+        }
+    runCatching { getSystemServiceManager().registerClientCallback(serviceName, binder, callback) }
+        .onSuccess { unusedName = serviceName }
+        .onFailure { Log.d(TAG, "Proxy service `$serviceName` stays registered: $it") }
+  }
 }
 
 /**
@@ -59,7 +100,9 @@ object SystemServerService : Binder(), IBinder.DeathRecipient {
   private var proxyServiceName: String? = null
   private var originService: IBinder? = null
 
-  var systemServerRequested = false
+  // Latched from a binder thread when system_server attaches, cleared from another when it dies,
+  // and read on the daemon's main thread.
+  @Volatile var systemServerRequested = false
 
   fun registerProxyService(serviceName: String) {
     // Register as the service name early to setup an IPC for `system_server`.
@@ -80,6 +123,20 @@ object SystemServerService : Binder(), IBinder.DeathRecipient {
         .onFailure { Log.e(TAG, "Failed to register proxy service `$serviceName`", it) }
   }
 
+  /**
+   * Arranges for our proxy to leave servicemanager once the module it was there for is done with
+   * it; see [ServiceRegistrationWatcher.unregisterWhenUnused] for why it cannot simply be asked.
+   *
+   * Worth arranging because the name invented for a late injection is ours alone, so unlike
+   * `serial` nothing will ever come along and replace it. Android R and later; older
+   * servicemanagers offer no way to take a name back, and there it stays registered.
+   */
+  fun unregisterProxyService() {
+    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) return
+    val name = proxyServiceName ?: return
+    ServiceRegistrationWatcher.unregisterWhenUnused(name, this)
+  }
+
   /**
    * Adopts the real system service once servicemanager reports its registration, so that
    * [onTransact] starts forwarding to it. Only ever called from [ServiceRegistrationWatcher].

hiddenapi/stubs/src/main/java/android/os/IClientCallback.java

@@ -0,0 +1,14 @@
+package android.os;
+public interface IClientCallback extends IInterface
+{
+    public static abstract class Stub extends android.os.Binder implements android.os.IClientCallback {
+    }
+    /**
+     * Called when the number of clients of a registered service crosses zero in either
+     * direction, as servicemanager sees it on its own interval.
+     *
+     * @param registered the binder the callback was registered with
+     * @param hasClients whether anything other than servicemanager holds it
+     */
+    public void onClients(android.os.IBinder registered, boolean hasClients) throws android.os.RemoteException;
+}

hiddenapi/stubs/src/main/java/android/os/IServiceManager.java

@@ -6,6 +6,10 @@ public interface IServiceManager extends IInterface {
 
     public void registerForNotifications(String name, IServiceCallback cb);
 
+    public void tryUnregisterService(String name, IBinder service);
+
+    public void registerClientCallback(String name, IBinder service, IClientCallback cb);
+
     abstract class Stub extends Binder implements IServiceManager {
         public static IServiceManager asInterface(IBinder obj) {
             throw new UnsupportedOperationException();

hiddenapi/stubs/src/main/java/android/os/ServiceManager.java

@@ -15,6 +15,17 @@ public static IBinder getService(String name) {
         throw new RuntimeException("STUB");
     }
 
+    /**
+     * Returns a reference to a service with the given name if one is already registered.
+     *
+     * @param name the name of the service to get
+     * @return a reference to the service, or <code>null</code> if the service doesn't exist
+     */
+    @Nullable
+    public static IBinder checkService(String name) {
+        throw new RuntimeException("STUB");
+    }
+
     /**
      * Place a new @a service called @a name into the service
      * manager.