[10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Jayathirth D v

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

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Prahalad kumar Narayanan
Hello Jay

Good day to you. I looked into the changes.
Here are my views-

    . As per the code changes in the lines mentioned below, every "Throwable" (other than the 4 exceptions listed in the first catch block) is considered- "BufferOverflow/OOM".  This is incorrect.
          . Say for example, a future change to code within readImage method resulted in a NullPointerException, this would get incorrectly reported as- BufferOverflow/ OOM as per the changes.

    . Secondly, throwing one IIOException with a combined message will confuse the programmer- Was it OOM ? or was it BufferOverflow that caused the reader to exit ?
          . If the intent is to process and throw IIOException with meaningful message, I believe, we should handle them separately and throw one IIOException for each with a suitable message.

    1675         } catch (IOException |
    1676                  IndexOutOfBoundsException |
    1677                  IllegalStateException |
    1678                  IllegalArgumentException e)
    1679         {
    1680             throw e;
    1681         } catch (Throwable e) {
    1682             throw new IIOException("BufferOverflow/OOM while reading"
    1683                     + " the image");
    1684         }

Thanks
Have a good day

Prahalad

----- Original Message -----
From: Jayathirth D V
Sent: Monday, November 13, 2017 2:53 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

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
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Phil Race
In reply to this post by Jayathirth D v
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

 


Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Jayathirth D v

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

 

 

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Prahalad kumar Narayanan
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Jayathirth D v
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Prahalad kumar Narayanan
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

Jayathirth D v
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
 
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8190332: PngReader throws NegativeArraySizeException/OOM error when IHDR width is very large

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