[OpenJDK 2D-Dev] [9] Review request for 8038000: java.awt.image.RasterFormatException: Incorrect scanline stride (original) (raw)

anton nashatyrev anton.nashatyrev at oracle.com
Tue Apr 1 15:39:11 UTC 2014

Hello Jim,

On 28.03.2014 3:25, Jim Graham wrote:

Hi Anton,

A lot of those tests seem out of whack in that they test related conditions, but not the exact condition itself. What we really want is for every index of the form: offset + y * scanlineStride + x + {0 -> numcomponents-1} => [0, buf.length-1] to be in the array for all valid x,y. There are a lot of tests there that are sufficient to prove this, but are overly restrictive in that they reject a bunch of cases. The fix you propose only fixes this for a case where h=1, but what about: w = 10 h = 2 numcomponents = 1 scanlineStride = 1000 buffer.length = 1010 The buffer contains the data for all possible pixel fetches, but since it isn't 2000 in length, we reject it.

My fix actually relaxes the condition, and the case above is not rejected: (height > 1 && scanlineStride > data.length) is FALSE here and hence the exception is not thrown

Also, we restrict a bunch of parameters to "MAXINT / some other factor" because we blindly multiply things out and don't want to deal with overflow, but a better "pixel array" test would use division (I think we do this in our native code): buf.length / w <= h_ _except that you need to deal with offset and scanlineStride for h-1_ _lines and then w for the last one, so this gets complicate, but you have:_ _((buf.length - offset - w) / (h-1)) < scanlineStride_ _but then you have to special case h=1 to avoid the divide by 0 so you_ _get:_ _if (w <= 0 || h <= 0 || scanlineStride < 0 || offset < 0) exception;_ _if (offset >= buf.length) exception; int len = buf.length - offset; // known positive if (len < w) exception;_ _if (h > 1) { if (((len - w) / (h - 1)) < scanlineStride) exception;_ _}_ _Note that the test for (len < w) is done in all cases because it_ _prevents the calculation in the "h>1" case from having a negative numerator, which would make the test invalid. These tests are also valid for scan=0 for the rare case where someone wants every row of an image to contain the same data (we may use a case like that in the GIF loading code that needs to replicate incoming data for interlaced GIFs). It doesn't go so far as to allow scan=-1 or similar cases where the user wants to play games with aliasing parts of rows in a backwards cascade.

There are a lot of checks in the *Raster.verify() methods. I'm not 100% confident but they look pretty equivalent to the algorithm you have described (BTW the 0 scanline is also acceptable). Anyways that was a security fix some time ago when some of those validations have been added and I'm not sure we would like to perform some major refactorings here unless any incompatibilities are found.

Thank you! Anton.

...jim On 3/26/14 10:35 AM, anton nashatyrev wrote: Hello, could you please review the following fix:

fix: http://cr.openjdk.java.net/~anashaty/8038000/webrev.00/ <http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/> bug: https://bugs.openjdk.java.net/browse/JDK-8038000 The last row in the Raster shouldn't be necessary of the scanlineStride length and thus the Raster with height 1 might have a buffer smaller than a scanlineStride. Thanks! Anton.

More information about the 2d-dev mailing list