Multiple reads for metadata of a PNG causes a null pointer on Android 2.2.2

[leonbloy]

From my.goog....@gmail.com on October 15, 2013 02:16:41

What steps will reproduce the problem? 1.Create a PNG with pngj 2.Read the file once, skipping all bytes to just get metadata 3.Do it again in the same Thread and you'll get a null pointer exception What is the expected output? What do you see instead? I expect the second read not to throw a null pointer. A null pointer. What version of the product are you using? On what operating system? pngj version 2.0.0 on Windows 7 & Android 2.2.2 Please provide any additional information below. I'd like to use this library for an Android application that I'm creating, however I can't get passed a null pointer that it's throwing.

If you create a PNG with the library, and then try and retrieve the metadata from it twice in a row in a single Thread on Android you will get the following null pointer (works fine on Windows);

ar.com.hjg.pngj.PngjException: java.lang.NullPointerException at ar.com.hjg.pngj.chunks.ChunkHelper.compressBytes(ChunkHelper.java:219) at ar.com.hjg.pngj.chunks.PngChunkZTXT.parseFromRaw(PngChunkZTXT.java:57) at ar.com.hjg.pngj.chunks.ChunkFactory.createChunk(ChunkFactory.java:32) at ar.com.hjg.pngj.ChunkSeqReaderPng.postProcessChunk(ChunkSeqReaderPng.java:148) at ar.com.hjg.pngj.ChunkSeqReader$2.chunkDone(ChunkSeqReader.java:178) at ar.com.hjg.pngj.ChunkReader.feedBytes(ChunkReader.java:134) at ar.com.hjg.pngj.ChunkSeqReader.consume(ChunkSeqReader.java:99) at ar.com.hjg.pngj.ChunkSeqReaderPng.consume(ChunkSeqReaderPng.java:184) at ar.com.hjg.pngj.BufferedStreamFeeder.feed(BufferedStreamFeeder.java:60) at ar.com.hjg.pngj.BufferedStreamFeeder.feed(BufferedStreamFeeder.java:46) at ar.com.hjg.pngj.PngReader.end(PngReader.java:441) at ar.com.hjg.pngj.PngReader.readSkippingAllRows(PngReader.java:367)

Here is the code that is using the pngj library;

String metadata = null;
pngr.setShouldCloseStream(false); pngr.getChunkseq().setIncludeNonBufferedChunks(true); pngr.readSkippingAllRows(); // reads only metadata for(PngChunk c : pngr.getChunksList().getChunks()) { if (!ChunkHelper.isText(c))
continue; PngChunkTextVar ct = (PngChunkTextVar) c; String key = ct.getKey(); if(PngChunkTextVar.KEY_Comment.equalsIgnoreCase(key)) { metadata = ct.getVal(); break; } }

...

pngr.end(); pngr.close();

Executing the above, twice, produces the null pointer. I have tried a hundred permutations of the above to no avail.

What's weird is that the error is coming from a class that is attempting to compress data even though I'm not writing anything. The code above only reads the file. Theoretically there should be no reason for the code to take that execution path.

In any event, maybe you'll be able to figure it out.

thanks

Original issue: http://code.google.com/p/pngj/issues/detail?id=26

[leonbloy]

From my.goog....@gmail.com on October 14, 2013 22:19:21

For additional clarification, the PngReader that's used in the codes snippet above is allocated new for each execution of the code.

[leonbloy]

From my.goog....@gmail.com on October 15, 2013 19:52:04

I've narrowed it down to your use of a ThreadLocal Inflater in the ChunkHelper class. This explains why the error manifests itself across multiple uses within the same Thread.

Either the Inflater is bad per this JDK bug report: http://bugs.sun.com/view_bug.do?bug_id=7003462 ... or it's not being reset properly between uses or it's being "nulled" out somewhere in your code betwixt ChunkHelper calls.

In any event, you might want to consider not using a ThreadLocal here, but rather, an Inflater per instance of the PngReader or PngWriter. Yes, it would mean lower performance but possibly higher reliability. Plus, you might want to test the Inflater before each use?

Anyways, based on my research thus far this is what I've come up with.

thanks

[leonbloy]

From hjg.com.ar@gmail.com on October 15, 2013 20:10:50

Thanks for the report. I will look into it. It's possible that the ThreadLocal Inflater is premature optimization, but I'm still not sure how this could go wrong, as it's reset() on each call. "Inflater per instance of the PngReader or PngWriter"... actually those inflater instances are used only for compressing text in ITXT/ZTXT chunks, so I would need to create one instance for each chunk. Still then, the performance degradation looks unimportant.

Status: Accepted
Owner: hjg.com.ar@gmail.com

[leonbloy]

From my.goog....@gmail.com on October 15, 2013 20:40:48

Hey, thanks for the reply.

Yeah, this is a weird one, I'm thinking it could be related to the bug I posted above even though it's potentially a different JVM AFAIK, (Dalvik versus Sun's). I don't know if perhaps the two implementations share similar code or not. I think the reason why the reset wouldn't protect against the error is because internally the JVM uses a pool of "inflaters" and when you call reset it returns one that's bad.

On further investigation I'm definitely getting this error from the Inflater class;

Caused by: java.lang.NullPointerException at java.util.zip.Inflater.reset(Inflater.java:299) at ar.com.hjg.pngj.chunks.ChunkHelper.getInflater(ChunkHelper.java:314) at ar.com.hjg.pngj.chunks.ChunkHelper.compressBytes(ChunkHelper.java:211)

I got this exception info by immediately accessing the ChunkHelper's static Inflater Class, after invoking the PngReader.readSkippingAllRows call for the first time on the Thread, and then calling reset() on it. Internally, the reset operation is throwing a null pointer.

Also, it makes sense that it's happening for ITXT/ZTXT chunks because that's actually what I'm after in the PNG, the metadata that's stored in a ITXT chunk. If I iterate through the entire PNG using PngReader.readRow then I don't get the null pointer but I also don't seem to get any metadata. (Not sure why that happens but could just possibly be my ignorance of the API thus far). However, the minute I use the PngReader.readSkippingAllRows operation twice in a Thread I get the null pointer from the Inflater (apparently due to the reset).

I think the reason it works the first time is because perhaps the first Inflater instance that is constructed is done correctly by the JVM so it works, but subsequent "resets" on it are failing. Again, one way to test this would be to always allocate a new Inflater per the PngReader/PngWriter classes when they are constructed.

Again, this is just speculation on my part at this point.

thanks

[leonbloy]

From my.goog....@gmail.com on October 15, 2013 20:49:09

Yes, you're right, you'd need to create the new Inflater instance for each Chunk versus Reader/Writer.

[leonbloy]

From hjg.com.ar@gmail.com on October 16, 2013 07:38:57

Ok, I've decide to get rid of those ThreadLocal Inflaters, expect a new version (if I finish other changes on hold) this weekend

[leonbloy]

From my.goog....@gmail.com on October 16, 2013 15:18:05

Thanks for the quick response. I'm kind of torn on this one. To be honest, I think that your use of the single Inflater that gets reused is a good optimization. But then again if the Java API classes are pooling them as well maybe it's not needed?

I hesitate having you make the change because of what looks like is a bug in the JDK, which is actually a pretty old JDK. According to Google's stats ~3% of Android devices use this version (2.2). However, ~30% of Android devices use 2.3 and I don't know if this issue is fixed in that version or not. So, either the change your proposing to make helps 3% of devices that would use this code or 33%!

In any event, it's up to you if you want to change it, obviously. No hurry from my end, I got lots of other stuff to do on my app in the meantime. :)

thanks

[leonbloy]

From my.goog....@gmail.com on November 08, 2013 19:49:37

Hello, I don't mean any pressure by this but I was wondering if you had decided to make the Inflater change or if you were just going to leave it as is?

I know you had indicated that you were going to make the change but then I responded with at least a reasonable explanation of why you may not want to and just wanted to know where you came down on it.

thanks!

[leonbloy]

From hjg.com.ar@gmail.com on November 10, 2013 17:18:07

@my.goog.id.73 : I've already made the change in the sources https://code.google.com/p/pngj/source/detail?r=366d68aef38e39873f55a7ab4efb23ab43523829 But I've not yet made it into a release, because I'm wokring on other more complex changes. I hope to release a new version this week.

[leonbloy]

From my.goog....@gmail.com on November 11, 2013 18:02:13

Oh, OK, thanks for the quick reply.

Sorry, I didn't think to check there ... was just checking for the release.

Thanks again for changing that, no hurry on the release, whenever you get the time!

Appreciate it.

thanks.

[leonbloy]

From hjg.com.ar@gmail.com on April 23, 2014 19:42:12

Status: Fixed