<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hello,<br>
<br>
could you please review a slightly update fix version (the
regression test upgraded) <br>
fix: <a
href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/">http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/</a><br>
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8038000">https://bugs.openjdk.java.net/browse/JDK-8038000
</a><br>
<br>
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. <br>
<br>
Thank you!<br>
Anton.<br>
<br>
<div class="moz-cite-prefix">On 01.04.2014 19:39, anton nashatyrev
wrote:<br>
</div>
<blockquote cite="mid:533ADD9F.50501@oracle.com" type="cite">Hello
Jim,
<br>
<br>
On 28.03.2014 3:25, Jim Graham wrote:
<br>
<blockquote type="cite">Hi Anton,
<br>
<br>
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:
<br>
<br>
offset + y * scanlineStride + x + {0 -> numcomponents-1}
=> [0, buf.length-1]
<br>
<br>
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:
<br>
<br>
w = 10
<br>
h = 2
<br>
numcomponents = 1
<br>
scanlineStride = 1000
<br>
buffer.length = 1010
<br>
<br>
The buffer contains the data for all possible pixel fetches, but
since it isn't 2000 in length, we reject it.
<br>
</blockquote>
<br>
My fix actually relaxes the condition, and the case above is not
rejected:
<br>
(height > 1 && scanlineStride > data.length) is
FALSE here and hence the exception is not thrown
<br>
<br>
<blockquote type="cite">
<br>
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):
<br>
<br>
buf.length / w <= h
<br>
<br>
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:
<br>
<br>
((buf.length - offset - w) / (h-1)) < scanlineStride
<br>
<br>
but then you have to special case h=1 to avoid the divide by 0
so you get:
<br>
<br>
if (w <= 0 || h <= 0 || scanlineStride < 0 ||
offset < 0) exception;
<br>
if (offset >= buf.length) exception;
<br>
int len = buf.length - offset; // known positive
<br>
if (len < w) exception;
<br>
if (h > 1) {
<br>
if (((len - w) / (h - 1)) < scanlineStride)
exception;
<br>
}
<br>
<br>
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.
<br>
</blockquote>
<br>
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.
<br>
<br>
Thank you!
<br>
Anton.
<br>
<br>
<blockquote type="cite">
<br>
...jim
<br>
<br>
On 3/26/14 10:35 AM, anton nashatyrev wrote:
<br>
<blockquote type="cite">Hello,
<br>
could you please review the following fix:
<br>
<br>
fix: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~anashaty/8038000/webrev.00/">http://cr.openjdk.java.net/~anashaty/8038000/webrev.00/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/"><http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/></a>
<br>
bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8038000">https://bugs.openjdk.java.net/browse/JDK-8038000</a>
<br>
<br>
The last row in the Raster shouldn't be necessary of the
scanlineStride
<br>
length and thus the Raster with height 1 might have a buffer
smaller
<br>
than a scanlineStride.
<br>
<br>
Thanks!
<br>
Anton.
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>