-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
handle field / method name collision #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w00t! Just some nitpicks:
lib/class-factory.js
Outdated
@@ -669,9 +669,13 @@ function ClassFactory (vm) { | |||
try { | |||
const fieldName = invokeObjectMethodNoArgs(env.handle, field, fieldGetName); | |||
try { | |||
const fieldjsName = env.stringFromJni(fieldName); | |||
var fieldjsName = env.stringFromJni(fieldName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use let
here to allow re-assigning – var
is a legacy keyword from ES5.
lib/class-factory.js
Outdated
const fieldHandle = env.newGlobalRef(field); | ||
fieldHandles.push(fieldHandle); | ||
// If we have a collided method, suffix the fieldName | ||
if(jsMethods.hasOwnProperty(fieldjsName)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Comment can be removed as it's already clear what the code is doing.
- Missing space after
if
. - Theoretically there might be a method prefixed with
_
having the same name, so usingwhile
here would be better.
test/re/frida/MethodTest.java
Outdated
|
||
static int Particle2(){ | ||
return 4; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field- and method-names should follow the coding style, i.e. camelCase
, not CamelCase
.
Ok, I have fixed according to your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there
lib/class-factory.js
Outdated
const fieldHandle = env.newGlobalRef(field); | ||
fieldHandles.push(fieldHandle); | ||
while (jsMethods.hasOwnProperty(fieldjsName)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Let's move these 3 lines to right after the
let
, as they belong together. Then put a blank line between that and thefieldHandle
stuff (those two lines), and then one blank line before assigning tojsFields
. Just to make the code a bit more readable. - Missing a space before
{
.
test/re/frida/MethodTest.java
Outdated
return 3; | ||
} | ||
|
||
static int particle2(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space before {
.
test/re/frida/MethodTest.java
Outdated
|
||
class Collider { | ||
static int particle = 1; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace.
test/re/frida/MethodTest.java
Outdated
@@ -65,6 +65,62 @@ public void genericsCanBeUsed() { | |||
assertEquals("Badger", script.getNextMessage()); | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extra blank line here.
Damn I miss stylecop ^^ Should be good now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one :)
lib/class-factory.js
Outdated
|
||
const fieldHandle = env.newGlobalRef(field); | ||
fieldHandles.push(fieldHandle); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace.
The last blank line isn't blank, it's got spaces before the CRLF. |
Allow interception when a field and a method have the same name.
note : I've added some unit tests that I wasn't able to run (no build env here)
Should fix #20 :)