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

ApkDecoder locks input file #1160

Closed
MarcMil opened this Issue Feb 13, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@MarcMil

MarcMil commented Feb 13, 2016

Running

        ApkDecoder decoder = new ApkDecoder();
        try {
            decoder.setDecodeSources(ApkDecoder.DECODE_SOURCES_NONE);
            decoder.setDecodeResources(ApkDecoder.DECODE_RESOURCES_FULL);
            decoder.setApkFile(new File("i.apk"));
            decoder.setOutDir(new File("out"));
            decoder.setAnalysisMode(true, true);
            decoder.decode();
            Thread.sleep(60000 * 100);
        } catch (Exception e) {
            e.printStackTrace();
        }

causes Apktool to lock the input file under Windows until the JVM is exited.

@BurgerZ

This comment has been minimized.

Show comment
Hide comment
@BurgerZ

BurgerZ Feb 13, 2016

Contributor

Using Thread.sleep(60000 * 100); is bad practice. You don't need to pause main thread this way. If you want to wait till apk decode process finish, just use Future to get() result.

Contributor

BurgerZ commented Feb 13, 2016

Using Thread.sleep(60000 * 100); is bad practice. You don't need to pause main thread this way. If you want to wait till apk decode process finish, just use Future to get() result.

@MarcMil

This comment has been minimized.

Show comment
Hide comment
@MarcMil

MarcMil Feb 13, 2016

The sleep is only for demo purposes.
Decode is synchronous, the moment the call comes back the decoding has finished.
But my program is not finished yet, since it is a GUI program. As long the JVM has not exited, the apk file is locked. Instead of the sleep, imagine some long running call.

MarcMil commented Feb 13, 2016

The sleep is only for demo purposes.
Decode is synchronous, the moment the call comes back the decoding has finished.
But my program is not finished yet, since it is a GUI program. As long the JVM has not exited, the apk file is locked. Instead of the sleep, imagine some long running call.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Feb 14, 2016

Owner

Yes, I see the problem. You create the lock during setApkFile, but that is never cleared after decode.

I haven't actually made a wrapper around apktool, so haven't encountered this naturally, because the last step in the actual binary is decode() and after that the JVM closes.

Owner

iBotPeaches commented Feb 14, 2016

Yes, I see the problem. You create the lock during setApkFile, but that is never cleared after decode.

I haven't actually made a wrapper around apktool, so haven't encountered this naturally, because the last step in the actual binary is decode() and after that the JVM closes.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Feb 14, 2016

Owner

ah setApkFile is actually a File so doesn't need to be closed. There was this commit recently - 81404c8 but that was unclosed streams revolving around smali and from your code example you don't disassemble sources so this wouldn't be related.

Must have more unclosed streams in the resource portion of Apktool.

Owner

iBotPeaches commented Feb 14, 2016

ah setApkFile is actually a File so doesn't need to be closed. There was this commit recently - 81404c8 but that was unclosed streams revolving around smali and from your code example you don't disassemble sources so this wouldn't be related.

Must have more unclosed streams in the resource portion of Apktool.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Feb 15, 2016

Owner

In my initial pass through using FindBugs. I found one location where we didn't close a stream, patched here - b29df52

Owner

iBotPeaches commented Feb 15, 2016

In my initial pass through using FindBugs. I found one location where we didn't close a stream, patched here - b29df52

@MarcMil

This comment has been minimized.

Show comment
Hide comment
@MarcMil

MarcMil Feb 24, 2016

Here are the stack traces of the places where unclosed streams are:
For debugging such problems, I'd recommend to use s.th. like http://file-leak-detector.kohsuke.org/

#2 C:\Users\mmiltenberger\Downloads\NativeIDFunction.apk by thread:main on Wed Feb 24 11:25:32 PST 2016
    at java.util.zip.ZipFile.<init>(ZipFile.java:150)
    at java.util.zip.ZipFile.<init>(ZipFile.java:163)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:53)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:38)
    at brut.androlib.res.util.ExtFile.getDirectory(ExtFile.java:55)
    at brut.androlib.ApkDecoder.hasResources(ApkDecoder.java:283)
    at brut.androlib.ApkDecoder.getResTable(ApkDecoder.java:237)
    at brut.androlib.ApkDecoder.setAnalysisMode(ApkDecoder.java:194)
    at Bla.main(Bla.java:14)
#3 C:\Users\mmiltenberger\apktool\framework\1.apk by thread:main on Wed Feb 24 11:25:32 PST 2016
    at java.util.zip.ZipFile.<init>(ZipFile.java:150)
    at java.util.zip.ZipFile.<init>(ZipFile.java:163)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:53)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:38)
    at brut.androlib.res.util.ExtFile.getDirectory(ExtFile.java:55)
    at brut.androlib.res.AndrolibResources.getResPackagesFromApk(AndrolibResources.java:552)
    at brut.androlib.res.AndrolibResources.loadFrameworkPkg(AndrolibResources.java:124)
    at brut.androlib.res.data.ResTable.getPackage(ResTable.java:83)
    at brut.androlib.res.data.ResTable.getResSpec(ResTable.java:66)
    at brut.androlib.res.data.ResTable.getResSpec(ResTable.java:62)
    at brut.androlib.res.decoder.ResAttrDecoder.decode(ResAttrDecoder.java:38)
    at brut.androlib.res.decoder.AXmlResourceParser.getAttributeValue(AXmlResourceParser.java:369)
    at org.xmlpull.v1.wrapper.classic.XmlPullParserDelegate.getAttributeValue(XmlPullParserDelegate.java:69)
    at brut.androlib.res.decoder.XmlPullStreamDecoder$1.parseManifest(XmlPullStreamDecoder.java:97)
    at brut.androlib.res.decoder.XmlPullStreamDecoder$1.event(XmlPullStreamDecoder.java:65)
    at brut.androlib.res.decoder.XmlPullStreamDecoder.decode(XmlPullStreamDecoder.java:141)
    at brut.androlib.res.decoder.XmlPullStreamDecoder.decodeManifest(XmlPullStreamDecoder.java:153)
    at brut.androlib.res.decoder.ResFileDecoder.decodeManifest(ResFileDecoder.java:140)
    at brut.androlib.res.AndrolibResources.decodeManifestWithResources(AndrolibResources.java:208)
    at brut.androlib.Androlib.decodeManifestWithResources(Androlib.java:140)
    at brut.androlib.ApkDecoder.decode(ApkDecoder.java:104)
    at Bla.main(Bla.java:15)

MarcMil commented Feb 24, 2016

Here are the stack traces of the places where unclosed streams are:
For debugging such problems, I'd recommend to use s.th. like http://file-leak-detector.kohsuke.org/

#2 C:\Users\mmiltenberger\Downloads\NativeIDFunction.apk by thread:main on Wed Feb 24 11:25:32 PST 2016
    at java.util.zip.ZipFile.<init>(ZipFile.java:150)
    at java.util.zip.ZipFile.<init>(ZipFile.java:163)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:53)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:38)
    at brut.androlib.res.util.ExtFile.getDirectory(ExtFile.java:55)
    at brut.androlib.ApkDecoder.hasResources(ApkDecoder.java:283)
    at brut.androlib.ApkDecoder.getResTable(ApkDecoder.java:237)
    at brut.androlib.ApkDecoder.setAnalysisMode(ApkDecoder.java:194)
    at Bla.main(Bla.java:14)
#3 C:\Users\mmiltenberger\apktool\framework\1.apk by thread:main on Wed Feb 24 11:25:32 PST 2016
    at java.util.zip.ZipFile.<init>(ZipFile.java:150)
    at java.util.zip.ZipFile.<init>(ZipFile.java:163)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:53)
    at brut.directory.ZipRODirectory.<init>(ZipRODirectory.java:38)
    at brut.androlib.res.util.ExtFile.getDirectory(ExtFile.java:55)
    at brut.androlib.res.AndrolibResources.getResPackagesFromApk(AndrolibResources.java:552)
    at brut.androlib.res.AndrolibResources.loadFrameworkPkg(AndrolibResources.java:124)
    at brut.androlib.res.data.ResTable.getPackage(ResTable.java:83)
    at brut.androlib.res.data.ResTable.getResSpec(ResTable.java:66)
    at brut.androlib.res.data.ResTable.getResSpec(ResTable.java:62)
    at brut.androlib.res.decoder.ResAttrDecoder.decode(ResAttrDecoder.java:38)
    at brut.androlib.res.decoder.AXmlResourceParser.getAttributeValue(AXmlResourceParser.java:369)
    at org.xmlpull.v1.wrapper.classic.XmlPullParserDelegate.getAttributeValue(XmlPullParserDelegate.java:69)
    at brut.androlib.res.decoder.XmlPullStreamDecoder$1.parseManifest(XmlPullStreamDecoder.java:97)
    at brut.androlib.res.decoder.XmlPullStreamDecoder$1.event(XmlPullStreamDecoder.java:65)
    at brut.androlib.res.decoder.XmlPullStreamDecoder.decode(XmlPullStreamDecoder.java:141)
    at brut.androlib.res.decoder.XmlPullStreamDecoder.decodeManifest(XmlPullStreamDecoder.java:153)
    at brut.androlib.res.decoder.ResFileDecoder.decodeManifest(ResFileDecoder.java:140)
    at brut.androlib.res.AndrolibResources.decodeManifestWithResources(AndrolibResources.java:208)
    at brut.androlib.Androlib.decodeManifestWithResources(Androlib.java:140)
    at brut.androlib.ApkDecoder.decode(ApkDecoder.java:104)
    at Bla.main(Bla.java:15)

MarcMil pushed a commit to MarcMil/Apktool that referenced this issue May 2, 2017

@iBotPeaches iBotPeaches closed this in #1490 May 2, 2017

iBotPeaches added a commit that referenced this issue May 5, 2017

Pass exception back to user
 - check object is not null before closing
 - refs #1160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment