skylot / jadx

Dex to Java decompiler
Apache License 2.0
41.81k stars 4.88k forks source link

[core] Not deterministic XML parsing error #1740

Closed bagipro closed 1 year ago

bagipro commented 1 year ago

Hey,

I've caught a really weird bug that throws the following error:

Error decode xml
java.lang.NullPointerException: Cannot read the array length because "this.resourceIds" is null
    at jadx.core.xmlgen.BinaryXMLParser.getAttributeName(BinaryXMLParser.java:361)
    at jadx.core.xmlgen.BinaryXMLParser.parseAttribute(BinaryXMLParser.java:304)
    at jadx.core.xmlgen.BinaryXMLParser.parseElement(BinaryXMLParser.java:282)
    at jadx.core.xmlgen.BinaryXMLParser.decode(BinaryXMLParser.java:125)
    at jadx.core.xmlgen.BinaryXMLParser.parse(BinaryXMLParser.java:81)

However, this error is thrown with a ~50% chance (in other cases this.resourceIds isn't null). In case of an error, you will see a stacktrace in output logs.

It leads to incorrect decoding of AndroidManifest.xml:

<?xml version="1.0" encoding="utf-8"?>>
...

and other XML files aren't decoded too

Arguments:

jadx --log-level error --output-dir OUT app.apk

APK: https://drive.google.com/file/d/1Sm0d0I1hYW_93HQUcCvt5nEYfMtRJAi-/view?usp=sharing

UPD: I found a race condition in https://github.com/skylot/jadx/blob/master/jadx-core/src/main/java/jadx/core/xmlgen/ResourcesSaver.java#L32. If AndroidManifest.xml is processed first, no errors will happen. However, if other XML files are processed first, NullPointerException will be thrown and BinaryXMLParser will remain in a corrupted state and output extra >

bagipro commented 1 year ago

Hey!

Does anybody have an idea how it potentially can be fixed? Is RES_STRING_POOL_TYPE value always present in AndroidManifest.xml? If yes, there's a sense to process this file first.

Or it can be located in other XML files?

skylot commented 1 year ago

Fixed. Not the best solution, but it should work :slightly_smiling_face: