Hello Jay The change looks good.
I just noticed the braces ' { ' for the first catch block is on a new line. It's a minor observation, does not need another webrev. Thanks Have a good day Prahalad -----Original Message----- From: Jayathirth D V Sent: Thursday, November 16, 2017 2:06 PM To: Prahalad Kumar Narayanan; Philip Race; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large Hi Prahalad, Thanks for your inputs. I have updated comments in PNGImageReader and updated the jtreg comment in test case. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8190332/webrev.02/ Thanks, Jay -----Original Message----- From: Prahalad Kumar Narayanan Sent: Wednesday, November 15, 2017 6:22 PM To: Jayathirth D V; Philip Race; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large Hello Jay Good day to you. Overall, the code change looks good. Few minor changes to the comment lines: . You may re-phrase the new comments introduced at Line 1366. . Please include the comments that explained recover strategy. This got deleted in your change. . The new comments imply that OOM is wrapped and thrown to the caller. . However, the deleted comments substantiate why such a decision was taken (we will not estimate memory requirements). Helps the programmer in future. . Modified comments lines (for reference) 1365 * 1366 * If the read operation triggers OutOfMemoryError, the same 1367 * will be wrapped in an IIOException at PNGImageReader.read 1368 * method. 1369 * 1370 * The recovery strategy for this case should be 1371 * defined on the level of application, so we will not 1372 * try to estimate the required amount of the memory and/or 1373 * handle OOM in any way 1374 */ . The jtreg comments in the test-case have a Javadoc comment style rather than multi-line comment. . You may change it or retain the current style- as I do see many test files in repo with Javadoc style comments. . Reference link: http://openjdk.java.net/jtreg/faq.html#question3.1 Thanks Have a good day Prahalad ----- Original Message ----- From: Jayathirth D V Sent: Tuesday, November 14, 2017 3:44 PM To: Philip Race; Prahalad Kumar Narayanan; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large Hi, Thanks for the review Phil and Prahalad. Based on the suggestions I have updated the code to wrap underlying Exception/Error in IIOException instead of overriding the message content and removed the check for IndexOutOfBoundsException. Corresponding test case changes are also done. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8190332/webrev.01/ Thanks, Jay From: Phil Race Sent: Tuesday, November 14, 2017 12:06 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large 1676 IndexOutOfBoundsException | IndexOutOfBoundsException is a specified exception but as such is thrown outside this try block so I think you should not re-throw it here but should let it be handled by the block below. throw new IIOException("BufferOverflow/OOM while reading" 1683 + " the image"); Whilst that's the issue here I think this message will look quite odd if what we actually had thrown is something like ClassCastException so I think you should leave it to the underlying exception to report the issue. Also I had said to wrap the original exception, so what I expected was throw new IIOException("Caught exception during read: ", e); -phil. On 11/13/2017 01:23 AM, Jayathirth D V wrote: Hello All, Please review the following fix in JDK10 : Bug : https://bugs.openjdk.java.net/browse/JDK-8190332 Webrev : http://cr.openjdk.java.net/~jdv/8190332/webrev.00/ Issue : Two types of issues are fixed under the same solution, so JDK-8190511 is closed as duplicate of this issue. 1) PNGImageReader throws OOM error when IHDR width equals/or greater than VM's array size limit. 2) PNGImageReader throws NegativeArraySizeException when IHDR width value is very high. Root cause : 1) VM doesn't allow creation of array with Size >= ((2 to the power of 31) - 2) so we throw OOM error. 2) We use width from IHDR to calculate needed "bytesPerRow" value as "(inputBands*passWidth*bitDepth + 7)/8". When IHDR width is very high we overflow the integer size of bytesPerRow and when we try to create array using bytesPerRow it throws NegativeArraySizeException. Solution : According to PNG spec maximum value that can be stored in IHDR width/height is (2 to the power of 31). We can't support PNG images with so large height/width, because based on other parameters like inputsBands, bitDepth we will definitely overflow than the maximum buffer value of VM. Also PNG is not a preferred format for such kind of large images. Instead of catching these OOMError/NegativeArraySizeException at many different levels in PNGImageReader we can catch Throwable at higher level and convert OOMError/ NegativeArraySizeException into IIOException. Thanks, Jay