Hello,
could you please review a slightly update fix version (the
regression test upgraded)
fix: http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/
bug: https://bugs.openjdk.java.net/browse/JDK-8038000
Jim, thanks for your in-depth analysis, the validation indeed doesn't
look ideal. However my fix addresses the concrete regression which was
introduced by these validations, so I'm leaving the fix as is.
Thank you!
Anton.
On 01.04.2014 19:39, anton nashatyrev wrote:
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 "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.
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.
