Guard the hook dispatch against re-entering itself

Hooking Object.getClass() made the dispatch call itself. R8 compiles Kotlin's
parameter null checks into obj.getClass(), and one of those is the first
instruction of the trampoline callback, so the first getClass the process ran
after the hook landed re-entered the trampoline, and re-entered again from its
own prologue, until the stack was gone and before any hooker had run. Nothing
recovers from there: lsplant marks a hooked method non-compilable, so the call
site can never be inlined away afterwards, and the framework dex is loaded from
memory and never gets an oat file, so the interpreter reaches the trampoline
every time.

The lowering is new. Built from the same source, 3046 has 16 Object.getClass()
call sites in the framework dex and 3047 has 207; the difference between them is
#792, which moved the build from AGP 8.13.1 to 9.3.1. Reported as #798, where a
module that hooks every method named getClass over Class.getMethods() - a list
that always contains the one inherited from Object - crash-looped com.miui.home
through six process starts. The build it was compared against turned out to be a
branch artefact from before the migration; the version code counts commits on
master rather than on the branch being built, which is why it read as 3047.

The trampoline entry point is native now, so the re-entrancy check runs ahead of
anything a compiler can put in front of it. A Kotlin body cannot promise that,
which is the whole of the bug. While the guard is raised, a hooked method entered
from the framework's own frames runs its original instead of dispatching again.

Where the guard comes down took several attempts on a device to get right, and
the shape that finally works comes from writing VectorChain in Java. The chain is
the surface the API hands to modules, so every method on it is entered from module
code with the guard down, and the guard cannot cover a callee's prologue - in
Kotlin, R8 opens each with a parameter null check compiled into Object.getClass(),
a hookable call ahead of any statement of ours. A hooker that rebuilds its
arguments then re-enters twice per frame and the nesting cap bounds a tree rather
than a chain. The parameter types come from a Java @nonnull interface and cannot
be made nullable, so the only way not to emit the checks is not to write this in
Kotlin; javac emits none.

Being Java, the chain's own bookkeeping calls nothing a module can hook - a
constructor, an array read, two field writes. So it needs no guard of its own: one
lower per node covers both calls that leave the framework, the hooker and the
terminal, and nothing raises. The guard comes down for those two, and for the
original run through an Invoker; it is raised only by the native trampoline and,
for its own bookkeeping, by the legacy bridge.

Because a site that gets this wrong is invisible until a device hangs, the two
sides live in DispatchGuard.kt as callIntoModule and enterFramework, the raw
primitives are named nowhere else, and checkDispatchGuard fails the build if they
are. That check is not sufficient on its own: the Java-facing wrappers in that
same file were at one point recursing into themselves through a SAM conversion,
which reads correctly in source and only shows in the bytecode.

A hooker that calls the method it hooks recurses in module code, where the guard
does not reach, and hooking Object.getClass has the compiler write such calls on
the module's behalf. Past a nesting of thirty-two the thread latches into serving
originals until it unwinds, and names the method once. Latching rather than
re-arming per frame matters for the same reason as above: a hooker that re-enters
more than once per frame would otherwise branch at every level.

Three things follow from the entry point being native. A registration that fails
now refuses the hook, because the alternative is UnsatisfiedLinkError thrown out
of whatever the application was calling, which is far harder to trace back. The
two invoke bridges take the argument array rather than a vararg, since the spread
copied it on every dispatch and the JVM descriptor is the same either way. And
the trampoline's package joins the ones the daemon renames as it loads the dex:
keeping a native method keeps its class name, so R8 stopped renaming it, and it
would otherwise stand as a fixed string in every injected process.

tests/dispatch-guard asserts five properties of dispatch, of which the fourth is
the one that is easy to lose and hard to see: the dispatch must not dispatch its
own internal calls. A dispatch that re-enters itself still returns the right
answers, at a multiple of the cost, until a real workload turns that into an ANR -
so it is asserted by the cap staying silent rather than by any result. Its checks
are chosen for the shapes that reach different parts of the framework rather than
for what any one module does, since three defects here survived a reading of the
bytecode and were only caught by running something shaped differently.

Verified on a Pixel 7a running Android 16, against builds of this branch and of
master without it. Without: the target hangs on the first dispatch after the hook
lands and is killed by an ANR whose trace holds no main thread, which is the
signature reported in #798. With, and with that hook live throughout: 33 results,
one process id, no ANR, and the cap firing once, for the hooker written to recurse
into itself. That covers ordinary, static and constructor hooks, a class
initializer, both Invoker types, a hooker that rebuilds its arguments, a legacy
de.robv hook, a hooker seeing another module's hook, and an Invoker whose original
calls a method hooked elsewhere. An empty pass-through hook measured about ten
percent slower per dispatch than master, from the added native round trip.

Fixes #798.

Author: JingMatrix

daemon/src/main/jni/obfuscation.cpp

@@ -22,11 +22,17 @@ namespace {
 
 std::once_flag init_flag;
 
+// The trampoline's callback is a native method, so R8 keeps its class under its real name the way
+// it already keeps the nativebridge classes - a stable string in every injected process for anyone
+// looking for one. Renaming the package here restores what R8 used to do for it. Nothing resolves
+// this package by name: hook_bridge.cpp receives the hooker class as a jclass and reads its members
+// off that, and every other reference is a type descriptor in the same dex, rewritten with it.
 std::map<std::string, std::string> signatures = {
     {"Lde/robv/android/xposed/", ""},         {"Landroid/app/AndroidApp", ""},
     {"Landroid/content/res/XRes", ""},        {"Landroid/content/res/XModule", ""},
     {"Lio/github/libxposed/api/Xposed", ""},  {"Lorg/matrix/vector/core/", ""},
-    {"Lorg/matrix/vector/nativebridge/", ""}, {"Lorg/matrix/vector/service/", ""},
+    {"Lorg/matrix/vector/impl/hooks/", ""},   {"Lorg/matrix/vector/nativebridge/", ""},
+    {"Lorg/matrix/vector/service/", ""},
 };
 
 jclass class_file_descriptor = nullptr;

legacy/src/main/java/org/matrix/vector/legacy/LegacyDelegateImpl.java

@@ -8,6 +8,7 @@
 import org.matrix.vector.impl.di.LegacyPackageInfo;
 import org.matrix.vector.impl.di.OriginalInvoker;
 import org.matrix.vector.impl.hooks.VectorLegacyCallback;
+import org.matrix.vector.impl.hooks.DispatchGuard;
 import org.matrix.vector.impl.utils.VectorMetaDataReader;
 
 import java.io.File;
@@ -64,26 +65,40 @@ public void onSystemServerLoaded(ClassLoader classLoader) {
 
     @Override
     public Object processLegacyHook(Executable executable, Object thisObject, Object[] args, Object[] legacyHooks, OriginalInvoker invokeOriginal) {
-        VectorLegacyCallback<Executable> callback = new VectorLegacyCallback<>(executable, thisObject, args);
-        XposedBridge.LegacyApiSupport<Executable> legacy = new XposedBridge.LegacyApiSupport<>(callback, legacyHooks);
-
-        legacy.handleBefore();
+        // The bookkeeping here is framework code reached from a hooker, so the guard has to go back
+        // up; it comes down only for the three calls that leave again — the two module callbacks and
+        // the original. Left unguarded, a module hooking anything this path touches makes it call
+        // itself. See #798.
+        return DispatchGuard.intoFramework(() -> {
+            VectorLegacyCallback<Executable> callback = new VectorLegacyCallback<>(executable, thisObject, args);
+            XposedBridge.LegacyApiSupport<Executable> legacy = new XposedBridge.LegacyApiSupport<>(callback, legacyHooks);
+
+            DispatchGuard.intoModule(() -> {
+                legacy.handleBefore();
+                return null;
+            });
 
-        if (!callback.isSkipped()) {
-            try {
-                Object result = invokeOriginal.invoke();
-                callback.setResult(result);
-            } catch (Throwable t) {
-                callback.setThrowable(t);
+            if (!callback.isSkipped()) {
+                DispatchGuard.intoModule(() -> {
+                    try {
+                        callback.setResult(invokeOriginal.invoke());
+                    } catch (Throwable t) {
+                        callback.setThrowable(t);
+                    }
+                    return null;
+                });
             }
-        }
 
-        legacy.handleAfter();
+            DispatchGuard.intoModule(() -> {
+                legacy.handleAfter();
+                return null;
+            });
 
-        if (callback.getThrowable() != null) {
-            sneakyThrow(callback.getThrowable());
-        }
-        return callback.getResult();
+            if (callback.getThrowable() != null) {
+                sneakyThrow(callback.getThrowable());
+            }
+            return callback.getResult();
+        });
     }
 
     @Override

native/src/jni/hook_bridge.cpp

@@ -7,6 +7,7 @@
 #include <lsplant.hpp>
 #include <limits>
 #include <memory>
+#include <mutex>
 #include <shared_mutex>
 #include <vector>
 
@@ -89,6 +90,183 @@ SharedHashMap<jmethodID, std::unique_ptr<HookItem>> hooked_methods;
 
 // Cached JNI method and field IDs for performance.
 jmethodID invoke = nullptr;
+
+/**
+ * Depth of the framework's own hook dispatch on this thread.
+ *
+ * Non-zero means a hooked method was entered from inside the dispatch itself, which must run the
+ * original rather than start a second dispatch. The dispatch cannot promise to avoid hookable
+ * methods, because it does not get to choose which ones it calls: AGP 9's R8 compiles Kotlin's
+ * parameter null checks into Object.getClass(), and one of those lands as the first instruction of
+ * the callback. A module hooking such a method would otherwise make the dispatch re-enter itself
+ * until the stack is gone, before any hooker runs. See #798.
+ *
+ * The counter is read before any Java frame exists, which is why the trampoline entry point is a
+ * native method: nothing a compiler emits can run ahead of it.
+ */
+thread_local uint32_t dispatch_depth = 0;
+
+/**
+ * How deep hook dispatch is nested on this thread, counting only dispatches that actually entered
+ * Java. Unlike dispatch_depth this is never suspended, because it exists to bound the one thing the
+ * guard cannot reach: a hooker that calls the method it hooks, directly or through a call the
+ * compiler generated for it. That recursion lives entirely in module code, runs with the guard
+ * down by design, and would otherwise take the process out with it.
+ *
+ * Thirty-two is deep enough that no chain of one hook calling into another will reach it, and
+ * shallow enough that a runaway costs a small constant instead of thousands of frames. A method
+ * whose hooks genuinely nest deeper than this - a hooked recursive method, say - stops being hooked
+ * past the limit, which is why hitting it is reported.
+ */
+thread_local uint32_t dispatch_nesting = 0;
+constexpr uint32_t kMaxDispatchNesting = 32;
+
+/**
+ * Set when the cap is reached, cleared when the thread unwinds back out of dispatch entirely.
+ *
+ * Without it the cap bounds depth but not work. A hooker that re-enters more than once per frame -
+ * two calls, or one call the compiler wrote twice - branches at every level, so re-arming the cap on
+ * the way back down turns one call into a tree of that many dispatches. Latching instead means the
+ * thread serves originals from the moment it is in trouble until it is out of it.
+ */
+thread_local bool dispatch_degraded = false;
+
+// Bounded so a runaway cannot itself flood the log. Eight is enough to name several distinct
+// offenders in one boot.
+std::atomic<int> runaway_reports{0};
+constexpr int kMaxRunawayReports = 8;
+
+// Resolved from the hooker class the first time a hook is installed. That call is the only place
+// the class is available: the framework dex is repackaged per build, so its name cannot be
+// looked up, and it is also the earliest moment anything can reach the trampoline.
+std::once_flag hooker_init;
+bool hooker_ready = false;
+jmethodID hooker_dispatch = nullptr;
+jfieldID hooker_method = nullptr;
+jfieldID hooker_is_static = nullptr;
+jclass object_class = nullptr;
+jmethodID to_string = nullptr;
+jclass invocation_target_exception = nullptr;
+jmethodID get_cause = nullptr;
+
+/**
+ * @brief Invokes a hooked method's original implementation, or the method itself if unhooked.
+ */
+jobject InvokeBackup(JNIEnv *env, jobject executable, jobject thiz, jobjectArray args) {
+    auto target = env->FromReflectedMethod(executable);
+    HookItem *hook_item = nullptr;
+    hooked_methods.if_contains(target,
+                               [&hook_item](const auto &it) { hook_item = it.second.get(); });
+
+    // If a hook item exists, invoke its backup. Otherwise, invoke the method directly
+    // (though this case should be rare if called from a hook callback).
+    jobject method_to_invoke = hook_item ? hook_item->GetBackup() : executable;
+    if (!method_to_invoke) {
+        // Hooking might have failed or is not complete.
+        return nullptr;
+    }
+    return env->CallObjectMethod(method_to_invoke, invoke, thiz, args);
+}
+
+/**
+ * @brief Serves a trampoline hit that arrived from inside the dispatch, without entering Java.
+ *
+ * Runs the original and hands its exception back unwrapped, the way the call site that the
+ * compiler generated would have seen it. Nothing here may call back into the framework: this path
+ * exists precisely because the framework is already on the stack below it.
+ */
+jobject InvokeOriginalReentrant(JNIEnv *env, jobject hooker, jobjectArray args) {
+    jobject executable = env->GetObjectField(hooker, hooker_method);
+    jobject receiver = nullptr;
+    jobjectArray actual = args;
+
+    if (!env->GetBooleanField(hooker, hooker_is_static)) {
+        receiver = env->GetObjectArrayElement(args, 0);
+        const jsize count = env->GetArrayLength(args) - 1;
+        actual = env->NewObjectArray(count, object_class, nullptr);
+        for (jsize i = 0; i < count; ++i) {
+            jobject arg = env->GetObjectArrayElement(args, i + 1);
+            env->SetObjectArrayElement(actual, i, arg);
+            env->DeleteLocalRef(arg);
+        }
+    }
+
+    jobject result = InvokeBackup(env, executable, receiver, actual);
+
+    // Method.invoke reports whatever the target threw wrapped; the caller here is ordinary code
+    // that never went through reflection, so hand it the cause instead.
+    if (jthrowable thrown = env->ExceptionOccurred(); thrown) {
+        env->ExceptionClear();
+        jthrowable to_throw = thrown;
+        if (env->IsInstanceOf(thrown, invocation_target_exception)) {
+            if (auto cause = env->CallObjectMethod(thrown, get_cause); cause) {
+                to_throw = static_cast<jthrowable>(cause);
+            }
+        }
+        env->Throw(to_throw);
+        return nullptr;
+    }
+    return result;
+}
+
+/**
+ * @brief Names the method whose hooks stopped nesting, at most a few times per process.
+ *
+ * Reads the executable and asks it for its name only here, on the rare path, so the dispatch does
+ * not pay for a diagnostic it almost never emits. The guard is raised for the duration because
+ * Executable.toString() is ordinary Java and may itself touch whatever the module hooked.
+ */
+void ReportRunaway(JNIEnv *env, jobject hooker) {
+    if (runaway_reports.fetch_add(1, std::memory_order_relaxed) >= kMaxRunawayReports) return;
+
+    ++dispatch_depth;
+    jobject executable = env->GetObjectField(hooker, hooker_method);
+    auto name = static_cast<jstring>(env->CallObjectMethod(executable, to_string));
+    if (env->ExceptionCheck()) {
+        env->ExceptionClear();
+    } else if (name) {
+        if (const char *chars = env->GetStringUTFChars(name, nullptr); chars) {
+            LOGE(
+                "Hook dispatch nested past {} for {}; running the original instead. A hooker that "
+                "calls the method it hooks recurses into itself, and the compiler emits such calls "
+                "on its behalf - a null check becomes Object.getClass(), for one.",
+                kMaxDispatchNesting, chars);
+            env->ReleaseStringUTFChars(name, chars);
+        }
+    }
+    if (name) env->DeleteLocalRef(name);
+    env->DeleteLocalRef(executable);
+    --dispatch_depth;
+}
+
+/**
+ * @brief The trampoline entry point, registered onto VectorNativeHooker.callback.
+ *
+ * Native so that the re-entrancy check is genuinely first: a Kotlin body would let the compiler
+ * put its own prologue ahead of it, which is the whole of #798.
+ */
+jobject HookerCallback(JNIEnv *env, jobject hooker, jobjectArray args) {
+    if (dispatch_depth != 0) {
+        return InvokeOriginalReentrant(env, hooker, args);
+    }
+
+    if (dispatch_degraded || dispatch_nesting >= kMaxDispatchNesting) {
+        if (!dispatch_degraded) {
+            dispatch_degraded = true;
+            ReportRunaway(env, hooker);
+        }
+        return InvokeOriginalReentrant(env, hooker, args);
+    }
+
+    ++dispatch_nesting;
+    ++dispatch_depth;
+    jobject result = env->CallObjectMethod(hooker, hooker_dispatch, args);
+    // Restored on the exception path too: CallObjectMethod returns with the exception pending, and
+    // leaving either counter raised would silently disable every hook on this thread from here on.
+    --dispatch_depth;
+    if (--dispatch_nesting == 0) dispatch_degraded = false;
+    return result;
+}
 }  // namespace
 
 namespace vector::native::jni {
@@ -121,6 +299,45 @@ VECTOR_DEF_NATIVE_METHOD(jboolean, HookBridge, hookMethod, jboolean useModernApi
     } finally{.newHook = newHook};
 #endif
 
+    // The hooker class arrives only here, and this call is also the earliest moment anything can
+    // reach its trampoline, so bind the native entry point and the members it reads now.
+    std::call_once(hooker_init, [env, hooker] {
+        static const JNINativeMethod entry[] = {
+            {"callback", "([Ljava/lang/Object;)Ljava/lang/Object;",
+             VECTOR_JNI_CAST(void *)(HookerCallback)},
+        };
+        if (env->RegisterNatives(hooker, entry, 1) != JNI_OK) {
+            LOGF("Cannot register the hook trampoline entry point");
+            return;
+        }
+        hooker_dispatch =
+            env->GetMethodID(hooker, "dispatch", "([Ljava/lang/Object;)Ljava/lang/Object;");
+        hooker_method = env->GetFieldID(hooker, "method", "Ljava/lang/reflect/Executable;");
+        hooker_is_static = env->GetFieldID(hooker, "isStatic", "Z");
+
+        auto object = env->FindClass("java/lang/Object");
+        object_class = static_cast<jclass>(env->NewGlobalRef(object));
+        to_string = env->GetMethodID(object, "toString", "()Ljava/lang/String;");
+        env->DeleteLocalRef(object);
+
+        auto ite = env->FindClass("java/lang/reflect/InvocationTargetException");
+        invocation_target_exception = static_cast<jclass>(env->NewGlobalRef(ite));
+        env->DeleteLocalRef(ite);
+
+        auto throwable = env->FindClass("java/lang/Throwable");
+        get_cause = env->GetMethodID(throwable, "getCause", "()Ljava/lang/Throwable;");
+        env->DeleteLocalRef(throwable);
+
+        hooker_ready = hooker_dispatch && hooker_method && hooker_is_static && object_class &&
+                       to_string && invocation_target_exception && get_cause;
+        if (!hooker_ready) LOGF("Cannot resolve the hook trampoline members");
+    });
+
+    // Refusing the hook is the only honest outcome: the trampoline would land on an unregistered
+    // native method and throw UnsatisfiedLinkError out of whatever the app was calling, which is
+    // a great deal harder to trace back than a module being told its hook did not take.
+    if (!hooker_ready) return JNI_FALSE;
+
     auto target = env->FromReflectedMethod(hookMethod);
     HookItem *hook_item = nullptr;
 
@@ -215,19 +432,39 @@ VECTOR_DEF_NATIVE_METHOD(jboolean, HookBridge, deoptimizeMethod, jobject hookMet
  */
 VECTOR_DEF_NATIVE_METHOD(jobject, HookBridge, invokeOriginalMethod, jobject hookMethod,
                          jobject thiz, jobjectArray args) {
-    auto target = env->FromReflectedMethod(hookMethod);
-    HookItem *hook_item = nullptr;
-    hooked_methods.if_contains(target,
-                               [&hook_item](const auto &it) { hook_item = it.second.get(); });
+    return InvokeBackup(env, hookMethod, thiz, args);
+}
 
-    // If a hook item exists, invoke its backup. Otherwise, invoke the method directly
-    // (though this case should be rare if called from a hook callback).
-    jobject method_to_invoke = hook_item ? hook_item->GetBackup() : hookMethod;
-    if (!method_to_invoke) {
-        // Hooking might have failed or is not complete.
-        return nullptr;
-    }
-    return env->CallObjectMethod(method_to_invoke, invoke, thiz, args);
+/**
+ * @brief Lowers the dispatch guard for the duration of a call into module code.
+ *
+ * A hooker, and the original method the chain ends in, are entitled to a full dispatch of whatever
+ * they call; only the framework's own bookkeeping must not re-enter. The previous depth is
+ * returned rather than assumed to be one, so nesting restores exactly what it found.
+ */
+VECTOR_DEF_NATIVE_METHOD(jint, HookBridge, suspendDispatch) {
+    const auto saved = dispatch_depth;
+    dispatch_depth = 0;
+    return static_cast<jint>(saved);
+}
+
+/** @brief Restores the depth returned by suspendDispatch. */
+VECTOR_DEF_NATIVE_METHOD(void, HookBridge, resumeDispatch, jint depth) {
+    dispatch_depth = static_cast<uint32_t>(depth);
+}
+
+/**
+ * @brief Raises the guard over a stretch of framework code, returning the depth to restore.
+ *
+ * The chain is re-entered from module code, with the guard already down, so it has to raise the
+ * guard for its own bookkeeping rather than assume it holds. Allocating one chain node calls
+ * getClass twice - R8's null checks on the constructor's parameters - and without this each of
+ * those would dispatch, allocate another node, and call getClass again.
+ */
+VECTOR_DEF_NATIVE_METHOD(jint, HookBridge, raiseDispatch) {
+    const auto saved = dispatch_depth;
+    dispatch_depth = 1;
+    return static_cast<jint>(saved);
 }
 
 /**
@@ -691,6 +928,9 @@ static JNINativeMethod gMethods[] = {
                          "Executable;)[[Ljava/lang/Object;"),
     VECTOR_NATIVE_METHOD(HookBridge, findStaticInitializer,
                          "(Ljava/lang/Class;[JJ)Ljava/lang/reflect/Executable;"),
+    VECTOR_NATIVE_METHOD(HookBridge, suspendDispatch, "()I"),
+    VECTOR_NATIVE_METHOD(HookBridge, resumeDispatch, "(I)V"),
+    VECTOR_NATIVE_METHOD(HookBridge, raiseDispatch, "()I"),
 };
 
 /**

tests/dispatch-guard/.gitignore

@@ -0,0 +1,3 @@
+build/
+.gradle/
+local.properties