From 72863a6874dd5a664b121e20e1e64695e8d6f579 Mon Sep 17 00:00:00 2001 From: JingMatrix Date: Thu, 30 Jul 2026 08:36:09 +0200 Subject: [PATCH] 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. 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. --- daemon/src/main/jni/obfuscation.cpp | 8 +- .../vector/legacy/LegacyDelegateImpl.java | 47 ++-- native/src/jni/hook_bridge.cpp | 264 +++++++++++++++++- xposed/README.md | 10 +- xposed/build.gradle.kts | 34 +++ xposed/consumer-rules.pro | 8 +- .../matrix/vector/impl/hooks/BaseInvoker.kt | 33 ++- .../matrix/vector/impl/hooks/DispatchGuard.kt | 93 ++++++ .../matrix/vector/impl/hooks/VectorChain.java | 190 +++++++++++++ .../matrix/vector/impl/hooks/VectorChain.kt | 133 --------- .../vector/impl/hooks/VectorHookRecord.kt | 26 ++ .../vector/impl/hooks/VectorNativeHooker.kt | 46 ++- .../matrix/vector/nativebridge/HookBridge.kt | 37 ++- 13 files changed, 737 insertions(+), 192 deletions(-) create mode 100644 xposed/src/main/kotlin/org/matrix/vector/impl/hooks/DispatchGuard.kt create mode 100644 xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.java delete mode 100644 xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.kt create mode 100644 xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorHookRecord.kt diff --git a/daemon/src/main/jni/obfuscation.cpp b/daemon/src/main/jni/obfuscation.cpp index d6ecc83fa..1e4b14d58 100644 --- a/daemon/src/main/jni/obfuscation.cpp +++ b/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 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; diff --git a/legacy/src/main/java/org/matrix/vector/legacy/LegacyDelegateImpl.java b/legacy/src/main/java/org/matrix/vector/legacy/LegacyDelegateImpl.java index 885d40878..58726cf4e 100644 --- a/legacy/src/main/java/org/matrix/vector/legacy/LegacyDelegateImpl.java +++ b/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 callback = new VectorLegacyCallback<>(executable, thisObject, args); - XposedBridge.LegacyApiSupport 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 callback = new VectorLegacyCallback<>(executable, thisObject, args); + XposedBridge.LegacyApiSupport 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 diff --git a/native/src/jni/hook_bridge.cpp b/native/src/jni/hook_bridge.cpp index 2cbae6777..61dcc7db3 100644 --- a/native/src/jni/hook_bridge.cpp +++ b/native/src/jni/hook_bridge.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -89,6 +90,183 @@ SharedHashMap> 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 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(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(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(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(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(saved); +} + +/** @brief Restores the depth returned by suspendDispatch. */ +VECTOR_DEF_NATIVE_METHOD(void, HookBridge, resumeDispatch, jint depth) { + dispatch_depth = static_cast(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(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"), }; /** diff --git a/xposed/README.md b/xposed/README.md index 9c652d733..1f03d24be 100644 --- a/xposed/README.md +++ b/xposed/README.md @@ -4,7 +4,7 @@ This module implements the [libxposed](https://github.com/libxposed/api) API for ## Architectural Overview -The `xposed` module is designed with strict boundaries to ensure stability during the Android boot process and application lifecycles. It is written entirely in Kotlin and operates independently of the legacy Xposed API (`de.robv.android.xposed`). +The `xposed` module is designed with strict boundaries to ensure stability during the Android boot process and application lifecycles. It is written in Kotlin — with one deliberate exception, `VectorChain`, in Java for the reason given in its own header — and operates independently of the legacy Xposed API (`de.robv.android.xposed`). It defines a Dependency Injection (DI) contract (`LegacyFrameworkDelegate`) which the `legacy` module must implement and inject during startup. ## Core Components @@ -12,10 +12,14 @@ It defines a Dependency Injection (DI) contract (`LegacyFrameworkDelegate`) whic ### 1. The Hooking Engine * **`VectorHookBuilder`**: Implements the `HookBuilder` API. It validates the target `Executable`, bundles the module's `Hooker`, `priority`, and `ExceptionMode` into a `VectorHookRecord`, and registers it natively via JNI. -* **`VectorNativeHooker`**: The JNI trampoline target. When a hooked method is executed, the C++ layer invokes `callback(Array)` on this class. It fetches the active hooks (both modern and legacy) from the native registry as global `jobject` references, constructs the root `VectorChain`, and initiates execution. -* **`VectorChain`**: Implements the recursive `proceed()` state machine. +* **`VectorNativeHooker`**: The JNI trampoline target. `callback(Array)` is a `native` method implemented in `hook_bridge.cpp`; it raises the dispatch guard and calls `dispatch`, which fetches the active hooks (both modern and legacy) from the native registry as global `jobject` references, constructs the root `VectorChain`, and initiates execution. +* **`VectorChain`**: Implements the recursive `proceed()` state machine. It is the surface handed to modules, so it is written in Java on purpose: every method is entered from module code, and in Kotlin R8 would open each with a parameter null check compiled into `Object.getClass()` — a hookable call ahead of any statement of ours. Being Java, its bookkeeping calls nothing a module can hook, so it lowers the dispatch guard exactly once per node, covering both the hooker and the terminal, and raises nothing. * **Exception Handling**: It implements the logic for `ExceptionMode`. In `PROTECTIVE` mode, if an interceptor throws an exception *before* calling `proceed()`, the chain skips the interceptor. If it throws *after* calling `proceed()`, the chain catches the exception and restores the cached downstream result/throwable to protect the host process. +* **`DispatchGuard`**: A per-thread flag answering one question — is the code running now ours, or a module's? While it is raised, a hooked method entered from the framework's own frames runs its original rather than dispatching again. This is not optional: R8 compiles Kotlin's parameter null checks into `Object.getClass()`, so the dispatch calls methods a module may hook whether or not anyone wrote those calls, and a hook on one of them would otherwise make the dispatch call itself. + + The flag is raised by the native trampoline before any Java frame exists, and lowered around the two calls that leave the framework: a `Hooker`, and the original method (with the legacy before/after callbacks around it). Because `VectorChain` is Java it needs no protection of its own, so a whole node is one lower and no raise. `callIntoModule` and `enterFramework` in `DispatchGuard.kt` are the only ways to move the flag; `:xposed:checkDispatchGuard` fails the build if the underlying primitives are named anywhere else. Recursion that lives in module code is out of the guard's reach and is bounded instead by a nesting cap, which reports the method and runs its original. `tests/dispatch-guard` asserts all of this on a device. + ### 2. The Invocation System The `Invoker` system allows modules to execute methods while bypassing standard JVM access checks, with granular control over hook execution. diff --git a/xposed/build.gradle.kts b/xposed/build.gradle.kts index ef5cee53c..00a27f7f5 100644 --- a/xposed/build.gradle.kts +++ b/xposed/build.gradle.kts @@ -8,6 +8,40 @@ plugins { ktfmt { kotlinLangStyle() } +// The dispatch guard is a thread-local flag whose correctness lives entirely in where it is raised +// and lowered, and a site that gets it wrong is invisible until a device hangs — which is how #798 +// took two rounds to fix. Keep the primitives inside DispatchGuard.kt so the invariant is one file. +val checkDispatchGuard by + tasks.registering { + val sources = + listOf(rootProject.file("xposed/src/main"), rootProject.file("legacy/src/main")) + inputs.files(sources.map { fileTree(it) }) + outputs.upToDateWhen { true } + doLast { + val offenders = + sources + .flatMap { fileTree(it).files } + .filter { it.extension == "kt" || it.extension == "java" } + .filter { it.name != "DispatchGuard.kt" && it.name != "HookBridge.kt" } + .filter { f -> + f.readLines().any { + it.contains("suspendDispatch") || + it.contains("resumeDispatch") || + it.contains("raiseDispatch") + } + } + if (offenders.isNotEmpty()) { + throw GradleException( + "The dispatch guard primitives belong to DispatchGuard.kt; call " + + "callIntoModule/enterFramework instead. Offenders: " + + offenders.joinToString { it.name } + ) + } + } + } + +tasks.matching { it.name.startsWith("assemble") }.configureEach { dependsOn(checkDispatchGuard) } + android { namespace = "org.matrix.vector.impl" diff --git a/xposed/consumer-rules.pro b/xposed/consumer-rules.pro index d18731d55..efb6da963 100644 --- a/xposed/consumer-rules.pro +++ b/xposed/consumer-rules.pro @@ -6,8 +6,14 @@ native ; } -# Preserve the JNI Hook Trampoline +# Preserve the JNI Hook Trampoline. hook_bridge.cpp resolves every one of these by name off the +# hooker class the first time a hook is installed: callback is the native entry point it registers, +# dispatch is what that entry point calls once the re-entrancy guard is up, and the two fields are +# what the guarded path reads to reach the original without entering Java at all. -keepclassmembers class org.matrix.vector.impl.hooks.VectorNativeHooker { public (java.lang.reflect.Executable); public java.lang.Object callback(java.lang.Object[]); + public java.lang.Object dispatch(java.lang.Object[]); + public java.lang.reflect.Executable method; + public boolean isStatic; } diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/BaseInvoker.kt b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/BaseInvoker.kt index a8f8e2564..bf8bf3378 100644 --- a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/BaseInvoker.kt +++ b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/BaseInvoker.kt @@ -47,14 +47,16 @@ internal abstract class BaseInvoker, U : Executable>( val filteredHooks = allModernHooks.filter { it.priority <= currentType.maxPriority }.toTypedArray() - val terminal: (Any?, Array) -> Any? = { tObj, tArgs -> + // No guard of its own; see VectorNativeHooker. + val terminal = VectorTerminal { tObj, tArgs -> + val actual = tArgs ?: emptyArray() val delegate = VectorBootstrap.delegate if (legacyHooks.isNotEmpty() && delegate != null) { - delegate.processLegacyHook(executable, tObj, tArgs, legacyHooks) { - invokeOriginal(tObj, tArgs) + delegate.processLegacyHook(executable, tObj, actual, legacyHooks) { + invokeOriginal(tObj, actual) } } else { - invokeOriginal(tObj, tArgs) + invokeOriginal(tObj, actual) } } @@ -75,8 +77,15 @@ internal abstract class BaseInvoker, U : Executable>( } } - /** Invokes the original executable, reporting a target exception as Method#invoke does. */ - private fun dispatchOriginal(thisObject: Any?, args: Array): Any? { + /** + * Invokes the original executable, reporting a target exception as Method#invoke does. + * + * Lowered here rather than at the call sites: proceedInvocation raises the guard for its own + * bookkeeping, and two of its paths reach this directly. Running the original with the guard up + * would hand it a whole call tree in which no module's hooks fire, silently and with a plausible + * return value. Type.Origin skips the hooks on *this* executable, not on everything it calls. + */ + private fun dispatchOriginal(thisObject: Any?, args: Array): Any? = callIntoModule { // invokeOriginalMethod dispatches through a cached Method.invoke id. For a hooked // executable that is applied to lsplant's backup Method, which is correct. For an // executable with no hook item at all it is applied to the reflected object we passed in @@ -89,15 +98,15 @@ internal abstract class BaseInvoker, U : Executable>( requireNotNull(thisObject) { "A constructor invoked as a method needs a receiver: $executable" } - return HookBridge.invokeSpecialMethod( + return@callIntoModule HookBridge.invokeSpecialMethod( executable, getExecutableShorty(), executable.declaringClass, thisObject, - *args, + args, ) } - return HookBridge.invokeOriginalMethod(executable, thisObject, *args) + HookBridge.invokeOriginalMethod(executable, thisObject, args) } /** @@ -151,7 +160,7 @@ internal class VectorMethodInvoker(method: Method) : getExecutableShorty(), executable.declaringClass, thisObject, - *args, + args, ) } } @@ -175,7 +184,7 @@ internal class VectorCtorInvoker(constructor: Constructor) : getExecutableShorty(), executable.declaringClass, thisObject, - *args, + args, ) return null } @@ -202,7 +211,7 @@ internal class VectorCtorInvoker(constructor: Constructor) : getExecutableShorty(), executable.declaringClass, obj, - *args, + args, ) return obj } diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/DispatchGuard.kt b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/DispatchGuard.kt new file mode 100644 index 000000000..29406c43b --- /dev/null +++ b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/DispatchGuard.kt @@ -0,0 +1,93 @@ +package org.matrix.vector.impl.hooks + +import org.matrix.vector.nativebridge.HookBridge + +/** + * The two sides of the dispatch guard, and the only place that touches it. + * + * The guard answers one question: is the code running right now ours, or someone else's? While the + * answer is "ours", a hooked method entered from here runs its original instead of dispatching + * again — which is what stops the dispatch from calling itself when a module hooks something the + * dispatch happens to call. It does not get to choose what that is: since AGP 9, R8 compiles + * Kotlin's parameter null checks into Object.getClass(), so any non-private method of ours opens + * with a call a module is allowed to hook. See #798. + * + * The guard is raised by the trampoline before any Java frame exists, so the default answer is + * "ours". Keeping it correct is then a matter of marking the two kinds of crossing, and the reason + * they live here rather than at each site is that getting one wrong is invisible until a device + * hangs: this went through two rounds of exactly that. Nothing outside this file may name + * [HookBridge.suspendDispatch], [HookBridge.resumeDispatch] or [HookBridge.raiseDispatch], and + * `:xposed:checkDispatchGuard` fails the build if it does. + * + * There are four crossings into module code and no others: a hooker, a module lifecycle callback, + * a legacy before/after callback, and the original method the chain ends in. + */ +internal inline fun callIntoModule(block: () -> R): R { + val depth = HookBridge.suspendDispatch() + try { + return block() + } finally { + HookBridge.resumeDispatch(depth) + } +} + +/** + * Marks framework code that module code can call into — the API surface handed to modules, and + * anything reached from a hooker. + * + * These entry points are reached with the guard already down, so they have to put it back up rather + * than assume it holds. + */ +internal inline fun enterFramework(block: () -> R): R { + val depth = HookBridge.raiseDispatch() + try { + return block() + } finally { + HookBridge.resumeDispatch(depth) + } +} + +/** + * The same two, for the legacy bridge, which is Java and cannot use the inline forms. + * + * Named apart from them on purpose. Sharing the name made the lambda SAM-convert to [Body] and the + * member overload win over the top-level function, so each of these called itself: the primitives + * were never reached, and every legacy hook died of a StackOverflowError on its first invocation. + * The source read correctly; only the bytecode showed it. + */ +object DispatchGuard { + + fun interface Body { + fun run(): R + } + + /** + * The bare primitives, for Java code on the dispatch path that cannot afford the lambda a + * [Body] would allocate on every call. Pair each with [restore] in a finally. + */ + @JvmStatic fun raise(): Int = HookBridge.raiseDispatch() + + @JvmStatic fun lower(): Int = HookBridge.suspendDispatch() + + @JvmStatic fun restore(depth: Int) = HookBridge.resumeDispatch(depth) + + @JvmStatic + fun intoModule(body: Body): R { + val depth = HookBridge.suspendDispatch() + try { + return body.run() + } finally { + HookBridge.resumeDispatch(depth) + } + } + + @JvmStatic + fun intoFramework(body: Body): R { + val depth = HookBridge.raiseDispatch() + try { + return body.run() + } finally { + HookBridge.resumeDispatch(depth) + } + } +} diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.java b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.java new file mode 100644 index 000000000..9ca921785 --- /dev/null +++ b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.java @@ -0,0 +1,190 @@ +package org.matrix.vector.impl.hooks; + +import io.github.libxposed.api.XposedInterface; + +import org.lsposed.lspd.util.Utils; + +import java.lang.reflect.Executable; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +/** + * Core interceptor chain engine. Manages recursive hook execution and enforces + * {@link XposedInterface.ExceptionMode} protections. + * + *

Java rather than Kotlin, and deliberately so. This is the surface the API hands to modules, so + * every method here is entered from module code with the dispatch guard down, and the guard cannot + * cover a callee's prologue. In Kotlin, R8 opens each of these with a null check on the parameters, + * compiled since AGP 9 into {@code Object.getClass()} — a call a module is allowed to hook, made + * before any statement of ours can raise the guard. 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 {@code @NonNull} Java interface and cannot be made nullable, so the only way to + * not emit the checks was to not write this in Kotlin. javac emits none. See #798. + */ +public final class VectorChain implements XposedInterface.Chain { + + private final Executable executable; + private final Object thisObj; + private final Object[] args; + private final VectorHookRecord[] hooks; + private final int hookIndex; + private final VectorTerminal terminal; + + /** Whether this node has forwarded execution downstream. */ + boolean proceedCalled = false; + + /** The outcome of the rest of the chain, cached so a parent can recover it. */ + Object downstreamResult = null; + Throwable downstreamThrowable = null; + + public VectorChain( + Executable executable, + Object thisObj, + Object[] args, + VectorHookRecord[] hooks, + int hookIndex, + VectorTerminal terminal) { + this.executable = executable; + this.thisObj = thisObj; + this.args = args; + this.hooks = hooks; + this.hookIndex = hookIndex; + this.terminal = terminal; + } + + @Override + public Executable getExecutable() { + return executable; + } + + @Override + public Object getThisObject() { + return thisObj; + } + + /** + * Immutable, and a snapshot rather than a view: the chain rewrites the argument array in place + * when a hooker calls proceed(args) and when a legacy hook edits its arguments, which would + * otherwise change a list a hooker is still holding. + */ + @Override + public List getArgs() { + return Collections.unmodifiableList(Arrays.asList(args.clone())); + } + + @Override + public Object getArg(int index) { + return args[index]; + } + + @Override + public Object proceed() throws Throwable { + return internalProceed(thisObj, args); + } + + @Override + public Object proceed(Object[] currentArgs) throws Throwable { + return internalProceed(thisObj, currentArgs); + } + + @Override + public Object proceedWith(Object thisObject) throws Throwable { + return internalProceed(thisObject, args); + } + + @Override + public Object proceedWith(Object thisObject, Object[] currentArgs) throws Throwable { + return internalProceed(thisObject, currentArgs); + } + + /** + * Lowers the guard for everything below it, and raises nothing. + * + * Being Java, the bookkeeping here calls nothing a module can hook: a constructor, an array + * read, two field writes. So it needs no protection of its own, and one lower covers both calls + * that leave the framework — the hooker, and the terminal that reaches the original. + */ + private Object internalProceed(Object thisObject, Object[] currentArgs) throws Throwable { + int outer = DispatchGuard.lower(); + try { + proceedCalled = true; + + // Reached the end of the modern hooks; run the original (and the legacy hooks). + if (hookIndex >= hooks.length) { + try { + Object result = terminal.run(thisObject, currentArgs); + downstreamResult = result; + downstreamThrowable = null; + return result; + } catch (Throwable t) { + downstreamResult = null; + downstreamThrowable = t; + throw t; + } + } + + VectorHookRecord record = hooks[hookIndex]; + VectorChain nextChain = + new VectorChain( + executable, thisObject, currentArgs, hooks, hookIndex + 1, terminal); + + try { + Object result = record.getHooker().intercept(nextChain); + downstreamResult = result; + downstreamThrowable = null; + return result; + } catch (Throwable t) { + // Recording the recovery keeps this node's cached state consistent: once the + // hooker's exception has been suppressed, parent nodes must observe the recovered + // outcome and not the exception we just swallowed. + try { + Object result = + handleInterceptorException( + t, record, nextChain, thisObject, currentArgs); + downstreamResult = result; + downstreamThrowable = null; + return result; + } catch (Throwable t2) { + downstreamResult = null; + downstreamThrowable = t2; + throw t2; + } + } + } finally { + DispatchGuard.restore(outer); + } + } + + /** Handles exceptions thrown by a hooker according to its ExceptionMode. */ + private Object handleInterceptorException( + Throwable t, + VectorHookRecord record, + VectorChain nextChain, + Object recoveryThis, + Object[] recoveryArgs) + throws Throwable { + // The exception came from downstream — a lower hook or the original method. + if (nextChain.proceedCalled && t == nextChain.downstreamThrowable) { + throw t; + } + + // Passthrough mode does not rescue the process from hooker crashes. + if (record.getExceptionMode() == XposedInterface.ExceptionMode.PASSTHROUGH) { + throw t; + } + + String hookerName = record.getHooker().getClass().getName(); + if (!nextChain.proceedCalled) { + // Crashed before calling proceed(); skip the hooker and continue the chain. + Utils.logD("Hooker [" + hookerName + "] crashed before proceed. Skipping.", t); + return nextChain.internalProceed(recoveryThis, recoveryArgs); + } + // Crashed after calling proceed(); suppress it and restore the downstream state. + Utils.logD("Hooker [" + hookerName + "] crashed after proceed. Restoring state.", t); + if (nextChain.downstreamThrowable != null) { + throw nextChain.downstreamThrowable; + } + return nextChain.downstreamResult; + } +} diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.kt b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.kt deleted file mode 100644 index 8867c52b8..000000000 --- a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorChain.kt +++ /dev/null @@ -1,133 +0,0 @@ -package org.matrix.vector.impl.hooks - -import io.github.libxposed.api.XposedInterface -import io.github.libxposed.api.XposedInterface.Chain -import io.github.libxposed.api.XposedInterface.ExceptionMode -import java.lang.reflect.Executable -import java.util.Collections -import org.lsposed.lspd.util.Utils - -/** Represents a registered hook configuration, stored natively by [HookBridge]. */ -data class VectorHookRecord( - val hooker: XposedInterface.Hooker, - val priority: Int, - val exceptionMode: ExceptionMode, -) - -/** - * Core interceptor chain engine. Manages recursive hook execution and enforces [ExceptionMode] - * protections. - */ -class VectorChain( - private val executable: Executable, - private val thisObj: Any?, - private val args: Array, - private val hooks: Array, - private val hookIndex: Int, - private val terminal: (thisObj: Any?, args: Array) -> Any?, -) : Chain { - - // Tracks if this specific chain node has forwarded execution downstream - internal var proceedCalled: Boolean = false - private set - - // Stores the actual result/exception from the rest of the chain/original method - internal var downstreamResult: Any? = null - internal var downstreamThrowable: Throwable? = null - - override fun getExecutable(): Executable = executable - - override fun getThisObject(): Any? = thisObj - - // Immutable, and a snapshot rather than a view: the chain rewrites this array in place when a - // hooker calls proceed(args) and when a legacy hook edits its arguments, which would otherwise - // change a list a hooker is still holding. - override fun getArgs(): List = Collections.unmodifiableList(args.toMutableList()) - - override fun getArg(index: Int): Any? = args[index] - - override fun proceed(): Any? = internalProceed(thisObj, args) - - override fun proceed(currentArgs: Array): Any? = internalProceed(thisObj, currentArgs) - - override fun proceedWith(thisObject: Any): Any? = internalProceed(thisObject, args) - - override fun proceedWith(thisObject: Any, currentArgs: Array): Any? = - internalProceed(thisObject, currentArgs) - - private fun internalProceed(thisObject: Any?, currentArgs: Array): Any? { - proceedCalled = true - - // Reached the end of the modern hooks; trigger the original executable (and legacy hooks) - if (hookIndex >= hooks.size) { - return executeDownstream { terminal(thisObject, currentArgs) } - } - - val record = hooks[hookIndex] - val nextChain = - VectorChain(executable, thisObject, currentArgs, hooks, hookIndex + 1, terminal) - - return try { - executeDownstream { record.hooker.intercept(nextChain) } - } catch (t: Throwable) { - // Recording the recovery keeps this node's cached state consistent: once the hooker's - // exception has been suppressed, parent nodes must observe the recovered outcome and - // not the exception we just swallowed. - executeDownstream { - handleInterceptorException(t, record, nextChain, thisObject, currentArgs) - } - } - } - - /** - * Executes the block and caches the downstream state so parent chains can recover it if the - * current interceptor crashes during post-processing. - * - * Exactly one of [downstreamResult] and [downstreamThrowable] is meaningful after this returns, - * so both are always written; leaving a stale value behind would let a parent node resurrect an - * exception this node already handled. - */ - private inline fun executeDownstream(block: () -> Any?): Any? { - return try { - val result = block() - downstreamResult = result - downstreamThrowable = null - result - } catch (t: Throwable) { - downstreamResult = null - downstreamThrowable = t - throw t - } - } - - /** Handles exceptions thrown by a hooker according to its [ExceptionMode]. */ - private fun handleInterceptorException( - t: Throwable, - record: VectorHookRecord, - nextChain: VectorChain, - recoveryThis: Any?, - recoveryArgs: Array, - ): Any? { - // Check if the exception originated from downstream (lower hooks or original method) - if (nextChain.proceedCalled && t === nextChain.downstreamThrowable) { - throw t - } - - // Passthrough mode does not rescue the process from hooker crashes - if (record.exceptionMode == ExceptionMode.PASSTHROUGH) { - throw t - } - - val hookerName = record.hooker.javaClass.name - if (!nextChain.proceedCalled) { - // Crash occurred before calling proceed(); skip hooker and continue the chain - Utils.logD("Hooker [$hookerName] crashed before proceed. Skipping.", t) - return nextChain.internalProceed(recoveryThis, recoveryArgs) - } else { - // Crash occurred after calling proceed(); suppress and restore downstream state - Utils.logD("Hooker [$hookerName] crashed after proceed. Restoring state.", t) - nextChain.downstreamThrowable?.let { throw it } - return nextChain.downstreamResult - } - } -} diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorHookRecord.kt b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorHookRecord.kt new file mode 100644 index 000000000..720a5408e --- /dev/null +++ b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorHookRecord.kt @@ -0,0 +1,26 @@ +package org.matrix.vector.impl.hooks + +import io.github.libxposed.api.XposedInterface +import io.github.libxposed.api.XposedInterface.ExceptionMode + +/** Represents a registered hook configuration, stored natively by [HookBridge]. */ +data class VectorHookRecord( + val hooker: XposedInterface.Hooker, + val priority: Int, + val exceptionMode: ExceptionMode, +) + +/** + * What the chain ends in: the original executable, and the legacy callbacks around it. + * + * A Java interface rather than a Kotlin function type, so the chain - which is Java, see + * VectorChain - can name it without dragging in kotlin.jvm.functions. + * + * [args] is declared nullable although it never is. A non-null parameter makes kotlinc emit an + * assertion that R8 compiles into Object.getClass(), which would be the first instruction of the + * lambda implementing this - reached with the guard already down, since the chain lowers it before + * calling here. That one call is enough to put the dispatch back into itself. See #798. + */ +fun interface VectorTerminal { + fun run(thisObject: Any?, args: Array?): Any? +} diff --git a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorNativeHooker.kt b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorNativeHooker.kt index c8b65fe78..dab6d5622 100644 --- a/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorNativeHooker.kt +++ b/xposed/src/main/kotlin/org/matrix/vector/impl/hooks/VectorNativeHooker.kt @@ -81,43 +81,65 @@ class VectorHookBuilder( * The native callback entrypoint. Instantiated natively by [HookBridge] when a hooked method is * hit. */ -class VectorNativeHooker(private val method: T) { +class VectorNativeHooker(@JvmField val method: T) { + + // Read from C++ on the re-entrant path, which cannot afford to call Java to ask. + @JvmField val isStatic = Modifier.isStatic(method.modifiers) - private val isStatic = Modifier.isStatic(method.modifiers) private val returnType = if (method is Method) method.returnType else null - /** Invoked by C++ via JNI. */ - fun callback(args: Array): Any? { + /** + * The trampoline entry point, called by the generated hook method. + * + * Native, and implemented in hook_bridge.cpp, so that the re-entrancy check is genuinely the + * first thing that runs. A Kotlin body cannot promise that: R8 puts the parameter null check + * ahead of it, and since AGP 9 that check is a call to Object.getClass(), which a module is + * allowed to hook. See #798. + */ + external fun callback(args: Array): Any? + + /** The dispatch proper, called by [callback] with the guard raised. */ + fun dispatch(args: Array): Any? { val thisObject = if (isStatic) null else args[0] - val actualArgs = if (isStatic) args else args.sliceArray(1 until args.size) + // Not sliceArray: it copies through Arrays.copyOfRange, and this runs on every dispatch of + // every hooked method. The element type is known here, so build the array directly. + val actualArgs = + if (isStatic) args + else arrayOfNulls(args.size - 1).also { System.arraycopy(args, 1, it, 0, it.size) } // Retrieve the hook snapshots. Null means every hook was removed after this trampoline was // entered, which is indistinguishable from having none. val snapshots = HookBridge.callbackSnapshot(VectorHookRecord::class.java, method) - ?: return invokeOriginalSafely(thisObject, actualArgs) + ?: return callIntoModule { invokeOriginalSafely(thisObject, actualArgs) } @Suppress("UNCHECKED_CAST") val modernHooks = snapshots[0] as Array val legacyHooks = snapshots[1] // Fast path: No hooks active if (modernHooks.isEmpty() && legacyHooks.isEmpty()) { - return invokeOriginalSafely(thisObject, actualArgs) + return callIntoModule { invokeOriginalSafely(thisObject, actualArgs) } } - val terminal: (Any?, Array) -> Any? = { tObj, tArgs -> + // No guard of its own: the chain lowers it before calling here, and VectorTerminal takes a + // nullable array so this lambda has no null check ahead of its first statement. + val terminal = VectorTerminal { tObj, tArgs -> + val actual = tArgs ?: emptyArray() val delegate = VectorBootstrap.delegate if (legacyHooks.isNotEmpty() && delegate != null) { - delegate.processLegacyHook(method, tObj, tArgs, legacyHooks) { - invokeOriginalSafely(tObj, tArgs) + delegate.processLegacyHook(method, tObj, actual, legacyHooks) { + invokeOriginalSafely(tObj, actual) } } else { - invokeOriginalSafely(tObj, tArgs) + invokeOriginalSafely(tObj, actual) } } val rootChain = VectorChain(method, thisObject, actualArgs, modernHooks, 0, terminal) + // The chain lowers the guard itself, around the hooker and around the terminal. It cannot + // be lowered here: the chain's own bookkeeping has to stay guarded, or it dispatches on + // every node it builds. val result = rootChain.proceed() // Type safety validation before returning to C++ @@ -163,7 +185,7 @@ class VectorNativeHooker(private val method: T) { /** Safely invokes the original method, unwrapping InvocationTargetExceptions. */ private fun invokeOriginalSafely(tObj: Any?, tArgs: Array): Any? { return try { - HookBridge.invokeOriginalMethod(method, tObj, *tArgs) + HookBridge.invokeOriginalMethod(method, tObj, tArgs) } catch (ite: InvocationTargetException) { throw ite.cause ?: ite } diff --git a/xposed/src/main/kotlin/org/matrix/vector/nativebridge/HookBridge.kt b/xposed/src/main/kotlin/org/matrix/vector/nativebridge/HookBridge.kt index d469eb4a5..896ccd916 100644 --- a/xposed/src/main/kotlin/org/matrix/vector/nativebridge/HookBridge.kt +++ b/xposed/src/main/kotlin/org/matrix/vector/nativebridge/HookBridge.kt @@ -34,7 +34,14 @@ object HookBridge { IllegalArgumentException::class, InvocationTargetException::class, ) - external fun invokeOriginalMethod(method: Executable, thisObject: Any?, vararg args: Any?): Any? + // Takes the array rather than a vararg: a Kotlin spread copies it through Arrays.copyOf, and + // this runs on every dispatch of every hooked method. The JVM descriptor is unchanged, so the + // native registration still matches. + external fun invokeOriginalMethod( + method: Executable, + thisObject: Any?, + args: Array, + ): Any? @JvmStatic @Throws( @@ -47,9 +54,35 @@ object HookBridge { shorty: CharArray, clazz: Class, thisObject: Any?, - vararg args: Any?, + args: Array, ): Any? + /** + * Lowers the dispatch guard for the duration of a call into module code, and returns the depth + * to hand back to [resumeDispatch]. + * + * While the guard is raised, a hooked method entered from the framework's own frames runs its + * original instead of dispatching again. That is what keeps the dispatch from re-entering + * itself when a module hooks something the dispatch happens to call — including calls no one + * wrote, such as the Object.getClass() that R8 compiles Kotlin's null checks into. Hookers and + * the original method are entitled to a full dispatch, so the guard comes down around them. + */ + @JvmStatic @FastNative external fun suspendDispatch(): Int + + /** Restores the depth returned by [suspendDispatch] or [raiseDispatch]. */ + @JvmStatic @FastNative external fun resumeDispatch(depth: Int) + + /** + * Raises the guard over a stretch of framework code, returning the depth to hand back to + * [resumeDispatch]. + * + * The chain is re-entered from module code with the guard already down, so it cannot assume it + * holds. Building one chain node calls getClass twice — R8's null checks on the constructor's + * parameters — and each of those would otherwise dispatch, build another node, and call + * getClass again. + */ + @JvmStatic @FastNative external fun raiseDispatch(): Int + @JvmStatic @FastNative external fun instanceOf(obj: Any?, clazz: Class<*>): Boolean @JvmStatic @FastNative external fun setTrusted(cookie: Any?): Boolean