RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

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

RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Jayathirth D v

Hello All,

 

Please review the following fix in JDK :

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8191174

Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/

 

Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".

 

Exception in thread "main" java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride

                at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelInterleavedSampleModel.java:101)

                at java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java:642)

                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(PNGImageReader.java:974)

                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNGImageReader.java:1099)

                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PNGImageReader.java:1295)

                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1420)

                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1699)

                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)

                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)

                at PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:50)

 

Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.

 

Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.

 

Thanks,

Jay

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Prahalad kumar Narayanan

Hello Jay

Good day to you.

I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
Here is a simple math that considers the worst case scenario with max values:

. As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
      max_width : (2 ^ 31) - 1 = 2147483647
      max_input_bands = 4
      max_bit_depth = 16 (2 Bytes)

. As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
      // Fits into "long" data type
      max_bits_per_row = max_width * max_input_bands * max_bit_depth
                                             = 2147483647 x 4 x 16
                                             = 137438953408

      // Does not fit into "int" data type
      max_bytes_per_row = max_bits_per_row + 7 / 8
                                                 = 17179869176
                                                 = (0x 3FFFFFFF8 - Goes beyond 4B boundary)

      // Upon division by 2 for 16-bit images
      max_elts_per_row = max_bytes_per_row / 2
                                             = 8589934588
                                             = (0x 1FFFFFFFC - Goes beyond 4B boundary)

. If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
  Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.

Thank you
Have a good day

Prahalad N.

----- Original Message -----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 1:48 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hello All,

Please review the following fix in JDK :

Bug : https://bugs.openjdk.java.net/browse/JDK-8191174 
Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/ 

Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".

Exception in thread "main" java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
                at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelInterleavedSampleModel.java:101)
                at java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java:642)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(PNGImageReader.java:974)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNGImageReader.java:1099)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PNGImageReader.java:1295)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1420)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1699)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
                at PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:50)

Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.

Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Jayathirth D v
Hi Prahalad,

Thanks for your valuable inputs.

Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.

Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.

Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191174/webrev.01/ 

Thanks,
Jay

-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Tuesday, December 19, 2017 3:08 PM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper


Hello Jay

Good day to you.

I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
Here is a simple math that considers the worst case scenario with max values:

. As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
      max_width : (2 ^ 31) - 1 = 2147483647
      max_input_bands = 4
      max_bit_depth = 16 (2 Bytes)

. As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
      // Fits into "long" data type
      max_bits_per_row = max_width * max_input_bands * max_bit_depth
                                             = 2147483647 x 4 x 16
                                             = 137438953408

      // Does not fit into "int" data type
      max_bytes_per_row = max_bits_per_row + 7 / 8
                                                 = 17179869176
                                                 = (0x 3FFFFFFF8 - Goes beyond 4B boundary)

      // Upon division by 2 for 16-bit images
      max_elts_per_row = max_bytes_per_row / 2
                                             = 8589934588
                                             = (0x 1FFFFFFFC - Goes beyond 4B boundary)

. If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
  Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.

Thank you
Have a good day

Prahalad N.

----- Original Message -----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 1:48 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hello All,

Please review the following fix in JDK :

Bug : https://bugs.openjdk.java.net/browse/JDK-8191174 
Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/ 

Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".

Exception in thread "main" java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
                at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelInterleavedSampleModel.java:101)
                at java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java:642)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(PNGImageReader.java:974)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNGImageReader.java:1099)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PNGImageReader.java:1295)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1420)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1699)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
                at PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:50)

Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.

Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Prahalad kumar Narayanan
Hello Jay

Good day to you.

The code changes wrap the IllegalArgumentException in IIOException.
This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.

The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.

javax.imageio.IIOException: Error reading PNG image data
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
                at java.desktop/javax.imageio.ImageIO.read
                ...
Caused by: javax.imageio.IIOException: Caught exception during read:
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
                ...
Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
                at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
                at java.desktop/java.awt.image.Raster.createInterleavedRaster
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster

Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
So I think, this should be ok.

I would like to know of others' opinion too.

Thank you
Have a good day

Prahalad N.


-----Original Message-----
From: Jayathirth D V
Sent: Thursday, December 28, 2017 3:43 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hi Prahalad,

Thanks for your valuable inputs.

Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.

Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.

Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191174/webrev.01/ 

Thanks,
Jay

-----Original Message-----
From: Prahalad Kumar Narayanan
Sent: Tuesday, December 19, 2017 3:08 PM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper


Hello Jay

Good day to you.

I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
Here is a simple math that considers the worst case scenario with max values:

. As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
      max_width : (2 ^ 31) - 1 = 2147483647
      max_input_bands = 4
      max_bit_depth = 16 (2 Bytes)

. As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
      // Fits into "long" data type
      max_bits_per_row = max_width * max_input_bands * max_bit_depth
                                             = 2147483647 x 4 x 16
                                             = 137438953408

      // Does not fit into "int" data type
      max_bytes_per_row = max_bits_per_row + 7 / 8
                                                 = 17179869176
                                                 = (0x 3FFFFFFF8 - Goes beyond 4B boundary)

      // Upon division by 2 for 16-bit images
      max_elts_per_row = max_bytes_per_row / 2
                                             = 8589934588
                                             = (0x 1FFFFFFFC - Goes beyond 4B boundary)

. If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
  Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.

Thank you
Have a good day

Prahalad N.

----- Original Message -----
From: Jayathirth D V
Sent: Thursday, December 14, 2017 1:48 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hello All,

Please review the following fix in JDK :

Bug : https://bugs.openjdk.java.net/browse/JDK-8191174 
Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/ 

Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".

Exception in thread "main" java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
                at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelInterleavedSampleModel.java:101)
                at java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java:642)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(PNGImageReader.java:974)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNGImageReader.java:1099)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PNGImageReader.java:1295)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1420)
                at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1699)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
                at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
                at PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:50)

Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.

Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Sergey Bylokhov
Probably we can use Math.multiplyExact()? The current exception text
wrapped a few times "Pixel stride times width must be less than or equal
to the scanline stride" is not strictly correct because it is possible
that the image itself has correct data, but we cannot handle it because
of overflow.

On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:

> Hello Jay
>
> Good day to you.
>
> The code changes wrap the IllegalArgumentException in IIOException.
> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>
> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>
> javax.imageio.IIOException: Error reading PNG image data
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>                  at java.desktop/javax.imageio.ImageIO.read
>                  ...
> Caused by: javax.imageio.IIOException: Caught exception during read:
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>                  ...
> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>                  at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>                  at java.desktop/java.awt.image.Raster.createInterleavedRaster
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>
> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
> So I think, this should be ok.
>
> I would like to know of others' opinion too.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, December 28, 2017 3:43 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper
>
> Hi Prahalad,
>
> Thanks for your valuable inputs.
>
> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>
> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Tuesday, December 19, 2017 3:08 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper
>
>
> Hello Jay
>
> Good day to you.
>
> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
> Here is a simple math that considers the worst case scenario with max values:
>
> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>        max_width : (2 ^ 31) - 1 = 2147483647
>        max_input_bands = 4
>        max_bit_depth = 16 (2 Bytes)
>
> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>        // Fits into "long" data type
>        max_bits_per_row = max_width * max_input_bands * max_bit_depth
>                                               = 2147483647 x 4 x 16
>                                               = 137438953408
>
>        // Does not fit into "int" data type
>        max_bytes_per_row = max_bits_per_row + 7 / 8
>                                                   = 17179869176
>                                                   = (0x 3FFFFFFF8 - Goes beyond 4B boundary)
>
>        // Upon division by 2 for 16-bit images
>        max_elts_per_row = max_bytes_per_row / 2
>                                               = 8589934588
>                                               = (0x 1FFFFFFFC - Goes beyond 4B boundary)
>
> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>    Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
> ----- Original Message -----
> From: Jayathirth D V
> Sent: Thursday, December 14, 2017 1:48 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper
>
> Hello All,
>
> Please review the following fix in JDK :
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>
> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>
> Exception in thread "main" java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>                  at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelInterleavedSampleModel.java:101)
>                  at java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java:642)
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(PNGImageReader.java:974)
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNGImageReader.java:1099)
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PNGImageReader.java:1295)
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGImageReader.java:1420)
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageReader.java:1699)
>                  at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>                  at java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>                  at PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:50)
>
> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>
> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>
> Thanks,
> Jay
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Jayathirth D v
Hello Sergey,

Thanks for your valuable inputs.

As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .

By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191174/webrev.02/ 

Thanks,
Jay

-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 06, 2018 2:45 PM
To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.

On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:

> Hello Jay
>
> Good day to you.
>
> The code changes wrap the IllegalArgumentException in IIOException.
> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>
> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>
> javax.imageio.IIOException: Error reading PNG image data
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>                  at java.desktop/javax.imageio.ImageIO.read
>                  ...
> Caused by: javax.imageio.IIOException: Caught exception during read:
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>                  ...
> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>                  at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>                  at java.desktop/java.awt.image.Raster.createInterleavedRaster
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>
> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
> So I think, this should be ok.
>
> I would like to know of others' opinion too.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, December 28, 2017 3:43 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Hi Prahalad,
>
> Thanks for your valuable inputs.
>
> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>
> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Tuesday, December 19, 2017 3:08 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
>
> Hello Jay
>
> Good day to you.
>
> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
> Here is a simple math that considers the worst case scenario with max values:
>
> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>        max_width : (2 ^ 31) - 1 = 2147483647
>        max_input_bands = 4
>        max_bit_depth = 16 (2 Bytes)
>
> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>        // Fits into "long" data type
>        max_bits_per_row = max_width * max_input_bands * max_bit_depth
>                                               = 2147483647 x 4 x 16
>                                               = 137438953408
>
>        // Does not fit into "int" data type
>        max_bytes_per_row = max_bits_per_row + 7 / 8
>                                                   = 17179869176
>                                                   = (0x 3FFFFFFF8 -
> Goes beyond 4B boundary)
>
>        // Upon division by 2 for 16-bit images
>        max_elts_per_row = max_bytes_per_row / 2
>                                               = 8589934588
>                                               = (0x 1FFFFFFFC - Goes
> beyond 4B boundary)
>
> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>    Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
> ----- Original Message -----
> From: Jayathirth D V
> Sent: Thursday, December 14, 2017 1:48 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Hello All,
>
> Please review the following fix in JDK :
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>
> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>
> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
> stride times width must be less than or equal to the scanline stride
>                  at
> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelIn
> terleavedSampleModel.java:101)
>                  at
> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java
> :642)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(P
> NGImageReader.java:974)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNG
> ImageReader.java:1099)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PN
> GImageReader.java:1295)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGI
> mageReader.java:1420)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageR
> eader.java:1699)
>                  at
> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>                  at
> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>                  at
> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
> 50)
>
> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>
> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>
> Thanks,
> Jay
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Prahalad kumar Narayanan
Hello Jay

The change looks good.

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Jayathirth D V
Sent: Monday, January 08, 2018 3:35 PM
To: Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hello Sergey,

Thanks for your valuable inputs.

As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .

By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191174/webrev.02/ 

Thanks,
Jay

-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 06, 2018 2:45 PM
To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.

On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:

> Hello Jay
>
> Good day to you.
>
> The code changes wrap the IllegalArgumentException in IIOException.
> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>
> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>
> javax.imageio.IIOException: Error reading PNG image data
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>                  at java.desktop/javax.imageio.ImageIO.read
>                  ...
> Caused by: javax.imageio.IIOException: Caught exception during read:
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>                  at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>                  ...
> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>                  at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>                  at java.desktop/java.awt.image.Raster.createInterleavedRaster
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>
> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
> So I think, this should be ok.
>
> I would like to know of others' opinion too.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, December 28, 2017 3:43 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Hi Prahalad,
>
> Thanks for your valuable inputs.
>
> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>
> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Tuesday, December 19, 2017 3:08 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
>
> Hello Jay
>
> Good day to you.
>
> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
> Here is a simple math that considers the worst case scenario with max values:
>
> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>        max_width : (2 ^ 31) - 1 = 2147483647
>        max_input_bands = 4
>        max_bit_depth = 16 (2 Bytes)
>
> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>        // Fits into "long" data type
>        max_bits_per_row = max_width * max_input_bands * max_bit_depth
>                                               = 2147483647 x 4 x 16
>                                               = 137438953408
>
>        // Does not fit into "int" data type
>        max_bytes_per_row = max_bits_per_row + 7 / 8
>                                                   = 17179869176
>                                                   = (0x 3FFFFFFF8 -
> Goes beyond 4B boundary)
>
>        // Upon division by 2 for 16-bit images
>        max_elts_per_row = max_bytes_per_row / 2
>                                               = 8589934588
>                                               = (0x 1FFFFFFFC - Goes
> beyond 4B boundary)
>
> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>    Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>
> Thank you
> Have a good day
>
> Prahalad N.
>
> ----- Original Message -----
> From: Jayathirth D V
> Sent: Thursday, December 14, 2017 1:48 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Hello All,
>
> Please review the following fix in JDK :
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>
> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>
> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
> stride times width must be less than or equal to the scanline stride
>                  at
> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelIn
> terleavedSampleModel.java:101)
>                  at
> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java
> :642)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(P
> NGImageReader.java:974)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNG
> ImageReader.java:1099)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PN
> GImageReader.java:1295)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGI
> mageReader.java:1420)
>                  at
> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageR
> eader.java:1699)
>                  at
> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>                  at
> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>                  at
> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
> 50)
>
> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>
> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>
> Thanks,
> Jay
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Brian Burkhalter-2
Hi Jay,

+1

Brian

On Jan 8, 2018, at 10:50 PM, Prahalad Kumar Narayanan <[hidden email]> wrote:

The change looks good.

[…]
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191174/webrev.02/ 

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Sergey Bylokhov
In reply to this post by Jayathirth D v
Hi, Jay.
Can you please confirm that it is not possible to get overflow in:
"inputBands * bitDepth"?
1051 int bitsPerRow = Math.multiplyExact((inputBands * bitDepth),
passWidth);

On 08/01/2018 02:04, Jayathirth D V wrote:

> As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
> Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .
>
> By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.02/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Saturday, January 06, 2018 2:45 PM
> To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper
>
> Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.
>
> On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:
>> Hello Jay
>>
>> Good day to you.
>>
>> The code changes wrap the IllegalArgumentException in IIOException.
>> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>>
>> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>>
>> javax.imageio.IIOException: Error reading PNG image data
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>>                   at java.desktop/javax.imageio.ImageIO.read
>>                   ...
>> Caused by: javax.imageio.IIOException: Caught exception during read:
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>>                   ...
>> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>>                   at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>>                   at java.desktop/java.awt.image.Raster.createInterleavedRaster
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>>
>> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
>> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
>> So I think, this should be ok.
>>
>> I would like to know of others' opinion too.
>>
>> Thank you
>> Have a good day
>>
>> Prahalad N.
>>
>>
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Thursday, December 28, 2017 3:43 PM
>> To: Prahalad Kumar Narayanan; 2d-dev
>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Hi Prahalad,
>>
>> Thanks for your valuable inputs.
>>
>> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>>
>> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>>
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Prahalad Kumar Narayanan
>> Sent: Tuesday, December 19, 2017 3:08 PM
>> To: Jayathirth D V; 2d-dev
>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>>
>> Hello Jay
>>
>> Good day to you.
>>
>> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
>> Here is a simple math that considers the worst case scenario with max values:
>>
>> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>>         max_width : (2 ^ 31) - 1 = 2147483647
>>         max_input_bands = 4
>>         max_bit_depth = 16 (2 Bytes)
>>
>> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>>         // Fits into "long" data type
>>         max_bits_per_row = max_width * max_input_bands * max_bit_depth
>>                                                = 2147483647 x 4 x 16
>>                                                = 137438953408
>>
>>         // Does not fit into "int" data type
>>         max_bytes_per_row = max_bits_per_row + 7 / 8
>>                                                    = 17179869176
>>                                                    = (0x 3FFFFFFF8 -
>> Goes beyond 4B boundary)
>>
>>         // Upon division by 2 for 16-bit images
>>         max_elts_per_row = max_bytes_per_row / 2
>>                                                = 8589934588
>>                                                = (0x 1FFFFFFFC - Goes
>> beyond 4B boundary)
>>
>> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>>     Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>>
>> Thank you
>> Have a good day
>>
>> Prahalad N.
>>
>> ----- Original Message -----
>> From: Jayathirth D V
>> Sent: Thursday, December 14, 2017 1:48 PM
>> To: 2d-dev
>> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Hello All,
>>
>> Please review the following fix in JDK :
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
>> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>>
>> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>>
>> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
>> stride times width must be less than or equal to the scanline stride
>>                   at
>> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelIn
>> terleavedSampleModel.java:101)
>>                   at
>> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.java
>> :642)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(P
>> NGImageReader.java:974)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PNG
>> ImageReader.java:1099)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(PN
>> GImageReader.java:1295)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNGI
>> mageReader.java:1420)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImageR
>> eader.java:1699)
>>                   at
>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>>                   at
>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>>                   at
>> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
>> 50)
>>
>> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>>
>> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>>
>> Thanks,
>> Jay
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Jayathirth D v
Hello Sergey,

Thanks for reviewing the changes.

The maximum value of inputBands & bitDepth can be 4 & 16. Also in readHeader() we verify the values present in inputBands & bitDepth and if they are not in required bounds we throw IIOException. So (inputBands * bitDepth) will not cause overflow.

Also I noticed that we need to use  Math.multiplyExact() in skipPass() function too where we have same calculation.
So I have updated the code to reflect the same.

Please review the updated webrev:
http://cr.openjdk.java.net/~jdv/8191174/webrev.03/ 

Thanks,
Jay

-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 13, 2018 9:00 AM
To: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Hi, Jay.
Can you please confirm that it is not possible to get overflow in:
"inputBands * bitDepth"?
1051 int bitsPerRow = Math.multiplyExact((inputBands * bitDepth), passWidth);

On 08/01/2018 02:04, Jayathirth D V wrote:

> As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
> Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .
>
> By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.02/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Saturday, January 06, 2018 2:45 PM
> To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.
>
> On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:
>> Hello Jay
>>
>> Good day to you.
>>
>> The code changes wrap the IllegalArgumentException in IIOException.
>> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>>
>> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>>
>> javax.imageio.IIOException: Error reading PNG image data
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>>                   at java.desktop/javax.imageio.ImageIO.read
>>                   ...
>> Caused by: javax.imageio.IIOException: Caught exception during read:
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>>                   at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>>                   ...
>> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>>                   at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>>                   at java.desktop/java.awt.image.Raster.createInterleavedRaster
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>>
>> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
>> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
>> So I think, this should be ok.
>>
>> I would like to know of others' opinion too.
>>
>> Thank you
>> Have a good day
>>
>> Prahalad N.
>>
>>
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Thursday, December 28, 2017 3:43 PM
>> To: Prahalad Kumar Narayanan; 2d-dev
>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Hi Prahalad,
>>
>> Thanks for your valuable inputs.
>>
>> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>>
>> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>>
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Prahalad Kumar Narayanan
>> Sent: Tuesday, December 19, 2017 3:08 PM
>> To: Jayathirth D V; 2d-dev
>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>>
>> Hello Jay
>>
>> Good day to you.
>>
>> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
>> Here is a simple math that considers the worst case scenario with max values:
>>
>> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>>         max_width : (2 ^ 31) - 1 = 2147483647
>>         max_input_bands = 4
>>         max_bit_depth = 16 (2 Bytes)
>>
>> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>>         // Fits into "long" data type
>>         max_bits_per_row = max_width * max_input_bands * max_bit_depth
>>                                                = 2147483647 x 4 x 16
>>                                                = 137438953408
>>
>>         // Does not fit into "int" data type
>>         max_bytes_per_row = max_bits_per_row + 7 / 8
>>                                                    = 17179869176
>>                                                    = (0x 3FFFFFFF8 -
>> Goes beyond 4B boundary)
>>
>>         // Upon division by 2 for 16-bit images
>>         max_elts_per_row = max_bytes_per_row / 2
>>                                                = 8589934588
>>                                                = (0x 1FFFFFFFC - Goes
>> beyond 4B boundary)
>>
>> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>>     Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>>
>> Thank you
>> Have a good day
>>
>> Prahalad N.
>>
>> ----- Original Message -----
>> From: Jayathirth D V
>> Sent: Thursday, December 14, 2017 1:48 PM
>> To: 2d-dev
>> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Hello All,
>>
>> Please review the following fix in JDK :
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
>> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>>
>> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>>
>> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
>> stride times width must be less than or equal to the scanline stride
>>                   at
>> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelI
>> n
>> terleavedSampleModel.java:101)
>>                   at
>> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.jav
>> a
>> :642)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(
>> P
>> NGImageReader.java:974)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PN
>> G
>> ImageReader.java:1099)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(P
>> N
>> GImageReader.java:1295)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNG
>> I
>> mageReader.java:1420)
>>                   at
>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImage
>> R
>> eader.java:1699)
>>                   at
>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>>                   at
>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>>                   at
>> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
>> 50)
>>
>> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>>
>> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>>
>> Thanks,
>> Jay
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Sergey Bylokhov
Looks fine.
Thank you for clarification.

On 14/01/2018 23:08, Jayathirth D V wrote:

> Hello Sergey,
>
> Thanks for reviewing the changes.
>
> The maximum value of inputBands & bitDepth can be 4 & 16. Also in readHeader() we verify the values present in inputBands & bitDepth and if they are not in required bounds we throw IIOException. So (inputBands * bitDepth) will not cause overflow.
>
> Also I noticed that we need to use  Math.multiplyExact() in skipPass() function too where we have same calculation.
> So I have updated the code to reflect the same.
>
> Please review the updated webrev:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.03/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Saturday, January 13, 2018 9:00 AM
> To: Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper
>
> Hi, Jay.
> Can you please confirm that it is not possible to get overflow in:
> "inputBands * bitDepth"?
> 1051 int bitsPerRow = Math.multiplyExact((inputBands * bitDepth), passWidth);
>
> On 08/01/2018 02:04, Jayathirth D V wrote:
>> As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
>> Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .
>>
>> By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/8191174/webrev.02/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Saturday, January 06, 2018 2:45 PM
>> To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.
>>
>> On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:
>>> Hello Jay
>>>
>>> Good day to you.
>>>
>>> The code changes wrap the IllegalArgumentException in IIOException.
>>> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>>>
>>> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>>>
>>> javax.imageio.IIOException: Error reading PNG image data
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>>>                    at java.desktop/javax.imageio.ImageIO.read
>>>                    ...
>>> Caused by: javax.imageio.IIOException: Caught exception during read:
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>>>                    ...
>>> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>>>                    at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>>>                    at java.desktop/java.awt.image.Raster.createInterleavedRaster
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>>>
>>> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
>>> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
>>> So I think, this should be ok.
>>>
>>> I would like to know of others' opinion too.
>>>
>>> Thank you
>>> Have a good day
>>>
>>> Prahalad N.
>>>
>>>
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Thursday, December 28, 2017 3:43 PM
>>> To: Prahalad Kumar Narayanan; 2d-dev
>>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>> Hi Prahalad,
>>>
>>> Thanks for your valuable inputs.
>>>
>>> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>>>
>>> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>>>
>>> Please find updated webrev for review:
>>> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Prahalad Kumar Narayanan
>>> Sent: Tuesday, December 19, 2017 3:08 PM
>>> To: Jayathirth D V; 2d-dev
>>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>>
>>> Hello Jay
>>>
>>> Good day to you.
>>>
>>> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
>>> Here is a simple math that considers the worst case scenario with max values:
>>>
>>> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>>>          max_width : (2 ^ 31) - 1 = 2147483647
>>>          max_input_bands = 4
>>>          max_bit_depth = 16 (2 Bytes)
>>>
>>> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>>>          // Fits into "long" data type
>>>          max_bits_per_row = max_width * max_input_bands * max_bit_depth
>>>                                                 = 2147483647 x 4 x 16
>>>                                                 = 137438953408
>>>
>>>          // Does not fit into "int" data type
>>>          max_bytes_per_row = max_bits_per_row + 7 / 8
>>>                                                     = 17179869176
>>>                                                     = (0x 3FFFFFFF8 -
>>> Goes beyond 4B boundary)
>>>
>>>          // Upon division by 2 for 16-bit images
>>>          max_elts_per_row = max_bytes_per_row / 2
>>>                                                 = 8589934588
>>>                                                 = (0x 1FFFFFFFC - Goes
>>> beyond 4B boundary)
>>>
>>> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>>>      Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>>>
>>> Thank you
>>> Have a good day
>>>
>>> Prahalad N.
>>>
>>> ----- Original Message -----
>>> From: Jayathirth D V
>>> Sent: Thursday, December 14, 2017 1:48 PM
>>> To: 2d-dev
>>> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>> Hello All,
>>>
>>> Please review the following fix in JDK :
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
>>> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>>>
>>> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>>>
>>> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
>>> stride times width must be less than or equal to the scanline stride
>>>                    at
>>> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(PixelI
>>> n
>>> terleavedSampleModel.java:101)
>>>                    at
>>> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.jav
>>> a
>>> :642)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster(
>>> P
>>> NGImageReader.java:974)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(PN
>>> G
>>> ImageReader.java:1099)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(P
>>> N
>>> GImageReader.java:1295)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PNG
>>> I
>>> mageReader.java:1420)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImage
>>> R
>>> eader.java:1699)
>>>                    at
>>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>>>                    at
>>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>>>                    at
>>> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
>>> 50)
>>>
>>> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>>>
>>> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>>>
>>> Thanks,
>>> Jay
>>>
>>
>>
>> --
>> Best regards, Sergey.
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Prahalad kumar Narayanan
Hello Jay

This is a minor change on top of webrev.02.
Looks good.

Thanks
Have a good day

Prahalad N.

-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, January 16, 2018 10:42 PM
To: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStride calculation logic is not proper

Looks fine.
Thank you for clarification.

On 14/01/2018 23:08, Jayathirth D V wrote:

> Hello Sergey,
>
> Thanks for reviewing the changes.
>
> The maximum value of inputBands & bitDepth can be 4 & 16. Also in readHeader() we verify the values present in inputBands & bitDepth and if they are not in required bounds we throw IIOException. So (inputBands * bitDepth) will not cause overflow.
>
> Also I noticed that we need to use  Math.multiplyExact() in skipPass() function too where we have same calculation.
> So I have updated the code to reflect the same.
>
> Please review the updated webrev:
> http://cr.openjdk.java.net/~jdv/8191174/webrev.03/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Saturday, January 13, 2018 9:00 AM
> To: Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
> IllegalArgumentException because ScanlineStride calculation logic is
> not proper
>
> Hi, Jay.
> Can you please confirm that it is not possible to get overflow in:
> "inputBands * bitDepth"?
> 1051 int bitsPerRow = Math.multiplyExact((inputBands * bitDepth),
> passWidth);
>
> On 08/01/2018 02:04, Jayathirth D V wrote:
>> As you have mentioned throwing "Pixel stride times width must be less than or equal to the scanline stride" is not proper in this scenario as image contains proper width as per PNG specification.
>> Thanks for pointing to Math.multiplyExact() function and it is very beneficial for solving this issue. While calculating bytesPerRow itself we can use Math.multiplyExact() and throw "int overflow" ArithmeticException and it will be wrapped into IIOException because of changes present in https://bugs.openjdk.java.net/browse/JDK-8190332 .
>>
>> By doing this we will not have multiple "Caused by" chain and we will be throwing proper exception as per ImageIO specification.
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/8191174/webrev.02/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Saturday, January 06, 2018 2:45 PM
>> To: Prahalad Kumar Narayanan; Jayathirth D V; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>> IllegalArgumentException because ScanlineStride calculation logic is
>> not proper
>>
>> Probably we can use Math.multiplyExact()? The current exception text wrapped a few times "Pixel stride times width must be less than or equal to the scanline stride" is not strictly correct because it is possible that the image itself has correct data, but we cannot handle it because of overflow.
>>
>> On 28/12/2017 22:49, Prahalad Kumar Narayanan wrote:
>>> Hello Jay
>>>
>>> Good day to you.
>>>
>>> The code changes wrap the IllegalArgumentException in IIOException.
>>> This approach is better & aligns with how OutOfMemoryError was wrapped to fix JDK-8190332.
>>>
>>> The only point that I 'm not sure is- whether we could wrap IllegalArgumentException twice before throwing to the user code.
>>>
>>> javax.imageio.IIOException: Error reading PNG image data
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read
>>>                    at java.desktop/javax.imageio.ImageIO.read
>>>                    ...
>>> Caused by: javax.imageio.IIOException: Caught exception during read:
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass
>>>                    at java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage
>>>                    ...
>>> Caused by: java.lang.IllegalArgumentException: Pixel stride times width must be less than or equal to the scanline stride
>>>                    at java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>
>>>                    at java.desktop/java.awt.image.Raster.createInterleavedRaster
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>>>
>>> Since there are multiple levels of same exception, programmer could have trouble getting the cause using getCause() method.
>>> However, Invoking printStackTrace() on the outer most exception object gives complete call stack (as listed above).
>>> So I think, this should be ok.
>>>
>>> I would like to know of others' opinion too.
>>>
>>> Thank you
>>> Have a good day
>>>
>>> Prahalad N.
>>>
>>>
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Thursday, December 28, 2017 3:43 PM
>>> To: Prahalad Kumar Narayanan; 2d-dev
>>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>> Hi Prahalad,
>>>
>>> Thanks for your valuable inputs.
>>>
>>> Even though the fix resolves the issue for the particular test case we have it will not solve the buffer overflow problem as you have mentioned in highest limit case or many other cases where the computed value is very high.
>>>
>>> Also we cannot change datatype of bytesPerRow/eltsPerRow as they are used in many passed to many other Raster and Stream API's. The best approach to resolve this issue would be to wrap the Exception that we are getting while creating Raster/SampleModel into an IIOException as we have done in https://bugs.openjdk.java.net/browse/JDK-8190332 . By doing this we will abide by specification and also we will have complete stack of why we got the exception so that it can be debugged properly in future.
>>>
>>> Please find updated webrev for review:
>>> http://cr.openjdk.java.net/~jdv/8191174/webrev.01/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Prahalad Kumar Narayanan
>>> Sent: Tuesday, December 19, 2017 3:08 PM
>>> To: Jayathirth D V; 2d-dev
>>> Subject: RE: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>>
>>> Hello Jay
>>>
>>> Good day to you.
>>>
>>> I don't think it's possible to fix this issue despite having intermediate " long " variable to hold intermediate bits per pixel.
>>> Here is a simple math that considers the worst case scenario with max values:
>>>
>>> . As per the PNG specification, the maximum permissible width, number of bands, bit-depth are as follows-
>>>          max_width : (2 ^ 31) - 1 = 2147483647
>>>          max_input_bands = 4
>>>          max_bit_depth = 16 (2 Bytes)
>>>
>>> . As per the logic proposed in the fix, the intermediate bits per row would fit into a "long" variable but bytes per pixel would not fit into "int" variable.
>>>          // Fits into "long" data type
>>>          max_bits_per_row = max_width * max_input_bands * max_bit_depth
>>>                                                 = 2147483647 x 4 x 16
>>>                                                 = 137438953408
>>>
>>>          // Does not fit into "int" data type
>>>          max_bytes_per_row = max_bits_per_row + 7 / 8
>>>                                                     = 17179869176
>>>                                                     = (0x 3FFFFFFF8
>>> - Goes beyond 4B boundary)
>>>
>>>          // Upon division by 2 for 16-bit images
>>>          max_elts_per_row = max_bytes_per_row / 2
>>>                                                 = 8589934588
>>>                                                 = (0x 1FFFFFFFC -
>>> Goes beyond 4B boundary)
>>>
>>> . If we intend to store bytes per row (and eltsPerRow which is scanline stride) in a "long" variable, the same cannot be sent to Raster APIs because the APIs accept "int" type for scanline stride in arguments list.
>>>      Going by the Raster APIs used in PNGImageReader, a proper fix would require changes to its APIs as well to handle such large scanline stride values.
>>>
>>> Thank you
>>> Have a good day
>>>
>>> Prahalad N.
>>>
>>> ----- Original Message -----
>>> From: Jayathirth D V
>>> Sent: Thursday, December 14, 2017 1:48 PM
>>> To: 2d-dev
>>> Subject: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws
>>> IllegalArgumentException because ScanlineStride calculation logic is
>>> not proper
>>>
>>> Hello All,
>>>
>>> Please review the following fix in JDK :
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8191174
>>> Webrev : http://cr.openjdk.java.net/~jdv/8191174/webrev.00/
>>>
>>> Issue : When we try to read PNG image with large width we throw undocumented IllegalArgumentException with message "Pixel stride times width must be less than or equal to the scanline stride".
>>>
>>> Exception in thread "main" java.lang.IllegalArgumentException: Pixel
>>> stride times width must be less than or equal to the scanline stride
>>>                    at
>>> java.desktop/java.awt.image.PixelInterleavedSampleModel.<init>(Pixel
>>> I
>>> n
>>> terleavedSampleModel.java:101)
>>>                    at
>>> java.desktop/java.awt.image.Raster.createInterleavedRaster(Raster.ja
>>> v
>>> a
>>> :642)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.createRaster
>>> (
>>> P
>>> NGImageReader.java:974)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodePass(P
>>> N
>>> G
>>> ImageReader.java:1099)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.decodeImage(
>>> P
>>> N
>>> GImageReader.java:1295)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.readImage(PN
>>> G
>>> I
>>> mageReader.java:1420)
>>>                    at
>>> java.desktop/com.sun.imageio.plugins.png.PNGImageReader.read(PNGImag
>>> e
>>> R
>>> eader.java:1699)
>>>                    at
>>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1468)
>>>                    at
>>> java.desktop/javax.imageio.ImageIO.read(ImageIO.java:1363)
>>>                    at
>>> PngReaderLargeWidthStrideTest.main(PngReaderLargeWidthStrideTest.java:
>>> 50)
>>>
>>> Root cause : We use large width present in IHDR and calculate elements per row which is passed as scanlinestride for creating the required raster and its corresponding sample model.  When the call reaches creation of PixelInterleavedSampleModel it checks the condition of (pixelStride * w) > scanlineStride. Since in our case we pass this condition it throws IllegalArgumentException. We have invalid scanlineStride value because when we calculate elements per row/bytes per row value in PNGImageReader the integer variable buffer is overflowed and we maintain invalid value for bytesPerRow.
>>>
>>> Solution : We can maintain the intermediate bitsPerRow value in long datatype and calculate bytesPerRow using the long value and then typecast it to int bytesPerRow variable. By doing this we will maintain the required scanlineStride/eltsPerRow value properly. After this solution we will throw proper IIOException because of changes present in JDK-8190332 and not the undocumented IllegalArgumentException.
>>>
>>> Thanks,
>>> Jay
>>>
>>
>>
>> --
>> Best regards, Sergey.
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.