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

[Jim Graham]

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.

Also, we restrict a bunch of parameters to "MAX_INT / 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.

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