Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Conversation

@mh0rst
Copy link

@mh0rst mh0rst commented Jun 28, 2015

This patch fixes a problem with native methods which are hooked by a module and registered through AndroidRuntime::registerNativeMethods.

The problem occurred when I was testing XPrivacy with a custom build of Xposed with the change to enable hooking of native methods. When I enabled XPrivacy, I got stuck in a bootloop (shortened logcat):

W/XPrivacy/XMediaRecorder( 1963): Native method=public native void android.media.MediaRecorder.start() throws java.lang.IllegalStateException
W/XPrivacy/XMediaRecorder( 1963): Native method=public native void android.media.MediaRecorder.stop() throws java.lang.IllegalStateException
....
E/art     ( 1963): Failed to register non-native method android.media.MediaRecorder.start()V as native
F/art     ( 1963): art/runtime/jni_internal.cc:769] JNI FatalError called: RegisterNatives failed for 'android/media/MediaRecorder'; aborting...
F/art     ( 1963): art/runtime/runtime.cc:290] Runtime aborting...
F/art     ( 1963): art/runtime/runtime.cc:290] Aborting thread:
F/art     ( 1963): art/runtime/runtime.cc:290] "main" prio=5 tid=1 Native
F/art     ( 1963): art/runtime/runtime.cc:290]   | group="" sCount=0 dsCount=0 obj=0x12c31100 self=0xb4827800
F/art     ( 1963): art/runtime/runtime.cc:290]   | sysTid=1963 nice=0 cgrp=default sched=0/0 handle=0xb6fb8bec
F/art     ( 1963): art/runtime/runtime.cc:290]   | state=R schedstat=( 1247655114 26798226 268 ) utm=111 stm=13 core=1 HZ=100
F/art     ( 1963): art/runtime/runtime.cc:290]   | stack=0xbe1d2000-0xbe1d4000 stackSize=8MB
F/art     ( 1963): art/runtime/runtime.cc:290]   | held mutexes= "abort lock" "mutator lock"(shared held)
F/art     ( 1963): art/runtime/runtime.cc:290]   native: #00 pc 0000484c  /system/lib/libbacktrace_libc++.so (UnwindCurrent::Unwind(unsigned int, ucontext*)+23)

The problem is that after XPrivacy hooked the method MediaRecorder.start(), the initialization of the MediaRouter JNI code calls AndroidRuntime::registerNativeMethods (see https://android.googlesource.com/platform/frameworks/base/+/android-5.1.0_r1/media/jni/android_media_MediaRecorder.cpp line 517), which will ultimately call RegisterNativeMethods inside jni_internal.cc.
Because the hooked native Xposed method does not have the native flag, the call fails. Adding a check whether the original method is native (see changeset) makes the problem disappear and the hooking successful (I am able to prevent the camera app from receiving an image through XPrivacy).

May be of interest for @M66B, as this fixes bootloops with XPrivacy.
A test version for Lollipop 5.1 on ARM including this patch is available at http://www.mediafire.com/download/ni4p461civf6csz/xposed-sdk22-arm-20150628.zip

@M66B
Copy link

M66B commented Jun 28, 2015

@mh0rst thanks for mentioning this to me. Maybe the test version can be of use, but I would rather like to see an official release with all these problems solved.

@mh0rst
Copy link
Author

mh0rst commented Jun 28, 2015

@rovo89 I'm also curious why the Xposed hooked methods are always marked as non-native (see https://github.com/rovo89/android_art/blob/xposed-lollipop/runtime/mirror/art_method.cc#L438). Is this to circumvent ART optimizations?

@rovo89
Copy link
Owner

rovo89 commented Jun 28, 2015

Thanks for the detailed pull request! The fix looks good, but I'm wondering if it's better or worse to handle it like in 1895202 instead (i.e. simulate that the method is non-native in most situations, but explicitely allow some very specific places to see the actual flags.

The reason for removing the flag from the hooked (but not the original) method is that there are many places that check this property and handle those methods differently than other methods. Most importantly, the native implementation would be called instead of the proxy entrypoint, and Xposed already re-uses the field with the function pointer for its internal structure.

I think I'll go for your implementation first, in order to finally release a version which is native-capable at all. Then I think it would be good to check all the places where IsNative() is called and decide which ones need special handling.

@rovo89 rovo89 added the bug label Jun 28, 2015
@rovo89
Copy link
Owner

rovo89 commented Jun 28, 2015

Merged in 5f4bc91, but I keep this open to think about the different approach mentioned above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants