Support for ART on Xposed by hedger · Pull Request #12 · frida/frida-java-bridge

hedger · 2017-02-02T20:24:46Z

No description provided.

oleavr

Awesome! Let's see if we can use the optionals feature to simplify the logic a bit.

oleavr · 2017-02-02T22:33:29Z

lib/android.js

+        return true;
+      }
+      return false;
    }


I think it would be cleaner if we made use of the optionals support, by having two entries and adding both of them to optionals. We could then check if the regular one is missing, and plug in a shim function that just forwards to the Xposed one with the extra argument set to 0. (That way the implementation doesn't need to know about Xposed.)

I'm not sure that passing an extra argument in case of basic ART will produce correct behavior on all platforms. It may break stack or overwrite register values.

I meant at the JS API level only, so class-factory.js doesn't need to know which of the two functions it's calling.

I see. Currently optionals doesn't have support for remaining counter, so in case if none of those exports could be found it'll still return a non-functional API object. And extending optionals to group and count exports by their name will result in similar, if not more, complex code.

Oh, I misread the code. I'll reimplement it with optionals.

hedger · 2017-02-03T13:02:58Z

Updated. Still there's a potential problem when neither of the 2 ::Clone versions could be found, which is VERY unlikely.

oleavr

Awesome! Just a few nitpicks:

oleavr · 2017-02-03T14:06:16Z

lib/class-factory.js

        const thread = api['art::Thread::CurrentFromGdb']();
-        const target = api['art::mirror::Object::Clone'](methodId, thread);
+        const target = api['art::mirror::Object::Clone'](methodId, thread);  
+


Let's ditch this trailing whitespace and blank line.

oleavr · 2017-02-03T14:08:33Z

lib/android.js

-    }
+    },
+    optionals : ['_ZN3art6mirror6Object5CloneEPNS_6ThreadEm',
+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadE']


Since we're changing the data structure let's use a Set here instead:

optionals: new Set([ '_ZN3art6mirror6Object5CloneEPNS_6ThreadEm', '_ZN3art6mirror6Object5CloneEPNS_6ThreadE', ]);

oleavr · 2017-02-03T14:09:22Z

lib/android.js

        } else {
-          const optional = optionals[name];
-          if (optional) {
+          if (optionals.indexOf(name) > -1) {


Using a Set this becomes just:

if (optionals.has(name)) {

oleavr · 2017-02-03T14:10:11Z

lib/android.js

+        this["art::mirror::Object::Clone"] = function(thisptr, threadptr) {
+          return nativeFn(thisptr, threadptr, 0);
+        }
+      }


The style here should follow semistandard for consistency.

oleavr · 2017-02-03T14:12:02Z

lib/android.js

    const functions = api.functions || {};
    const variables = api.variables || {};
-    const optionals = api.optionals || {};
+    const optionals = api.optionals || [];


const optionals = api.optionals || new Set();

Or actually let's just make optionals a plain array, and do the Set wrapping here:

const optionals = new Set(api.optionals || []);

hedger · 2017-02-03T14:14:11Z

Done.

oleavr · 2017-02-03T14:17:23Z

Thanks! Could you also revert the trailing whitespace (see comment) and tweak the style to follow semistandard?

hedger · 2017-02-03T14:18:06Z

Done.

oleavr

Final batch :)

oleavr · 2017-02-03T14:20:14Z

lib/android.js

      '_ZN3art6Thread14CurrentFromGdbEv': ['art::Thread::CurrentFromGdb', 'pointer', []],
      '_ZNK3art6Thread13DecodeJObjectEP8_jobject': ['art::Thread::DecodeJObject', 'pointer', ['pointer', 'pointer']],
-      '_ZN3art6mirror6Object5CloneEPNS_6ThreadE': ['art::mirror::Object::Clone', 'pointer', ['pointer', 'pointer']]
+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadE' : function(address) {


Extra space before :

Missing space after function

oleavr · 2017-02-03T14:20:29Z

lib/android.js

+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadE' : function(address) {
+        this["art::mirror::Object::Clone"] = new NativeFunction(address, 'pointer', ['pointer', 'pointer']);
+      },
+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadEm' : function(address) {


Extra space before :

Missing space after function

oleavr · 2017-02-03T14:21:33Z

lib/android.js

+        const nativeFn = new NativeFunction(address, 'pointer', ['pointer', 'pointer', 'int']);
+        this["art::mirror::Object::Clone"] = function(thisptr, threadptr) {
+          return nativeFn(thisptr, threadptr, 0)
+        }


Missing space after function

Missing semicolon inside and outside the function

oleavr · 2017-02-03T14:23:19Z

lib/android.js

      }
-    }
+    },
+    optionals : new Set([


Extra space before :

Let's use a plain array and do the Set() wrapping below (see earlier comment) – sorry for the back and forth

oleavr · 2017-02-03T14:23:32Z

lib/android.js

    const functions = api.functions || {};
    const variables = api.variables || {};
-    const optionals = api.optionals || {};
+    const optionals = api.optionals || new Set();


Let's do the Set() wrapping here.

hedger · 2017-02-03T14:27:11Z

Updated.

oleavr

I suck at reading code when the style is just slightly off, so I missed this:

oleavr · 2017-02-03T14:31:48Z

lib/android.js

+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadEm': function (address) {
+        const nativeFn = new NativeFunction(address, 'pointer', ['pointer', 'pointer', 'int']);
+        this["art::mirror::Object::Clone"] = function (thisptr, threadptr) {
+          return nativeFn(thisptr, threadptr, 0);


Sorry, I missed this one:
thisptr => thisPtr
threadptr => threadPtr

No problem, renamed.

oleavr · 2017-02-03T14:47:22Z

lib/android.js

+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadEm': function (address) {
+        const nativeFn = new NativeFunction(address, 'pointer', ['pointer', 'pointer', 'int']);
+        this["art::mirror::Object::Clone"] = function (thisPtr, threadPtr) {
+          return nativeFn(thisptr, threadptr, 0);


Thanks!

^ these two guys also need to be renamed.

One more thing, I noticed Xposed uses size_t for the last argument – we don't have a type for that yet in the NativeFunction API, so the most correct thing would be pointer as it's the same size (and use the NULL short-hand). That will look pretty weird though, so maybe we can make that clearer by doing:

const numTargetBytes = NULL; return nativeFn(thisPtr, threadPtr, numTargetBytes);

Sorry, my bad. NULL trick looks reasonable, added.

oleavr · 2017-02-03T14:51:04Z

lib/android.js

+        this["art::mirror::Object::Clone"] = new NativeFunction(address, 'pointer', ['pointer', 'pointer']);
+      },
+      '_ZN3art6mirror6Object5CloneEPNS_6ThreadEm': function (address) {
+        const nativeFn = new NativeFunction(address, 'pointer', ['pointer', 'pointer', 'int']);


Thanks! We'll also have to change the last argument here to 'pointer'.

oleavr · 2017-02-03T15:18:41Z

Thanks!

Support for ART on Xposed

8089f08

oleavr requested changes Feb 2, 2017

View reviewed changes

Cleaner version using optionals

72c88f9

fixed default value

e5fe3ff

oleavr requested changes Feb 3, 2017

View reviewed changes

oleavr reviewed Feb 3, 2017

View reviewed changes

changed optionals from array to Set()

93104d2

hedger added 2 commits February 3, 2017 17:16

Style fixes

619b513

Removed whitespaces!

3612e88

oleavr requested changes Feb 3, 2017

View reviewed changes

More style fixes.

92589ee

oleavr requested changes Feb 3, 2017

View reviewed changes

renamed Clone() arguments

ffd4064

oleavr reviewed Feb 3, 2017

View reviewed changes

Renamed args, part 2.

37c01b7

oleavr reviewed Feb 3, 2017

View reviewed changes

int -> pointer

32faf3d

oleavr approved these changes Feb 3, 2017

View reviewed changes

oleavr merged commit 7febbf4 into frida:master Feb 3, 2017

IlayAtoun mentioned this pull request Mar 19, 2026

[v7.0.11+] Process crash on accessing object["<init>"] with JVMTI available #384

Open

Uh oh!

Conversation

hedger commented Feb 2, 2017

Uh oh!

oleavr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger commented Feb 3, 2017

Uh oh!

oleavr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger commented Feb 3, 2017

Uh oh!

oleavr commented Feb 3, 2017

Uh oh!

hedger commented Feb 3, 2017

Uh oh!

oleavr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger commented Feb 3, 2017

Uh oh!

oleavr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleavr Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleavr commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

hedger Feb 3, 2017 •

edited

Loading

oleavr Feb 3, 2017 •

edited

Loading