#329 JPEGImageReader endless loop fix

imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/Application.java

@@ -90,7 +90,7 @@ class Application extends Segment {
 
             default:
                 // Generic APPn segment
-                byte[] bytes = new byte[length - 2];
+                byte[] bytes = new byte[Math.max(0, length - 2)];
                 data.readFully(bytes);
                 return new Application(marker, identifier, bytes);
         }

imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java

@@ -233,7 +233,14 @@ public final class JPEGImageReader extends ImageReaderBase {
 
     @Override
     public Iterator<ImageTypeSpecifier> getImageTypes(int imageIndex) throws IOException {
-        Iterator<ImageTypeSpecifier> types = delegate.getImageTypes(imageIndex);
+        Iterator<ImageTypeSpecifier> types;
+        try {
+            types = delegate.getImageTypes(imageIndex);
+        }
+        catch (IndexOutOfBoundsException | NegativeArraySizeException ignore) {
+            types = null;
+        }
+
         JPEGColorSpace csType = getSourceCSType(getJFIF(), getAdobeDCT(), getSOF());
 
         if (types == null || !types.hasNext() || csType == JPEGColorSpace.CMYK || csType == JPEGColorSpace.YCCK) {
@@ -302,7 +309,7 @@ public final class JPEGImageReader extends ImageReaderBase {
                 return rawType;
             }
         }
-        catch (IIOException | NullPointerException ignore) {
+        catch (IIOException | NullPointerException | ArrayIndexOutOfBoundsException | NegativeArraySizeException ignore) {
             // Fall through
         }
 
@@ -933,7 +940,13 @@ public final class JPEGImageReader extends ImageReaderBase {
             return new JPEGLosslessDecoderWrapper(this).readRaster(segments, imageInput);
         }
 
-        return delegate.readRaster(imageIndex, param);
+        try {
+            return delegate.readRaster(imageIndex, param);
+        }
+        catch (IndexOutOfBoundsException knownIssue) {
+            // com.sun.imageio.plugins.jpeg.JPEGBuffer doesn't do proper sanity check of input data.
+            throw new IIOException("Corrupt JPEG data: Bad segment length", knownIssue);
+        }
     }
 
     @Override

imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStream.java

@@ -40,6 +40,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
+import static com.twelvemonkeys.imageio.metadata.jpeg.JPEGSegmentUtil.isKnownJPEGMarker;
 import static com.twelvemonkeys.lang.Validate.notNull;
 import static java.util.Arrays.copyOf;
 
@@ -105,24 +106,25 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl {
 
             // Scan forward
             while (true) {
-                long realPosition = stream.getStreamPosition();
-
                 int trash = 0;
                 int marker = stream.readUnsignedByte();
 
-                // Skip bad padding before the marker
+                while (!isKnownJPEGMarker(marker)) {
-                while (marker != 0xff) {
+                    marker &= 0xff;
-                    marker = stream.readUnsignedByte();
-                    trash++;
-                    realPosition++;
-                }
 
-                marker = 0xff00 | stream.readUnsignedByte();
+                    // Skip bad padding before the marker
+                    while (marker != 0xff) {
+                        marker = stream.readUnsignedByte();
+                        trash++;
+                    }
 
-                // Skip over 0xff padding between markers
-                while (marker == 0xffff) {
-                    realPosition++;
                     marker = 0xff00 | stream.readUnsignedByte();
+
+                    // Skip over 0xff padding between markers
+                    while (marker == 0xffff) {
+                        marker = 0xff00 | stream.readUnsignedByte();
+                        trash++;
+                    }
                 }
 
                 if (trash != 0) {
@@ -131,6 +133,8 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl {
                     processWarningOccured(String.format("Corrupt JPEG data: %d extraneous bytes before marker 0x%02x", trash, marker & 0xff));
                 }
 
+                long realPosition = stream.getStreamPosition() - 2;
+
                 // We are now handling all important segments ourselves, except APP1/Exif and APP14/Adobe,
                 // as these segments affects image decoding.
                 boolean appSegmentMarker = isAppSegmentMarker(marker);
@@ -380,6 +384,7 @@ final class JPEGSegmentImageInputStream extends ImageInputStreamImpl {
                 fetchSegment();
             }
             catch (EOFException ignore) {
+                segments.add(new Segment(0, segment.realEnd(), segment.end(), Integer.MAX_VALUE * 2L - segment.realEnd()));
                 // This might happen if the segment lengths in the stream are bad.
                 // We MUST leave internal state untouched in this case.
                 // We ignore this exception here, but client code will get
@@ -415,7 +420,7 @@ 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 : segment.read(stream, 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

imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java

@@ -119,8 +119,15 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTest<JPEGImageReader
                 new TestData(getClassLoaderResource("/broken-jpeg/broken-invalid-adobe-ycc-gray.jpg"), new Dimension(11, 440)), // Image readable, broken metadata (fixable?)
                 new TestData(getClassLoaderResource("/broken-jpeg/broken-no-sof-ascii-transfer-mode.jpg"), new Dimension(-1, -1)), // Unreadable, can't find SOFn marker
                 new TestData(getClassLoaderResource("/broken-jpeg/broken-sos-before-sof.jpg"), new Dimension(-1, -1)), // Unreadable, can't find SOFn marker
-                new TestData(getClassLoaderResource("/broken-jpeg/broken-adobe-segment-length-beyond-eof.jpg"), new Dimension(-1, -1)) // Unreadable, no EOI
+                new TestData(getClassLoaderResource("/broken-jpeg/broken-adobe-segment-length-beyond-eof.jpg"), new Dimension(-1, -1)), // Unreadable, no EOI
-        );
+                new TestData(getClassLoaderResource("/broken-jpeg/513f29d0-02a8-11e7-9756-6035edb96e79.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/51432b02-02a8-11e7-9203-b42b1c43c0c3.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/5145e95a-02a8-11e7-8372-4787a7307ab8.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/514b20dc-02a8-11e7-92c6-d4fed7b4ebb1.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/514c48ea-02a8-11e7-8789-bb75321f404f.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/514e4122-02a8-11e7-8c03-0830d60cd585.jpg"), new Dimension(-1, -1)),
+                new TestData(getClassLoaderResource("/broken-jpeg/513f29d0-02a8-11e7-9756-6035edb96e79.jpg"), new Dimension(-1, -1))
+                );
 
         // More test data in specific tests below
     }
@@ -446,7 +453,7 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTest<JPEGImageReader
 
     @Test
     public void testBrokenReadRasterAfterGetMetadataException() throws IOException {
-        // See issue 107, from PDFBox team
+        // See issue #107, from PDFBox team
         JPEGImageReader reader = createReader();
 
         try {
@@ -458,7 +465,6 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTest<JPEGImageReader
                 }
                 catch (IOException ignore) {
                     // Expected IOException here, due to broken file
-//                    ignore.printStackTrace();
                 }
 
                 try {
@@ -477,6 +483,122 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTest<JPEGImageReader
         }
     }
 
+    @Test
+    public void testSPIRecognizesBrokenJPEG() throws IOException {
+        // TODO: There's a bug in com.sun.imageio.plugins.png.PNGImageReaderSpi.canDecode
+        // causing files < 8 bytes to not be recognized as anything...
+        ImageReaderSpi provider = createProvider();
+        for (TestData data : getBrokenTestData()) {
+            assertTrue(data.toString(), provider.canDecodeInput(data.getInputStream()));
+        }
+    }
+
+    // TODO: Consider wrapping the delegate in JPEGImageReader with methods that don't throw
+    // runtime exceptions, and instead throw IIOException?
+    @Test
+    public void testBrokenGetRawImageType() throws IOException {
+        JPEGImageReader reader = createReader();
+
+        try {
+            for (TestData broken : getBrokenTestData()) {
+                reader.setInput(broken.getInputStream());
+
+                try {
+                    reader.getRawImageType(0);
+                }
+                catch (IIOException expected) {
+                    assertNotNull(expected.getMessage());
+                }
+                catch (IOException expected) {
+                    if (!(expected instanceof EOFException)) {
+                        assertNotNull(expected.getMessage());
+                    }
+                }
+            }
+        }
+        finally {
+            reader.dispose();
+        }
+    }
+
+    @Test(timeout = 200)
+    public void testBrokenGetRawImageTypeIgnoreMetadata() throws IOException {
+        JPEGImageReader reader = createReader();
+
+        try {
+            for (TestData broken : getBrokenTestData()) {
+                reader.setInput(broken.getInputStream(), true, true);
+
+                try {
+                    reader.getRawImageType(0);
+                }
+                catch (IIOException expected) {
+                    assertNotNull(expected.getMessage());
+                }
+                catch (IOException expected) {
+                    if (!(expected instanceof EOFException)) {
+                        assertNotNull(expected.getMessage());
+                    }
+                }
+            }
+        }
+        finally {
+            reader.dispose();
+        }
+    }
+
+    @Test
+    public void testBrokenGetImageTypes() throws IOException {
+        JPEGImageReader reader = createReader();
+
+        try {
+            for (TestData broken : getBrokenTestData()) {
+                reader.setInput(broken.getInputStream());
+
+                try {
+                    reader.getImageTypes(0);
+                }
+                catch (IIOException expected) {
+                    assertNotNull(expected.getMessage());
+                }
+                catch (IOException expected) {
+                    if (!(expected instanceof EOFException)) {
+                        assertNotNull(expected.getMessage());
+                    }
+                }
+            }
+        }
+        finally {
+            reader.dispose();
+        }
+    }
+
+    @Test(timeout = 200)
+    public void testBrokenGetImageTypesIgnoreMetadata() throws IOException {
+        JPEGImageReader reader = createReader();
+
+        try {
+            for (TestData broken : getBrokenTestData()) {
+                reader.setInput(broken.getInputStream(), true, true);
+
+                try {
+                    reader.getImageTypes(0);
+                }
+                catch (IIOException expected) {
+                    assertNotNull(expected.getMessage());
+                }
+                catch (IOException expected) {
+                    if (!(expected instanceof EOFException)) {
+                        assertNotNull(expected.getMessage());
+                    }
+                }
+            }
+        }
+        finally {
+            reader.dispose();
+        }
+    }
+
     @Test
     public void testBrokenRead() throws IOException {
         JPEGImageReader reader = createReader();

imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGSegmentImageInputStreamTest.java

@@ -172,6 +172,32 @@ public class JPEGSegmentImageInputStreamTest {
         assertEquals(1061L, length); // Sanity check: same as file size, except padding and the filtered ICC_PROFILE segment
     }
 
+    @Test
+    public void testEOFExceptionInSegmentParsingShouldNotCreateBadState2() throws IOException {
+        ImageInputStream iis = new JPEGSegmentImageInputStream(ImageIO.createImageInputStream(getClassLoaderResource("/broken-jpeg/51432b02-02a8-11e7-9203-b42b1c43c0c3.jpg")));
+
+        byte[] buffer = new byte[4096];
+
+        // NOTE: This is a simulation of how the native parts of com.sun...JPEGImageReader would read the image...
+        assertEquals(2, iis.read(buffer, 0, buffer.length));
+        assertEquals(2, iis.getStreamPosition());
+
+        iis.seek(2000); // Just a random postion beyond EOF
+        assertEquals(2000, iis.getStreamPosition());
+
+        // So far, so good (but stream position is now really beyond EOF)...
+
+        // This however, will blow up with an EOFException internally (but we'll return -1 to be good)
+        assertEquals(-1, iis.read(buffer, 0, buffer.length));
+        assertEquals(-1, iis.read());
+        assertEquals(2000, iis.getStreamPosition());
+
+        // Again, should just continue returning -1 for ever
+        assertEquals(-1, iis.read());
+        assertEquals(-1, iis.read(buffer, 0, buffer.length));
+        assertEquals(2000, iis.getStreamPosition());
+    }
+
     @Test
     public void testEOFExceptionInSegmentParsingShouldNotCreateBadState() throws IOException {
         ImageInputStream iis = new JPEGSegmentImageInputStream(ImageIO.createImageInputStream(getClassLoaderResource("/broken-jpeg/broken-no-sof-ascii-transfer-mode.jpg")));
@@ -189,10 +215,12 @@ public class JPEGSegmentImageInputStreamTest {
 
         // This however, will blow up with an EOFException internally (but we'll return -1 to be good)
         assertEquals(-1, iis.read(buffer, 0, buffer.length));
+        assertEquals(-1, iis.read());
         assertEquals(0x2012, iis.getStreamPosition());
 
         // Again, should just continue returning -1 for ever
         assertEquals(-1, iis.read(buffer, 0, buffer.length));
+        assertEquals(-1, iis.read());
         assertEquals(0x2012, iis.getStreamPosition());
     }
 }

imageio/imageio-metadata/src/main/java/com/twelvemonkeys/imageio/metadata/jpeg/JPEGSegmentUtil.java

@@ -165,22 +165,24 @@ public final class JPEGSegmentUtil {
 //        int trash = 0;
         int marker = stream.readUnsignedByte();
 
-        // Skip trash padding before the marker
+        while (!isKnownJPEGMarker(marker)) {
-        while (marker != 0xff) {
+            // Skip trash padding before the marker
-            marker = stream.readUnsignedByte();
+            while (marker != 0xff) {
+                marker = stream.readUnsignedByte();
 //            trash++;
-        }
+            }
 
 //        if (trash != 0) {
             // TODO: Issue warning?
 //            System.err.println("trash: " + trash);
 //        }
 
-        marker = 0xff00 | stream.readUnsignedByte();
-
-        // Skip over 0xff padding between markers
-        while (marker == 0xffff) {
             marker = 0xff00 | stream.readUnsignedByte();
+
+            // Skip over 0xff padding between markers
+            while (marker == 0xffff) {
+                marker = 0xff00 | stream.readUnsignedByte();
+            }
         }
 
         if ((marker >> 8 & 0xff) != 0xff) {
@@ -192,7 +194,7 @@ public final class JPEGSegmentUtil {
         byte[] data;
 
         if (segmentIdentifiers.containsKey(marker)) {
-            data = new byte[length - 2];
+            data = new byte[Math.max(0, length - 2)];
             stream.readFully(data);
         }
         else {
@@ -218,6 +220,57 @@ public final class JPEGSegmentUtil {
         return new JPEGSegment(marker, data, length);
     }
 
+    public static boolean isKnownJPEGMarker(final int marker) {
+        switch (marker) {
+            case JPEG.SOI:
+            case JPEG.EOI:
+            case JPEG.DHT:
+            case JPEG.SOS:
+            case JPEG.DQT:
+            case JPEG.COM:
+            case JPEG.SOF0:
+            case JPEG.SOF1:
+            case JPEG.SOF2:
+            case JPEG.SOF3:
+            case JPEG.SOF5:
+            case JPEG.SOF6:
+            case JPEG.SOF7:
+            case JPEG.SOF9:
+            case JPEG.SOF10:
+            case JPEG.SOF11:
+            case JPEG.SOF13:
+            case JPEG.SOF14:
+            case JPEG.SOF15:
+            case JPEG.SOF55:
+            case JPEG.APP0:
+            case JPEG.APP1:
+            case JPEG.APP2:
+            case JPEG.APP3:
+            case JPEG.APP4:
+            case JPEG.APP5:
+            case JPEG.APP6:
+            case JPEG.APP7:
+            case JPEG.APP8:
+            case JPEG.APP9:
+            case JPEG.APP10:
+            case JPEG.APP11:
+            case JPEG.APP12:
+            case JPEG.APP13:
+            case JPEG.APP14:
+            case JPEG.APP15:
+            case JPEG.DRI:
+            case JPEG.TEM:
+            case JPEG.DAC:
+            case JPEG.DHP:
+            case JPEG.DNL:
+            case JPEG.EXP:
+            case JPEG.LSE:
+                return true;
+            default:
+                return false;
+        }
+    }
+
     private static class AllIdsList extends ArrayList<String> {
         @Override
         public String toString() {