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

[Prahalad Kumar Narayanan]

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