[OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException

[Prasanta Sadhukhan]

+1

Regards
Prasanta
On 11/21/2017 9:06 AM, Prahalad Kumar Narayanan wrote:
> Hello Jay
>
> The change looks good.
>
> Thanks
> Have a good day
>
> Prahalad
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Tuesday, November 21, 2017 9:01 AM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException
>
> Hi Prahalad,
>
> Thanks for the review.
> I merged the two functions into one instead of moving BufferedImage code into the two functions as it reduces the LOC by good amount.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191431/webrev.02/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Tuesday, November 21, 2017 8:13 AM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException
>
> Hello Jay
>
> Good day to you. I looked into the modified test code.
>
> The test file has two methods ( writeAndReadImageWithPalette & writeAndReadImageWithoutPalette ) with exactly the same implementation.
>      . Can we merge them into one method ?
>      . Another option would be to move the logic that prepares required buffered image within these methods.
>
>    86     private static void writeAndReadImageWithoutPalette(BufferedImage image)
>    87             throws IOException {
>    88         File output = File.createTempFile("output", ".png");
>    89         ImageInputStream stream = null;
>    90         try {
>    91             ImageIO.write(image, "png", output);
>    92
>    93             stream = ImageIO.createImageInputStream(output);
>    94             ImageReadParam param = PNG_READER.getDefaultReadParam();
>    95             PNG_READER.setInput(stream, true, true);
>    96             PNG_READER.read(0, param);
>    97         } finally {
>    98             if (stream != null) {
>    99                 stream.close();
>   100             }
>   101             Files.delete(output.toPath());
>   102         }
>   103     }
>
>   105     private static void writeAndReadImageWithPalette(BufferedImage image)
>   106             throws IOException {
>   107         File output = File.createTempFile("output", ".png");
>   108         ImageInputStream stream = null;
>   109         try {
>   110             ImageIO.write(image, "png", output);
>   111
>   112             stream = ImageIO.createImageInputStream(output);
>   113             ImageReadParam param = PNG_READER.getDefaultReadParam();
>   114             PNG_READER.setInput(stream, true, true);
>   115             PNG_READER.read(0, param);
>   116         } finally {
>   117             if (stream != null) {
>   118                 stream.close();
>   119             }
>   120             Files.delete(output.toPath());
>   121         }
>   122     }
>   123 }
>
> Thank you
> Have a good day
>
> Prahalad
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Monday, November 20, 2017 6:18 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException
>
> Hi Prahalad,
>
> Thanks for the review.
> I have updated the test case.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191431/webrev.01/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Monday, November 20, 2017 5:25 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException
>
> Hello Jay
>
> The code change at PNGImageReader is correct.
> But the test wouldn't delete temporary files when exception occurs.
>
> Thank you
> Have a good day
>
> Prahalad
>
> ----- Original Message -----
> From: Jayathirth D V
> Sent: Monday, November 20, 2017 4:34 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positions will cause IIOException
>
> Hello All,
>
> Please review the following fix in JDK10 :
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-8191431
> Webrev : http://cr.openjdk.java.net/~jdv/8191431/webrev.00/
>
> Issue : When we try to read multiple PNG images with different IDAT chunk positions using the same PNGImageReader instance we get "IIOException: Error reading PNG image data".
>
> Root cause : Issue is happening because of changes present in JDK-8164971.
>                  Under JDK-8164971 we have made changes such that imageStartPosition for IDAT chunk will be updated only once for a given PNGImageReader instance while reading metadata.
>                  case IDAT_TYPE:
>                      // If chunk type is 'IDAT', we've reached the image data.
>                      if (imageStartPosition == -1L) {
>                          /*
>                           * PNGs may contain multiple IDAT chunks containing
>                           * a portion of image data. We store the position of
>                           * the first IDAT chunk and continue with iteration
>                           * of other chunks that follow image data.
>                           */
>                          imageStartPosition = stream.getStreamPosition() - 8;
>                     }
>
>                  When we use same PNGImageReader instance to read another PNG image and if IDAT chunk position and length differs we will get IIOException.
>
> Solution : We already have resetStreamSettings() method which we call when user tries to decode an image under ImageReader.setInput(). In resetStreamSettings() function if we initialize imageStartPosition to '-1L' it will resolve this issue.
>
> Thanks,
> Jay