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.
