Update native method entry before calling backup

canyie · May 16, 2022 · c95af52 · c95af52
1 parent a35fff5
commit c95af52
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 27 deletions.
diff --git a/core/src/main/cpp/pine.cpp b/core/src/main/cpp/pine.cpp
@@ -11,10 +11,10 @@
 #include "utils/scoped_local_ref.h"
 #include "utils/log.h"
 #include "utils/jni_helper.h"
-#include "trampoline/extras.h"
 #include "utils/memory.h"
 #include "utils/well_known_classes.h"
 #include "trampoline/trampoline_installer.h"
+#include "trampoline/extras.h"
 
 using namespace pine;
 
@@ -396,13 +396,29 @@ void Pine_getArgsX86(JNIEnv* env, jclass, jint javaExtras, jintArray javaArray,
 }
 #endif
 
-void Pine_updateDeclaringClass(JNIEnv* env, jclass, jobject javaOrigin, jobject javaBackup) {
+void Pine_syncMethodInfo(JNIEnv* env, jclass, jobject javaOrigin, jobject javaBackup) {
     auto origin = art::ArtMethod::FromReflectedMethod(env, javaOrigin);
     auto backup = art::ArtMethod::FromReflectedMethod(env, javaBackup);
-    uint32_t declaring_class = origin->GetDeclaringClass();
-    if (declaring_class != backup->GetDeclaringClass()) {
-        LOGI("The declaring_class of method has moved by gc, update its reference in backup method.");
-        backup->SetDeclaringClass(declaring_class);
+
+    // ArtMethod is actually an instance of java class "java.lang.reflect.ArtMethod" on pre M
+    // declaring_class is a reference field so the runtime itself will update it if moved by GC
+    if (Android::version >= Android::kM) {
+        uint32_t declaring_class = origin->GetDeclaringClass();
+        if (declaring_class != backup->GetDeclaringClass()) {
+            LOGI("GC moved declaring class of method %p, also update in backup %p", origin, backup);
+            backup->SetDeclaringClass(declaring_class);
+        }
+    }
+
+    // JNI method entry may be changed by RegisterNatives or UnregisterNatives
+    // Use backup to check native as we may add kNative to access flags of origin (Android 8.0+ with debuggable mode)
+    if (backup->IsNative()) {
+        void* previous = backup->GetEntryPointFromJni();
+        void* current = origin->GetEntryPointFromJni();
+        if (current != previous) {
+            LOGI("Native entry of method %p was changed, also update in backup %p", origin, backup);
+            backup->SetEntryPointFromJni(current);
+        }
     }
 }
 
@@ -431,7 +447,7 @@ static const struct {
     const char* signature;
 } gFastNativeMethods[] = {
         {"getArtMethod", "(Ljava/lang/reflect/Member;)J"},
-        {"updateDeclaringClass", "(Ljava/lang/reflect/Member;Ljava/lang/reflect/Method;)V"},
+        {"syncMethodInfo", "(Ljava/lang/reflect/Member;Ljava/lang/reflect/Method;)V"},
         {"decompile0", "(Ljava/lang/reflect/Member;Z)Z"},
         {"disableJitInline0", "()Z"},
         {"setJitCompilationAllowed0", "(Z)V"},
@@ -470,7 +486,7 @@ static const JNINativeMethod gMethods[] = {
         {"disableJitInline0", "()Z", (void*) Pine_disableJitInline0},
         {"setJitCompilationAllowed0", "(Z)V", (void*) Pine_setJitCompilationAllowed},
         {"disableProfileSaver0", "()Z", (void*) Pine_disableProfileSaver0},
-        {"updateDeclaringClass", "(Ljava/lang/reflect/Member;Ljava/lang/reflect/Method;)V", (void*) Pine_updateDeclaringClass},
+        {"syncMethodInfo", "(Ljava/lang/reflect/Member;Ljava/lang/reflect/Method;)V", (void*) Pine_syncMethodInfo},
         {"getObject0", "(JJ)Ljava/lang/Object;", (void*) Pine_getObject0},
         {"getAddress0", "(JLjava/lang/Object;)J", (void*) Pine_getAddress0},
         {"setDebuggable0", "(Z)V", (void*) Pine_setDebuggable},

diff --git a/core/src/main/cpp/trampoline/extras.h b/core/src/main/cpp/trampoline/extras.h
@@ -26,16 +26,15 @@ namespace pine {
         ~Extras() {
         }
 
-#if defined(__aarch64__) || defined(__arm__) // Not supported spinlock on x86 platform
         void ReleaseLock() {
+#if defined(__aarch64__) || defined(__arm__) // Not supported spinlock on x86 platform
             CHECK(lock_flag == 0, "Unexpected lock_flag %d", lock_flag);
-
             dmb(); // Ensure all previous accesses are observed before the lock is released.
             lock_flag = 1;
             dsb(); // Ensure completion of the store that cleared the lock before sending the event.
             sev(); // Wake up the thread that is waiting for the lock.
-        }
 #endif
+        }
 
         /** Thread lock flag, 1: unlocked, 0: locked. */
         volatile uint32_t lock_flag = 1;

diff --git a/core/src/main/java/top/canyie/pine/Pine.java b/core/src/main/java/top/canyie/pine/Pine.java
@@ -427,21 +427,18 @@ public static long getAddress(long thread, Object o) {
     }
 
     static Object callBackupMethod(Member origin, Method backup, Object thisObject, Object[] args) throws InvocationTargetException, IllegalAccessException {
-        if (PineConfig.sdkLevel >= Build.VERSION_CODES.N) {
-            // On Android 7.0+, java.lang.Class object is movable and may cause crash when
-            // invoke backup method, so we update declaring_class when invoke backup method.
-            Class<?> declaring = origin.getDeclaringClass();
-            updateDeclaringClass(origin, backup);
-            // FIXME: GC happens here (you can add Runtime.getRuntime().gc()) will crash backup calling
-            Object result = backup.invoke(thisObject, args);
-
-            // Explicit use declaring_class object to ensure it has reference on stack
-            // and avoid being moved by gc.
-            declaring.getClass();
-            return result;
-        } else {
-            return backup.invoke(thisObject, args);
-        }
+        // java.lang.Class object is movable and may cause crash when invoke backup method,
+        // native entry of JNI method may be changed by RegisterNatives and UnregisterNatives,
+        // so we need to update them when invoke backup method.
+
+        Class<?> declaring = origin.getDeclaringClass();
+        syncMethodInfo(origin, backup);
+        // FIXME: GC happens here (you can add Runtime.getRuntime().gc() to test) will crash backup calling
+        Object result = backup.invoke(thisObject, args);
+        // Explicit use declaring_class object to ensure it has reference on stack
+        // and avoid being moved by gc. (invalid for now)
+        declaring.getClass();
+        return result;
     }
 
     /**
@@ -763,7 +760,7 @@ private static native Method hook0(long thread, Class<?> declaring, Member targe
 
     public static native void getArgsX86(int extras, int[] out, int ebx);
 
-    private static native void updateDeclaringClass(Member origin, Method backup);
+    private static native void syncMethodInfo(Member origin, Method backup);
 
     public static native long currentArtThread0();