search

drewnoakes / metadata-extractor

Extracts Exif, IPTC, XMP, ICC and other metadata from image, video and audio files
Apache License 2.0
2.56k stars 480 forks source link

Unclosed ByteArrayInputStreams triggering LeakedClosableViolation errors on Android #491

Open tkovacs-dev opened 4 years ago

tkovacs-dev commented 4 years ago

So there's a thing in Android called 'strict mode', which attempts to warn of some types of programming errors at runtime. One of these issues is not calling close() on a Closable. Unfortunately it's not aware of the fact that ByteArrayInputStreams don't really have to be closed. There are a number of places in metadataextractor where ByteArrayInputStreams are not closed, thus triggering errors like this to appear in the app's logcat log:

2020-07-24 09:50:31.219 17929-18443/app E/U: {5562|AsyncTask #3} A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
    android.os.strictmode.LeakedClosableViolation: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
        at android.os.StrictMode$AndroidCloseGuardReporter.report(StrictMode.java:1786)
        at dalvik.system.CloseGuard.warnIfOpen(CloseGuard.java:264)
        at java.util.zip.Inflater.finalize(Inflater.java:398)
        at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:252)
        at java.lang.Daemons$FinalizerDaemon.runInternal(Daemons.java:239)
        at java.lang.Daemons$Daemon.run(Daemons.java:105)
        at java.lang.Thread.run(Thread.java:764)
     Caused by: java.lang.Throwable: Explicit termination method 'end' not called
        at dalvik.system.CloseGuard.open(CloseGuard.java:221)
        at java.util.zip.Inflater.<init>(Inflater.java:114)
        at java.util.zip.Inflater.<init>(Inflater.java:121)
        at java.util.zip.InflaterInputStream.<init>(InflaterInputStream.java:112)
        at com.drew.imaging.png.PngMetadataReader.processChunk(PngMetadataReader.java:221)
        at com.drew.imaging.png.PngMetadataReader.readMetadata(PngMetadataReader.java:104)
        at com.drew.imaging.ImageMetadataReader.readMetadata(ImageMetadataReader.java:157)
        at com.drew.imaging.ImageMetadataReader.readMetadata(ImageMetadataReader.java:124)
        at com.drew.imaging.ImageMetadataReader.readMetadata(ImageMetadataReader.java:103)
        ...
        at android.os.AsyncTask$2.call(AsyncTask.java:333)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:245)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:764) 
drewnoakes commented 4 years ago

Thanks for the issue. Sounds reasonable. Can you create a pull request for this? I don't do Android development so cannot test a fix. It should be pretty straightforward.

Nadahar commented 4 years ago

It's not entirely clear to me what happens here, but if you provide metadata-extractor with an InputStream, it's your responsibility to close it. Only streams created by metadata-extractor should be closed by it, and I can't quite think of why metadata-extractor would do that unless you supply it with a File. But that would then be a file-based stream, not a ByteArrayInputStream, which makes me believe that this is "your" stream that you should close.

drewnoakes commented 4 years ago

There are a few places where the library constructs ByteArrayInputStream:

https://github.com/drewnoakes/metadata-extractor/search?q=ByteArrayInputStream

Nadahar commented 4 years ago

@drewnoakes Ok, sorry, I guess I was a bit too quick with my reply :wink:

It seems to be three locations in PngMetadataReader where they aren't closed: https://github.com/drewnoakes/metadata-extractor/blob/f318ec3757a1aade9be54e60ea220c9251c42ef5/Source/com/drew/imaging/png/PngMetadataReader.java#L179-L185 https://github.com/drewnoakes/metadata-extractor/blob/f318ec3757a1aade9be54e60ea220c9251c42ef5/Source/com/drew/imaging/png/PngMetadataReader.java#L224-L230 https://github.com/drewnoakes/metadata-extractor/blob/f318ec3757a1aade9be54e60ea220c9251c42ef5/Source/com/drew/imaging/png/PngMetadataReader.java#L268-L274

On the first, it's only closed if no exception is thrown. On the two latter, it's not closed under any circumstances. It is of course without any real consequence because it's a ByteArrayInputStream, but I think one should be very strict with always closing streams because it can cause so much havoc for other stream types. And then there's the static analyzers that don't know of course.

I don't remember if you're still on Java 6 as the minimum. I think try-with-resources came with Java 7. It is the "cleanest" way to handle this IMO, but if is of course no problem to close it "manually" in all potential locations either.