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

[Philip Race]

  ii. Handling of Delta encoding (0x00 0x02 xOffset yOffset)
                     . The scanline of decoded image should be copied to destination if yOff shifts next pixel's location to other line
                     . Secondly, the scanline index should be limited by boundary value after x and y offsets are added.
                     . Note: I couldn't create a suitable image with Delta encoding in image buffer. Hence this small change could not be tested.

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


Why "othervm" for the test ? I don't see anything the test does that 
requires this
and it just slows down the test harness.

-phil.

On 1/20/17, 1:09 AM, Prahalad Kumar Narayanan wrote:
> Hello Jay
>
> Thank you for your time in review.
>
> I 've incorporated review suggestions and the modified code is available for review under:
> Link: http://cr.openjdk.java.net/~pnarayanan/8167278/webrev.01/
>
> Quick info on the changes from previous revision:
> 1. Line stride calculation: This has been moved into the 'else if' section as suggested..
> 2. Typo error: At Line 1670 has been corrected
> 3. Un-used argument value- 'padding', in decodeRLE4 function()
>
>> Not using this padding parameter will cause any problems while decoding?
>            Line 1547: mentions - If width is not 32bit aligned then while un-compressing each scanline will have padding bytes.
>            The above comment (and thus padding value) is not applicable to ' current ' RLE4 decoding logic because,
>                  . The logic creates an intermediate scanline array exactly of size -width.
>                  . Besides, when the intermediate scanline is flushed to the destination, the logic assumes the destination is also of type- MultiPixelPackedSampleModel.
>
>> If it is not needed we can remove "padding" parameter in decodeRLE4() and its calculation in readRLE4().
>> If it is not under the scope of this bug you can raise a new bug for the same.
>            Yes - The padding is not applicable for current logic, it could be removed.
>            I have not removed the variable because it could help in fixing this bug- JDK-8172696 in future
>                     [JDK-8172696]  ClassCastException is thrown while decoding RLE4 Bitmap with a destination BufferedImage set in ImageReadParams
>
> Kindly review the new changes at your convenience.
>
> Thank you for your time in review
> Have a good day
>
> Prahalad N.
>
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, January 19, 2017 12:36 PM
> To: Prahalad Kumar Narayanan; 2d-dev at openjdk.java.net
> Subject: RE: [OpenJDK 2D-Dev] [9] RFR: JDK-8167278: ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) with RLE4 BMP
>
> Hi Prahalad,
>
> Please find my observations :
>
> 1) Since you have moved calculation of "lineStride" to different function I think we can optimize it more by moving the calculation of lineStride inside the "else if ((lineNo - sourceRegion.y) % scaleY == 0)" because it is not used in "if (noTransform)" case.
>
> 2) Also there is small typo at line 1670 "// REL4 decoding" and please remove trailing spaces in test case before pushing.
>
> Apart from these things changes are working fine.
>
> Also I noted that we are not using "padding" parameter  passed to decodeRLE4() function.
> Not using this padding parameter will cause any problems while decoding?
>
> If it is not needed we can remove "padding" parameter in decodeRLE4() and its calculation in readRLE4().If it is not under the scope of this bug you can raise a new bug for the same.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Friday, January 13, 2017 2:54 PM
> To: 2d-dev at openjdk.java.net
> Subject: [OpenJDK 2D-Dev] [9] RFR: JDK-8167278: ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) with RLE4 BMP
>
> Hello Everyone
>
> Good day to you.
>
> Request your time in reviewing the fix for bug
>      . [JDK-8167278] : ArrayIndexOutOfBoundsException when calling ImageIO.read(InputStream) with RLE4 BMP
>
> Root Cause
>      . The issue seems to stem from a mal-formed RLE4 Encoded Bitmap Image
>      . Specifically - the width as mentioned in header and encoded image data do not match.
>      . Unfortunately, the decoder logic doesn't contain guard checks to prevent out of bounds array access.
>
> Fix Details:
>      . Necessary guard conditions have been put to the RLE4 bitmap decoding logic.
>      . Besides, two new issues were observed in same function block. They have been addressed as well.
>               i. The last scanline of decoded image is missed in destination image (when EoF sequence arrives without EoL)
>                      . This was observed with a sample image created with gimp tool.
>               ii. Handling of Delta encoding (0x00 0x02 xOffset yOffset)
>                      . The scanline of decoded image should be copied to destination if yOff shifts next pixel's location to other line
>                      . Secondly, the scanline index should be limited by boundary value after x and y offsets are added.
>                      . Note: I couldn't create a suitable image with Delta encoding in image buffer. Hence this small change could not be tested.
>
> Other Details:
>      . The fix was run through both Jtreg and JCK test suites. No regressions were seen.
>
> The changes are available for your review under:
>      Link: http://cr.openjdk.java.net/~pnarayanan/8167278/webrev.00/
>
> Kindly review the changes at your convenience and share your feedback.
>
> Thank you for your time in review
> Have a good day
>
> Prahalad N.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170120/5416a106/attachment.html>