Skip to content
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

[core] Incorrect decompiling #884

Closed
amspeople opened this issue Mar 14, 2020 · 8 comments
Closed

[core] Incorrect decompiling #884

amspeople opened this issue Mar 14, 2020 · 8 comments
Labels
bug Core Issues in jadx-core module

Comments

@amspeople
Copy link

amspeople commented Mar 14, 2020

classes5.zip
Use jadx latest unstable.

  • class: com.xiaomi.hm.health.bt.device.HMDeviceSource
    image
    Instead of lines:
    put(Integer.valueOf(HMDeviceSource.a(50, 256)), HMDeviceSource.OTHER_MOZART);
    Must be code using method b, not a:
    put(Integer.valueOf(HMDeviceSource.b(50, 256)), HMDeviceSource.OTHER_MOZART);

Sample from Luyten:
image

@amspeople amspeople added Core Issues in jadx-core module bug labels Mar 14, 2020
@jpstotz
Copy link
Collaborator

jpstotz commented Mar 15, 2020

It looks like there the method HMDeviceSource.a(int,int) is missing in the Jadx decompiled code. I can see it in the smali code:

.method public static synthetic a(II)I
    .registers 2
    .line 1
    invoke-static {p0, p1}, Lcom/xiaomi/hm/health/bt/device/HMDeviceSource;->b(II)I
    move-result p0
    return p0
.end method

As you can see this method just calls HMDeviceSource.b(int,int). If this method would exists the code would be equivalent to the code generated by Luyten. I don't know why Luyten does not show this indirection through the a method (may be optimized in a way by dex2jar?).

@amspeople
Copy link
Author

may be optimized in a way by dex2jar?

Not. I have not used dex2jar. Just pulled the dex file from apk.
Earlier, jadx did "not stumble" on this code. In which version it worked correctly, I no longer remember.

@jpstotz
Copy link
Collaborator

jpstotz commented Mar 15, 2020

I tracked it down, the method is not decompiled because it has the flag synthetic and is a method of a enum class:
https://github.com/skylot/jadx/blob/master/jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java#L306

@skylot I am not very familiar with synthetic methods in enum classes but it seems to me like that we can't just hide all synthetic methods. Especially if they are referenced by other code parts.

@amspeople
Copy link
Author

Thank you so much for responding to my message. I hope @skylot will hear you. And me...

@skylot
Copy link
Owner

skylot commented Mar 15, 2020

@jpstotz You are right! Just removing isSynthetic from condition resolve issue and also allow jadx to replace a call with b. Looks like I made such removing due to possible obfuscation of standard methods, see TODO on this method :(

@jpstotz
Copy link
Collaborator

jpstotz commented Mar 16, 2020

@skylot Do you remember which type of methods should be removed by the isSynthetic check?

And that the current version is not able to hide the obfuscated methods of valueOf and values is in my opinion not that big problem. When Jadx has to deal with obfuscated code errors are inevitable, we can't foresee all the strange modifications the obfuscators can apply.

@skylot
Copy link
Owner

skylot commented Mar 16, 2020

Do you remember which type of methods should be removed by the isSynthetic check?

I tried to found one yesterday but looks like there are no such methods.

And that the current version is not able to hide the obfuscated methods of valueOf and values is in my opinion not that big problem

Agree, except values which uses removed $VALUES field, so it must be also removed.
I think it is safe to remove all methods which are uses $VALUES field. I will make such a change shortly.

Also looks like valueOf it is not safe to remove by name because it can be overriden and have different code. From java doc:

Note that for a particular enum type T, the implicitly declared public static T valueOf(String) method on that enum may be used instead of this method to map from a name to the corresponding enum constant.

@skylot
Copy link
Owner

skylot commented Mar 16, 2020

I made a fix. Hope it doesn't break anything :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Core Issues in jadx-core module
Projects
None yet
Development

No branches or pull requests

3 participants