Possible issue with invokeOriginalMethod since ART.

[dzan]

Hi,

if I read the source correctly a call to invokeOriginalMethod in the XposedBridge would trigger this code path (more or less);

invokeOriginalMethod (XposedBridge.java:774)
XposedBridge_invokeOriginalMethodNative (libxposed_art.cpp:126)
InvokeMethod (reflection.cc:519)
InvokeWithArgArray (reflection.cc:409)
ArtMethod::Invoke (art_method.cc:265)
.... some assembler trampolines ...
artQuickProxyInvokeHandler (quick_trampoline_entrypoints.cc:608)
.... checking accessFlags for xposed => handleHookedMethod call

I don't understand how this would ever invoke the original method if nowhere in there the xposed bit (accessFlags) was flipped? I couldn't find a line where that would happen.

Am I missing something or is invokeOriginalMethod broke at the moment? Maybe it's only supposed to be called with methods passed through the XC_Callback params and those are supposed to have the bit flipped? I thought it used to work with Dalvik, one could pass a Method obtained through reflection and have the original called?

Kind regards

[wanam]

The flag is set here: https://github.com/rovo89/android_art/blob/4ca2451514d0242b1f762e4d1dce89cff9cb1197/runtime/mirror/art_method.cc#L417

I'm using this method with no issues on my LL modules, but i'm not sure if this is restricted to the methods passed through XC_Callback.

[dzan]

@wanam I saw that flag being set but that's irrelevant for the code path I'm talking about. That set's the flag when a method is hooked in the first place. When invokeOriginalMethod is called in the XposedBridge that bit (flag) isn't flipped as far as I could see, but maybe I just overlooked things.

The method objects passed to the callbacks are the original ones it seems here. This would explain the flag on those being unset and thus working fine.

( That struct is filled here)

[rovo89]

I noticed this as well while working on Marshmallow support. It's important to know that my ART variant creates and additional ArtMethod as a backup of the hooked method. This backup links to the original implementation and has the kAccXposedOriginalMethod flag. It also creates a Method object which is connected to the original method.

The hooked method gets the kAccXposedHookedMethod flag and is therefore redirected through the proxy handling. It calls handleHookedMethod() and passes the Method object pointing to the original ArtMethod as one argument. So the method you can get from the callback parameters is already the original one, not the hooked one, and when it is passed to invokeOriginalMethodNative() (either by your code or handleHookedMethod()), the original method is called. However, if you call invokeOriginalMethod() with a different method object (e.g. retrieved via reflection), that would be the one with the kAccXposedHookedMethod, so it goes through all the callback.

What's missing here is a lookup in invokeOriginalMethodNative() to use the original method instead, but possibly it can be optimized to avoid this lookup in the usual case where it's called from handleHookedMethod().

That said, this was unnoticed in practice until now because the need to call the original method is very rare. Even for calls from the callback, it's usually not a good way to call the original method. Instead, make sure to divide your code into before/after properly instead of using XC_MethodReplacement. Same for conditionally skipping the call to the original method - simple use setResult(null) in the before callback and the original method won't be called by handleHookedMethod(). So it's really hard to make up examples when this function should be used.

[dzan]

@rovo89 Thanks for the confirmation! I am just experimenting a bit so not necessarily writing the cleanest code :-)

[wanam]

@rovo89 thanks for the clarification, here is an example:

    private static XC_MethodHook handleInterceptKeyBeforeQueueing = new XC_MethodHook(XCallback.PRIORITY_HIGHEST) {
        @Override
        protected void beforeHookedMethod(MethodHookParam param) throws Throwable {
            mContext = (Context) getObjectField(param.thisObject, "mContext");
            final boolean isScreenOn = (Boolean) XposedHelpers.callMethod(param.thisObject, "isScreenOn");
            if (!isScreenOn) {
                final KeyEvent event = (KeyEvent) param.args[0];
                final int keyCode = event.getKeyCode();

                if (event.getAction() == KeyEvent.ACTION_DOWN) {
                    mIsLongPress = false;
                    handleVolumeLongPress(param.thisObject, keyCode);
                    param.setResult(0);
                    return;
                } else {
                    handleVolumeLongPressAbort(param.thisObject);
                    if (mIsLongPress) {
                        param.setResult(0);
                        return;
                    }

                    // send an additional "key down" because the first
                    // one was eaten
                    // the "key up" is what we are just processing
                    Object[] newArgs = new Object[2];
                    newArgs[0] = new KeyEvent(KeyEvent.ACTION_DOWN, keyCode);
                    newArgs[1] = param.args[1];
                    XposedBridge.invokeOriginalMethod(param.method, param.thisObject, newArgs);
                }
            }
        }
    };

Can we just use the "callMethod" instead of invokeOriginalMethod?

[rovo89]

That's some of the oldest Xposed module code that exists, I have written it in April/May 2012 :) https://github.com/rovo89/XposedMods/blob/master/XposedTweakbox/src/de/robv/android/xposed/mods/tweakbox/VolumeKeysSkipTrack.java#L36

In this case, it's really needed because it's an additional call to the original method, a call which has been suppressed in a previous invocation of the callback.

The issue with invokeOriginalMethod() is fixed in Xposed version 78.

[rovo89]

https://github.com/rovo89/Xposed/commit/653c5eaf7281c9c8a6d981106d084cc98059544d + a small change in ART