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 [EMAIL PROTECTED] 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
diff --git a/src/share/classes/com/sun/imageio/plugins/png/PNGImageReader.java
b/src/share/classes/com/sun/imageio/plugins/png/PNGImageReader.java
--- a/src/share/classes/com/sun/imageio/plugins/png/PNGImageReader.java
+++ b/src/share/classes/com/sun/imageio/plugins/png/PNGImageReader.java
@@ -37,6 +37,7 @@
import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.DataInputStream;
+import java.io.EOFException;
import java.io.InputStream;
import java.io.IOException;
import java.io.SequenceInputStream;
@@ -59,7 +60,7 @@
import java.io.ByteArrayOutputStream;
import sun.awt.image.ByteInterleavedRaster;
-class PNGImageDataEnumeration implements Enumeration {
+class PNGImageDataEnumeration implements Enumeration<InputStream> {
boolean firstTime = true;
ImageInputStream stream;
@@ -72,7 +73,7 @@
int type = stream.readInt(); // skip chunk type
}
- public Object nextElement() {
+ public InputStream nextElement() {
try {
firstTime = false;
ImageInputStream iis = new SubImageInputStream(stream, length);
@@ -211,6 +212,7 @@
ByteArrayOutputStream baos = new ByteArrayOutputStream();
int b;
while ((b = stream.read()) != 0) {
+ if (b == -1) throw new EOFException();
baos.write(b);
}
return new String(baos.toByteArray(), charset);
@@ -559,7 +561,7 @@
byte[] b = new byte[chunkLength - keyword.length() - 1];
stream.readFully(b);
- metadata.tEXt_text.add(new String(b));
+ metadata.tEXt_text.add(new String(b, "ISO-8859-1"));
}
private void parse_tIME_chunk() throws IOException {
@@ -648,7 +650,7 @@
byte[] b = new byte[chunkLength - keyword.length() - 2];
stream.readFully(b);
- metadata.zTXt_text.add(new String(inflate(b)));
+ metadata.zTXt_text.add(new String(inflate(b), "ISO-8859-1"));
}
private void readMetadata() throws IIOException {
@@ -1263,7 +1265,7 @@
try {
stream.seek(imageStartPosition);
- Enumeration e = new PNGImageDataEnumeration(stream);
+ Enumeration<InputStream> e = new PNGImageDataEnumeration(stream);
InputStream is = new SequenceInputStream(e);
/* InflaterInputStream uses an Inflater instance which consumes
diff --git a/src/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
b/src/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
--- a/src/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
+++ b/src/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
@@ -674,13 +674,8 @@
private byte[] deflate(byte[] b) throws IOException {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
DeflaterOutputStream dos = new DeflaterOutputStream(baos);
-
- int len = b.length;
- for (int i = 0; i < len; i++) {
- dos.write((int)(0xff & b[i]));
- }
+ dos.write(b);
dos.close();
-
return baos.toByteArray();
}
@@ -736,7 +731,7 @@
cs.writeByte(compressionMethod);
String text = (String)textIter.next();
- cs.write(deflate(text.getBytes()));
+ cs.write(deflate(text.getBytes("ISO-8859-1")));
cs.finish();
}
}
diff --git a/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
b/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
@@ -211,8 +211,8 @@
public int sRGB_renderingIntent;
// tEXt chunk
- public ArrayList tEXt_keyword = new ArrayList(); // 1-79 char Strings
- public ArrayList tEXt_text = new ArrayList(); // Strings
+ public ArrayList<String> tEXt_keyword = new ArrayList<String>(); // 1-79
characters
+ public ArrayList<String> tEXt_text = new ArrayList<String>();
// tIME chunk
public boolean tIME_present;
@@ -235,13 +235,13 @@
public int tRNS_blue;
// zTXt chunk
- public ArrayList zTXt_keyword = new ArrayList(); // Strings
- public ArrayList zTXt_compressionMethod = new ArrayList(); // Integers
- public ArrayList zTXt_text = new ArrayList(); // Strings
+ public ArrayList<String> zTXt_keyword = new ArrayList<String>();
+ public ArrayList<Integer> zTXt_compressionMethod = new
ArrayList<Integer>();
+ public ArrayList<String> zTXt_text = new ArrayList<String>();
// Unknown chunks
- public ArrayList unknownChunkType = new ArrayList(); // Strings
- public ArrayList unknownChunkData = new ArrayList(); // byte arrays
+ public ArrayList<String> unknownChunkType = new ArrayList<String>();
+ public ArrayList<byte[]> unknownChunkData = new ArrayList<byte[]>();
public PNGMetadata() {
super(true,
@@ -426,21 +426,14 @@
return false;
}
- private ArrayList cloneBytesArrayList(ArrayList in) {
+ private ArrayList<byte[]> cloneBytesArrayList(ArrayList<byte[]> in) {
if (in == null) {
return null;
} else {
- ArrayList list = new ArrayList(in.size());
- Iterator iter = in.iterator();
- while (iter.hasNext()) {
- Object o = iter.next();
- if (o == null) {
- list.add(null);
- } else {
- list.add(((byte[])o).clone());
- }
+ ArrayList<byte[]> list = new ArrayList<byte[]>(in.size());
+ for (byte[] b: in) {
+ list.add((byte[])b.clone());
}
-
return list;
}
}
@@ -600,7 +593,7 @@
IIOMetadataNode iTXt_node = new IIOMetadataNode("iTXtEntry");
iTXt_node.setAttribute("keyword", iTXt_keyword.get(i));
iTXt_node.setAttribute("compressionFlag",
- iTXt_compressionFlag.get(i) ? "1" : "0");
+ iTXt_compressionFlag.get(i) ? "TRUE" : "FALSE");
iTXt_node.setAttribute("compressionMethod",
iTXt_compressionMethod.get(i).toString());
iTXt_node.setAttribute("languageTag",
@@ -2002,14 +1995,14 @@
sBIT_present = false;
sPLT_present = false;
sRGB_present = false;
- tEXt_keyword = new ArrayList();
- tEXt_text = new ArrayList();
+ tEXt_keyword = new ArrayList<String>();
+ tEXt_text = new ArrayList<String>();
tIME_present = false;
tRNS_present = false;
- zTXt_keyword = new ArrayList();
- zTXt_compressionMethod = new ArrayList();
- zTXt_text = new ArrayList();
- unknownChunkType = new ArrayList();
- unknownChunkData = new ArrayList();
+ zTXt_keyword = new ArrayList<String>();
+ zTXt_compressionMethod = new ArrayList<Integer>();
+ zTXt_text = new ArrayList<String>();
+ unknownChunkType = new ArrayList<String>();
+ unknownChunkData = new ArrayList<byte[]>();
}
}
diff --git a/test/javax/imageio/plugins/png/ItxtUtf8Test.java
b/test/javax/imageio/plugins/png/ItxtUtf8Test.java
new file mode 100644
--- /dev/null
+++ b/test/javax/imageio/plugins/png/ItxtUtf8Test.java
@@ -0,0 +1,216 @@
+/**
+ * @test
+ * @bug 6541476
+ * @summary Write and read a PNG file including an non-latin1 iTXt chunk
+ *
+ * @run main ItxtUtf8Test
+ *
+ * @run main/othervm/timeout=10 -Xmx2m ItxtUtf8Test truncate
+ */
+
+import java.awt.image.BufferedImage;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStream;
+import java.util.Arrays;
+import java.util.List;
+import javax.imageio.IIOException;
+import javax.imageio.IIOImage;
+import javax.imageio.ImageIO;
+import javax.imageio.ImageReader;
+import javax.imageio.ImageTypeSpecifier;
+import javax.imageio.ImageWriter;
+import javax.imageio.metadata.IIOMetadata;
+import javax.imageio.stream.ImageInputStream;
+import javax.imageio.stream.ImageOutputStream;
+import javax.imageio.stream.MemoryCacheImageInputStream;
+import javax.imageio.stream.MemoryCacheImageOutputStream;
+import org.w3c.dom.DOMImplementation;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.bootstrap.DOMImplementationRegistry;
+
+public class ItxtUtf8Test {
+
+ public static final String
+ TEXT = "\u24c9\u24d4\u24e7\u24e3" + // circled letters from BMP
+ "\ud835\udc13\ud835\udc1e\ud835\udc31\ud835\udc2d" + // from other plane
+ "\u24c9\u24d4\u24e7\u24e3", // a repetition for compression
+ VERBATIM = "\u24e5\u24d4\u24e1\u24d1\u24d0\u24e3\u24d8\u24dc",
+ COMPRESSED =
"\u24d2\u24de\u24dc\u24df\u24e1\u24d4\u24e2\u24e2\u24d4\u24d3";
+
+ public static final byte[]
+ VBYTES = {
+ (byte)0x00, (byte)0x00, (byte)0x00, (byte)0x56, // chunk length
+ (byte)0x69, (byte)0x54, (byte)0x58, (byte)0x74, // chunk type "iTXt"
+ (byte)0x76, (byte)0x65, (byte)0x72, (byte)0x62,
+ (byte)0x61, (byte)0x74, (byte)0x69, (byte)0x6d, // keyword "verbatim"
+ (byte)0x00, // separator terminating keyword
+ (byte)0x00, // compression flag
+ (byte)0x00, // compression method, must be zero
+ (byte)0x78, (byte)0x2d, (byte)0x63, (byte)0x69,
+ (byte)0x72, (byte)0x63, (byte)0x6c, (byte)0x65,
+ (byte)0x64, // language tag "x-circled"
+ (byte)0x00, // separator terminating language tag
+ (byte)0xe2, (byte)0x93, (byte)0xa5, // '\u24e5'
+ (byte)0xe2, (byte)0x93, (byte)0x94, // '\u24d4'
+ (byte)0xe2, (byte)0x93, (byte)0xa1, // '\u24e1'
+ (byte)0xe2, (byte)0x93, (byte)0x91, // '\u24d1'
+ (byte)0xe2, (byte)0x93, (byte)0x90, // '\u24d0'
+ (byte)0xe2, (byte)0x93, (byte)0xa3, // '\u24e3'
+ (byte)0xe2, (byte)0x93, (byte)0x98, // '\u24d8'
+ (byte)0xe2, (byte)0x93, (byte)0x9c, // '\u24dc'
+ (byte)0x00, // separator terminating the translated keyword
+ (byte)0xe2, (byte)0x93, (byte)0x89, // '\u24c9'
+ (byte)0xe2, (byte)0x93, (byte)0x94, // '\u24d4'
+ (byte)0xe2, (byte)0x93, (byte)0xa7, // '\u24e7'
+ (byte)0xe2, (byte)0x93, (byte)0xa3, // '\u24e3'
+ (byte)0xf0, (byte)0x9d, (byte)0x90, (byte)0x93, // '\ud835\udc13'
+ (byte)0xf0, (byte)0x9d, (byte)0x90, (byte)0x9e, // '\ud835\udc1e'
+ (byte)0xf0, (byte)0x9d, (byte)0x90, (byte)0xb1, // '\ud835\udc31'
+ (byte)0xf0, (byte)0x9d, (byte)0x90, (byte)0xad, // '\ud835\udc2d'
+ (byte)0xe2, (byte)0x93, (byte)0x89, // '\u24c9'
+ (byte)0xe2, (byte)0x93, (byte)0x94, // '\u24d4'
+ (byte)0xe2, (byte)0x93, (byte)0xa7, // '\u24e7'
+ (byte)0xe2, (byte)0x93, (byte)0xa3, // '\u24e3'
+ (byte)0xb5, (byte)0xcc, (byte)0x97, (byte)0x56 // CRC
+ },
+ CBYTES = {
+ // we don't want to check the chunk length,
+ // as this might depend on implementation.
+ (byte)0x69, (byte)0x54, (byte)0x58, (byte)0x74, // chunk type "iTXt"
+ (byte)0x63, (byte)0x6f, (byte)0x6d, (byte)0x70,
+ (byte)0x72, (byte)0x65, (byte)0x73, (byte)0x73,
+ (byte)0x65, (byte)0x64, // keyword "compressed"
+ (byte)0x00, // separator terminating keyword
+ (byte)0x01, // compression flag
+ (byte)0x00, // compression method, 0=deflate
+ (byte)0x78, (byte)0x2d, (byte)0x63, (byte)0x69,
+ (byte)0x72, (byte)0x63, (byte)0x6c, (byte)0x65,
+ (byte)0x64, // language tag "x-circled"
+ (byte)0x00, // separator terminating language tag
+ // we don't want to check the actual compressed data,
+ // as this might depend on implementation.
+ };
+/*
+*/
+
+ public static void main(String[] args) throws Exception {
+ List argList = Arrays.asList(args);
+ if (argList.contains("truncate")) {
+ try {
+ runTest(false, true);
+ throw new AssertionError("Expect an error for truncated file");
+ }
+ catch (IIOException e) {
+ // expected an error for a truncated image file.
+ }
+ }
+ else {
+ runTest(argList.contains("dump"), false);
+ }
+ }
+
+ public static void runTest(boolean dump, boolean truncate)
+ throws Exception
+ {
+ String format = "javax_imageio_png_1.0";
+ BufferedImage img =
+ new BufferedImage(16, 16, BufferedImage.TYPE_INT_RGB);
+ ImageWriter iw = ImageIO.getImageWritersByMIMEType("image/png").next();
+ ByteArrayOutputStream os = new ByteArrayOutputStream();
+ ImageOutputStream ios = new MemoryCacheImageOutputStream(os);
+ iw.setOutput(ios);
+ IIOMetadata meta =
+ iw.getDefaultImageMetadata(new ImageTypeSpecifier(img), null);
+ DOMImplementationRegistry registry;
+ registry = DOMImplementationRegistry.newInstance();
+ DOMImplementation impl = registry.getDOMImplementation("XML 3.0");
+ Document doc = impl.createDocument(null, format, null);
+ Element root, itxt, entry;
+ root = doc.getDocumentElement();
+ root.appendChild(itxt = doc.createElement("iTXt"));
+ itxt.appendChild(entry = doc.createElement("iTXtEntry"));
+ entry.setAttribute("keyword", "verbatim");
+ entry.setAttribute("compressionFlag", "false");
+ entry.setAttribute("compressionMethod", "0");
+ entry.setAttribute("languageTag", "x-circled");
+ entry.setAttribute("translatedKeyword", VERBATIM);
+ entry.setAttribute("text", TEXT);
+ itxt.appendChild(entry = doc.createElement("iTXtEntry"));
+ entry.setAttribute("keyword", "compressed");
+ entry.setAttribute("compressionFlag", "true");
+ entry.setAttribute("compressionMethod", "0");
+ entry.setAttribute("languageTag", "x-circled");
+ entry.setAttribute("translatedKeyword", COMPRESSED);
+ entry.setAttribute("text", TEXT);
+ meta.mergeTree(format, root);
+ iw.write(new IIOImage(img, null, meta));
+ iw.dispose();
+
+ byte[] bytes = os.toByteArray();
+ if (dump)
+ System.out.write(bytes);
+ if (findBytes(VBYTES, bytes) < 0)
+ throw new AssertionError("verbatim block not found");
+ if (findBytes(CBYTES, bytes) < 0)
+ throw new AssertionError("compressed block not found");
+ int length = bytes.length;
+ if (truncate)
+ length = findBytes(VBYTES, bytes) + 32;
+
+ ImageReader ir = ImageIO.getImageReader(iw);
+ ByteArrayInputStream is = new ByteArrayInputStream(bytes, 0, length);
+ ImageInputStream iis = new MemoryCacheImageInputStream(is);
+ ir.setInput(iis);
+ meta = ir.getImageMetadata(0);
+ Node node = meta.getAsTree(format);
+ for (node = node.getFirstChild();
+ !"iTXt".equals(node.getNodeName());
+ node = node.getNextSibling());
+ boolean verbatimSeen = false, compressedSeen = false;
+ for (node = node.getFirstChild();
+ node != null;
+ node = node.getNextSibling()) {
+ entry = (Element)node;
+ String keyword = entry.getAttribute("keyword");
+ String translatedKeyword = entry.getAttribute("translatedKeyword");
+ String text = entry.getAttribute("text");
+ if ("verbatim".equals(keyword)) {
+ if (verbatimSeen) throw new AssertionError("Duplicate");
+ verbatimSeen = true;
+ if (!VERBATIM.equals(translatedKeyword))
+ throw new AssertionError("Wrong translated keyword");
+ if (!TEXT.equals(text))
+ throw new AssertionError("Wrong text");
+ }
+ else if ("compressed".equals(keyword)) {
+ if (compressedSeen) throw new AssertionError("Duplicate");
+ compressedSeen = true;
+ if (!COMPRESSED.equals(translatedKeyword))
+ throw new AssertionError("Wrong translated keyword");
+ if (!TEXT.equals(text))
+ throw new AssertionError("Wrong text");
+ }
+ else {
+ throw new AssertionError("Unexpected keyword");
+ }
+ }
+ if (!(verbatimSeen && compressedSeen))
+ throw new AssertionError("Missing chunk");
+ }
+
+ private static final int findBytes(byte[] needle, byte[] haystack) {
+ HAYSTACK: for (int h = 0; h <= haystack.length - needle.length; ++h) {
+ for (int n = 0; n < needle.length; ++n) {
+ if (needle[n] != haystack[h + n]) {
+ continue HAYSTACK;
+ }
+ }
+ return h;
+ }
+ return -1;
+ }
+
+}
