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.
