+1
-phil.
On 11/16/2017 02:52 AM, Jayathirth D V wrote:
Hi Prahalad,
Thanks for the approval after review.
I explicitly added "{" in new line because of multiline condition in catch
block. I was advised previously to do so when there are multiline conditions before a
block from readability
perspective(http://mail.openjdk.java.net/pipermail/2d-dev/2016-April/006677.html ).
If Phil/Others can also give clarification on this, if needed I will make this
minor correction.
Regards,
Jay
-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Thursday, November 16, 2017 2:11 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
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