[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] 6541476: PNG imageio plugin incorrectly handles iTXt chunk
From:       Martin.vGagern () gmx ! net (Martin von Gagern)
Date:       2008-11-12 13:05:20
Message-ID: 491AD490.8060806 () gmx ! net
[Download RAW message or body]

Hi again.

As Igor kindly pointed out, there is in fact a fix for 6541476 in tree.
http://hg.openjdk.java.net/jdk7/2d/jdk/rev/3a9d06af8830
I now compared this to my own corrections. In general the changeset in
your repository has less modifications and looks somewhat cleaner. Still
I found several things to improve. I'll sum them up here, and attach a
patch.


1. Use of Generics.
Classification: Not a bug, but could be done better.

While the fix makes use of typesave generics for the iTXt chunks, it
doesn't change the other lists. My original patch here converted all
lists, so that PNGMetadata could be compiled with -Xlint:unchecked
without warnings. Just now I made an effort to extend this to the whole
png plugin directory, which involved PNGImageDataEnumeration as well.


2. EOF in readNullTerminatedString.
Classification: Bug, newly introduced regression.

The loop that reads null terminated strings just looks for 0 being
returned from ImageInputStream.read(). This ignores the possible value
of -1 which would indicate an end of stream. Because of this, truncated
PNG files could lead to long execution time (while the loop iterates at
the end of the stream) followed by an OutOfMemoryError (when enough -1
values have ben "read" and buffered). My patch manually checks for -1
and throws an EOFException if encountered. An alternative approach would
be to use DataInput.readyByte().


3. Usage of default charset.
Classification: Bug, parts already present in previous versions.

The bug fix introduced an invocation of the "String(byte[])" constructor
in parse_zTXt_chunk which is wrong. That constructor uses default
charset, where the old inflate method employed ISO-8859-1 conversion. I
added the charset name to that constructor invocation, and another one
for parse_tEXt_chunk where it had been missing even before changeset
3a9d06af8830. In write_zTXt the fix introduced a call to
String.getBytes() which is wrong for the same reasons. As the tEXt chunk
uses ImageOutputStream.writeBytes which does ISO-8859-1 conversion,
there was no previous bug in the PNGImageWriter.


4. Unnecessary overhead in deflate(byte[]).
Classification: Not a bug, but could be done better.

The newly modified deflate method now takes a byte array as input, not a
String anymore. Still it feeds bytes on at a time into the deflater,
causing unnecessary syntactic overhead and probably a performance
penalty as well. As the contract of the write(byte[]) method already
ensures that all bytes get written, simply writing the whole array to
the deflater is much simpler and more efficient. In the other direction
the inflate method could be written to read into a byte array, but
looping would still be required as the size of the array isn't known in
advance. There might be some performance to gain, but at the cost of
clarity, so I left it as it is.


5. Integer iTXtEntry at compressionFlag in generated native tree.
Classification: Bug, present in previous versions.

According to the DTD, the compressionFlag of an iTXtEntry should be
"TRUE" or "FALSE". The old code exported "0" or "1" using
Integer.toString() which the patch kept using a `? "1" : "0"' construct.
In the other direction mergeNativeTree uses getBooleanAttribute which
would do the correct thing once bug 5082756 has been fixed. I already
posted a patch for that. Right now it expects "true" or "false" so it
will accept neither "1"/"0" nor "TRUE"/"FALSE".


I also included another test case. The core is the test case I already
included in my post "6541476 (Eval)...". It has the advantage of
actually checking the bytes written to file, and ensuring that umodified
UTF-8 is used. This is no problem with the code from changeset
3a9d06af8830 but might be a nice thing to have in any case.

I also modified this test so that it exhibits the issue 2 above, the
missed EOF and resulting error for truncated PNG files. I hope I got the
 jtreg tags correct. I guess I could turn that truncated file scenario
into a separate test, but the way it is now I'm much more confident that
the test does what I expect, as I know it can read correct images and
only fails for truncated ones. I guess I could write test cases for the
other issues classified as "bug" above, if you think that worthwhile.


Please review my patch, and let me know when I should commit it locally
and export it via hg.

Greetings,
 Martin von Gagern
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bug6541476-corrections.patch
Url: http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20081112/54eb59c0/attachment.ksh 

[prev in list] [next in list] [prev in thread] [next in thread]