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

[Philip Race]

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