'Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error whe'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error whe
From:       Phil Race <philip.race () oracle ! com>
Date:       2017-11-17 18:30:50
Message-ID: 96d94e84-8690-b5df-0a1b-397a2c480ce5 () oracle ! com
[Download RAW message or body]

+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
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic