From c4630d9eeecee71dd29cfcffe4adaf8298e2ab54 Mon Sep 17 00:00:00 2001 From: Harald Kuhr Date: Sat, 21 Mar 2015 16:47:15 +0100 Subject: [PATCH] TMI-121: Fixed regression, introduced by filtering out Adobe/APP14 segments completely. Now makes sure the segments have the "expected" length 16, and anything after that is discarded. --- .../jpeg/JPEGImage10MetadataCleaner.java | 34 +++--- .../jpeg/JPEGSegmentImageInputStream.java | 112 ++++++++++++++++-- .../jpeg/JPEGSegmentImageInputStreamTest.java | 7 +- 3 files changed, 120 insertions(+), 33 deletions(-) diff --git a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImage10MetadataCleaner.java b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImage10MetadataCleaner.java index 6b88eed0..420bd1b4 100644 --- a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImage10MetadataCleaner.java +++ b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImage10MetadataCleaner.java @@ -155,30 +155,24 @@ final class JPEGImage10MetadataCleaner { } } - if (adobeDCT != null) { - // Special case: Broken AdobeDCT segment, inconsistent with SOF, use values from SOF - if ((adobeDCT.getTransform() == AdobeDCTSegment.YCCK && sof.componentsInFrame() < 4 || + // Special case: Broken AdobeDCT segment, inconsistent with SOF, use values from SOF + if (adobeDCT != null && (adobeDCT.getTransform() == AdobeDCTSegment.YCCK && sof.componentsInFrame() < 4 || adobeDCT.getTransform() == AdobeDCTSegment.YCC && sof.componentsInFrame() < 3)) { - - reader.processWarningOccurred(String.format( - "Invalid Adobe App14 marker. Indicates %s data, but SOF%d has %d color component(s). " + - "Ignoring Adobe App14 marker.", - adobeDCT.getTransform() == AdobeDCTSegment.YCCK ? "YCCK/CMYK" : "YCC/RGB", - sof.marker & 0xf, sof.componentsInFrame() - )); + reader.processWarningOccurred(String.format( + "Invalid Adobe App14 marker. Indicates %s data, but SOF%d has %d color component(s). " + + "Ignoring Adobe App14 marker.", + adobeDCT.getTransform() == AdobeDCTSegment.YCCK ? "YCCK/CMYK" : "YCC/RGB", + sof.marker & 0xf, sof.componentsInFrame() + )); - // We don't add this as unknown marker, as we are certain it's bogus by now + // Remove bad AdobeDCT + NodeList app14Adobe = tree.getElementsByTagName("app14Adobe"); + for (int i = app14Adobe.getLength() - 1; i >= 0; i--) { + Node item = app14Adobe.item(i); + item.getParentNode().removeChild(item); } - else { - // Otherwise, add back the Adobe tag we filtered out in JPEGSegmentImageInputStream - IIOMetadataNode app14Adobe = new IIOMetadataNode("app14Adobe"); - app14Adobe.setAttribute("version", String.valueOf(adobeDCT.getVersion())); - app14Adobe.setAttribute("flags0", String.valueOf(adobeDCT.getFlags0())); - app14Adobe.setAttribute("flags1", String.valueOf(adobeDCT.getFlags1())); - app14Adobe.setAttribute("transform", String.valueOf(adobeDCT.getTransform())); - markerSequence.insertBefore(app14Adobe, markerSequence.getFirstChild()); - } + // We don't add this as unknown marker, as we are certain it's bogus by now } Node next = null; diff --git a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java index 4b06c61e..78401b1c 100644 --- a/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java +++ b/imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java @@ -81,7 +81,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { segment = segments.get(currentSegment); if (streamPos >= segment.start && streamPos < segment.end()) { - stream.seek(segment.realStart + streamPos - segment.start); + segment.seek(stream, segment.realStart + streamPos - segment.start); return segment; } @@ -119,7 +119,11 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { // TODO: Should we just skip all app marker segments? // We are now handling all important segments ourselves - if (isAppSegmentMarker(marker) && !(marker == JPEG.APP1 && isAppSegmentWithId("Exif", stream))) { + // TODO: Need to re-insert the APP14, and instead make sure we limit it to 16 bytes, + // OR use two delegates, one for reading image data, and one for reading metadata... + boolean isApp14Adobe = false; + if (isAppSegmentMarker(marker) && !(marker == JPEG.APP1 && isAppSegmentWithId("Exif", stream) + || (isApp14Adobe = (marker == JPEG.APP14 && isAppSegmentWithId("Adobe", stream))))) { int length = stream.readUnsignedShort(); // Length including length field itself stream.seek(realPosition + 2 + length); // Skip marker (2) + length } @@ -138,17 +142,24 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { } else { // Length including length field itself - length = stream.readUnsignedShort() + 2; + length = 2 + stream.readUnsignedShort(); + } + + if (isApp14Adobe && length != 16) { + // Need to rewrite this segment, so that it gets length 16 and discard the remaining bytes... + segment = new AdobeAPP14Replacement(realPosition, segment.end(), length, stream); + } + else { + segment = new Segment(marker, realPosition, segment.end(), length); } - segment = new Segment(marker, realPosition, segment.end(), length); segments.add(segment); } currentSegment = segments.size() - 1; if (streamPos >= segment.start && streamPos < segment.end()) { - stream.seek(segment.realStart + streamPos - segment.start); + segment.seek(stream, segment.realStart + streamPos - segment.start); break; } @@ -167,14 +178,14 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { segment = segments.get(currentSegment); if (streamPos >= segment.start && streamPos < segment.end()) { - stream.seek(segment.realStart + streamPos - segment.start); + segment.seek(stream, segment.realStart + streamPos - segment.start); break; } } } else { - stream.seek(segment.realStart + streamPos - segment.start); + segment.seek(stream, segment.realStart + streamPos - segment.start); } return segment; @@ -188,7 +199,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { try { int length = stream.readUnsignedShort(); // Length including length field itself - byte[] data = new byte[Math.max(20, length - 2)]; + byte[] data = new byte[Math.min(segmentId.length() + 1, length - 2)]; stream.readFully(data); return segmentId.equals(asNullTerminatedAsciiString(data, 0)); @@ -251,7 +262,8 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { repositionAsNecessary(); - int read = stream.read(); +// int read = stream.read(); + int read = segment.read(stream); if (read != -1) { streamPos++; @@ -272,7 +284,8 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { repositionAsNecessary(); long bytesLeft = segment.end() - streamPos; // If no more bytes after reposition, we're at EOF - int count = bytesLeft == 0 ? -1 : stream.read(b, off + total, (int) Math.min(len - total, bytesLeft)); +// int count = bytesLeft == 0 ? -1 : stream.read(b, off + total, (int) Math.min(len - total, bytesLeft)); + int count = bytesLeft == 0 ? -1 : segment.read(stream, b, off + total, (int) Math.min(len - total, bytesLeft)); if (count == -1) { // EOF @@ -298,7 +311,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { } static class Segment { - private final int marker; + final int marker; final long realStart; final long start; @@ -319,9 +332,86 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl { return start + length; } + public void seek(final ImageInputStream stream, final long newPos) throws IOException { + stream.seek(newPos); + } + + public int read(final ImageInputStream stream) throws IOException { + return stream.read(); + } + + public int read(final ImageInputStream stream, byte[] b, int off, int len) throws IOException { + return stream.read(b, off, len); + } + + @Override public String toString() { return String.format("0x%04x[%d-%d]", marker, realStart, realEnd()); } } + + /** + * Workaround for a known bug in com.sun.imageio.plugins.jpeg.AdobeMarkerSegment, leaving the buffer in an + * inconsistent state, if the length of the APP14/Adobe is not exactly 16 bytes. + * + * @see Bug report + */ + static final class AdobeAPP14Replacement extends ReplacementSegment { + + AdobeAPP14Replacement(final long realStart, final long start, final long realLength, final ImageInputStream stream) throws IOException { + super(JPEG.APP14, realStart, start, realLength, createMarkerFixedLength(stream)); + } + + private static byte[] createMarkerFixedLength(final ImageInputStream stream) throws IOException { + byte[] segmentData = new byte[16]; + + segmentData[0] = (byte) ((JPEG.APP14 >> 8) & 0xff); + segmentData[1] = (byte) (JPEG.APP14 & 0xff); + segmentData[2] = (byte) 0; + segmentData[3] = (byte) 14; + + stream.readFully(segmentData, 4, segmentData.length - 4); + + return segmentData; + } + } + + static class ReplacementSegment extends Segment { + final long realLength; + final byte[] data; + + int pos; + + ReplacementSegment(final int marker, final long realStart, final long start, final long realLength, final byte[] replacementData) { + super(marker, realStart, start, replacementData.length); + this.realLength = realLength; + this.data = replacementData; + } + + @Override + long realEnd() { + return realStart + realLength; + } + + @Override + public void seek(final ImageInputStream stream, final long newPos) throws IOException { + pos = (int) (newPos - realStart); + stream.seek(newPos); + } + + @Override + public int read(final ImageInputStream stream) { + return data[pos++] & 0xff; + } + + @Override + public int read(final ImageInputStream stream, byte[] b, int off, int len) { + int length = Math.min(data.length - pos, len); + System.arraycopy(data, pos, b, off, length); + pos += length; + + return length; + } + } } diff --git a/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStreamTest.java b/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStreamTest.java index 4154b77b..f6af554b 100644 --- a/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStreamTest.java +++ b/imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStreamTest.java @@ -111,12 +111,15 @@ public class JPEGSegmentImageInputStreamTest { ImageInputStream stream = new JPEGSegmentImageInputStream(ImageIO.createImageInputStream(getClassLoaderResource("/jpeg/no-image-types-rgb-us-web-coated-v2-ms-photogallery-exif.jpg"))); List appSegments = JPEGSegmentUtil.readSegments(stream, JPEGSegmentUtil.APP_SEGMENTS); - assertEquals(1, appSegments.size()); + assertEquals(2, appSegments.size()); assertEquals(JPEG.APP1, appSegments.get(0).marker()); assertEquals("Exif", appSegments.get(0).identifier()); - // And thus, no JFIF, no Adobe, no XMP, no ICC_PROFILE or other segments + assertEquals(JPEG.APP14, appSegments.get(1).marker()); + assertEquals("Adobe", appSegments.get(1).identifier()); + + // And thus, no JFIF, no XMP, no ICC_PROFILE or other segments } @Test