Hi Prahalad, Changes are fine.
Thanks, Jay -----Original Message----- From: Philip Race Sent: Wednesday, January 25, 2017 6:48 AM To: Prahalad Kumar Narayanan Cc: 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8167278: ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) with RLE4 BMP OK .. +1 -phil. On 1/24/17, 12:57 AM, Prahalad Kumar Narayanan wrote: > Hello Phil > > I agree with you in your observation. We cannot include the image in the test > case. > > This morning, I created many RLE4 bitmap images using Gimp. But none of the > created images contained Delta sequence /or corrupt image data to cause > ArrayIndexOutOfBounds Exception. Henceforth, I 've removed the test case from > the proposed fix. I 've also substantiated the reasons in JBS for not > including a test case. > > The new webrev without test-case is available under > Link: http://cr.openjdk.java.net/~pnarayanan/8167278/webrev.03/ > > Kindly review at your convenience and share the feedback. > > Thank you for your time > Have a good day > > Prahalad N. > > > -----Original Message----- > From: Phil Race > Sent: Tuesday, January 24, 2017 2:06 AM > To: Prahalad Kumar Narayanan; Brian Burkhalter > Cc: 2d-dev@openjdk.java.net > Subject: Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8167278: > ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) > with RLE4 BMP > > I just noticed something. The bug report says :- > > > However, I have a public web application that allows users to upload > images, > > and I was surprised to find a failure caused by an unexpected > > ArrayIndexOutOfBoundsException when a user uploaded an apparently-valid > RLE4 BMP file. > > The attached code contains this BMP file, as a byte array. > > This means the submitter of this bug report almost certainly does not own > this image ! > So we cannot include it in the test to be checked in no matter how encoded. > > Thus you will need to create your own RLE4 encoded BMP. > If you can't do that then we won't be able to check in a test. > > -phil. > > On 01/23/2017 06:24 AM, Prahalad Kumar Narayanan wrote: >> Hello Phil& Brian >> >> Thank you for your time in review and feedback. >> >> . Testing with bmptestsuite >> . The test suite came handy to test the changes and confirm our >> approach. >> . The test suite has a collection of RLE4 compressed images that >> are- valid, questionable and corrupt. >> . The collection includes image with Delta sequence as >> well. >> >> . Handling of Delta sequence (0x00 0x02 xOffset yOffset) >> . The decodeRLE4(...) function deploys line-by-line decoding of >> compressed bitmap image. >> . Until decoding of one row (or line) of pixels is >> complete, the values are stored in intermediate scanline buffer : val. >> . Upon completion of decoding one row of pixels (ie., with >> EoL, EoF sequence ), contents of val are copied to destination image's >> buffer. >> >> . Declaration of val buffer is given as >> . Line 1619: byte[] val = new >> byte[width]; >> >> . As we see, the intermediate scanline array ' val ' is of size : >> width ( Not- height x width ) >> . Thus the offset addition to ' l ', in delta sequence will cause >> index out of bounds if accumulated offset goes beyond size of ' val ' buffer. >> . Hence the new expression at Line 1691 that limits the offset >> within the capacity of the buffer- val. >> . Line 1690: l += xoff + yoff*width; >> . Line 1691: l %= width; >> >> . If the yOffset shifts the decoding to another line, we should >> ensure to copy the decoded row to destination bitmap. >> . Failing to do so, will cause the current row to be missed on the >> destination image. >> . This is achieved with the new set of lines. >> . Line 1677: if (yoff != 0) { >> Line 1678: // Copy the >> decoded scanline to destination >> Line 1679: if >> (copyRLE4ScanlineToDst(lineNo, val, bdata)) { >> >> . When tested with bmptestsuite's rle4 images, Delta sequence >> handling required two additional changes (mentioned in 3.a and 3.b) >> >> 3. Changes from previous webrev >> 3.a. Proper update to the variable- lineNo >> . After handling a delta sequence (xOffset yOffsest), the >> variable- lineNo must be updated correspondingly >> . Reason: lineNo is used to locate exact row on >> Destination buffer to store intermediate scanline. >> . Line 1684: lineNo += isBottomUp ? >> -yOffset : yOffset; >> >> 3.b. Clearing of intermediate scanline buffer before starting to >> decode new row/line. >> . Ensures the previous row's decoded pixels do not appear >> on current row >> 3.c. Minor change to the condition in the while loop to ensure >> sufficient data is available before every iteration. >> 3.d. Removal of main/othervm from the jtreg comments in test file. >> >> 4. The build with changes was run through Jreg and JCK tests. No regressions >> were seen. >> . In addition, the build works well for all input RLE4 >> bitmap images from the test suite >> >> The changes are available for review in the link: >> http://cr.openjdk.java.net/~pnarayanan/8167278/webrev.02/ >> >> Kindly review the changes at your convenience& share your feedback. >> >> Thank you once again >> Have a good day >> >> Prahalad N. >> >> --- Original message --- >> From: Brian Burkhalter >> Sent: Saturday, January 21, 2017 6:05 AM >> To: Philip Race >> Cc: 2d-dev@openjdk.java.net; Prahalad Kumar Narayanan >> Subject: Re: [OpenJDK 2D-Dev] [9] RFR: JDK-8167278: >> ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) >> with RLE4 BMP >> >> On Jan 20, 2017, at 4:30 PM, Brian Burkhalter<brian.burkhal...@oracle.com> >> wrote: >> >> >> That is worrying me since I don't follow these lines are part of >> that:- >> >> 1684 // Move to the position (xoff, yoff). Since l-is >> used >> 1685 // to index into the scanline buffer, the >> calculation >> 1686 // must be limited by the size >> 1687 l += xoff + yoff*width; >> 1688 l %= width; >> 1687 was already there but 1688 and the comment are new and 1688 >> looks wrong to me as it would seem to throw away the y it just added in ... >> >> Indeed, if xoff is in the half-closed interval [0,width), then (xoff + >> yoff*width) % width == xoff. >> >> This does not however account for the accumulation into "l" which might >> negate my observation. >> >> Brian