[OpenJDK 2D-Dev] [9] RFR: JDK-8167278: ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) with RLE4 BMP

[Phil Race]

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 at 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.burkhalter at 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