[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8191174: PngReader throws IllegalArgumentException because ScanlineStri
From: Prahalad Kumar Narayanan <prahalad.kumar.narayanan () oracle ! com>
Date: 2018-01-17 5:18:33
Message-ID: 91a41763-4e03-4236-97a7-8a90e1060a50 () default
[Download RAW message or body]
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.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic