[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review request for 8038000: java.awt.image.RasterFormatException: Incorrect
From: Andrew Brygin <andrew.brygin () oracle ! com>
Date: 2014-04-16 14:32:16
Message-ID: 534E9470.3070901 () oracle ! com
[Download RAW message or body]
Hello Anton,
the fix looks fine to me.
Thanks,
Andrew
On 4/16/2014 6:30 PM, anton nashatyrev wrote:
> Hello Andrew,
>
> you are right, the fix may be less relaxing but still conforming:
> if the height == 1, but the minY - sampleModelTranslateY > 0 the
> data buffer should still contain at least one scanline.
> Below is the updated fix. I've also added explicit check for
> implicit (minY - sampleModelTranslateY >= 0) constraint.
>
> http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.02/
>
> Thank you!
> Anton.
>
> On 11.04.2014 15:06, Andrew Brygin wrote:
>> Hello Anton,
>>
>> we probably should use '(minY + height) > 1' as a condition for the
>> scanstride test, should not we?
>> Otherwise, we are not taking into account the case when minY is
>> greater than 0, and too big scanstride may be potentially dangerous.
>>
>> Thanks,
>> Andrew
>>
>>
>> On 4/10/2014 11:26 PM, anton nashatyrev wrote:
>>> 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.
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hello Anton,<br>
<br>
the fix looks fine to me.<br>
<br>
Thanks,<br>
Andrew<br>
<br>
On 4/16/2014 6:30 PM, anton nashatyrev wrote:<br>
</div>
<blockquote cite="mid:534E93EA.9020509@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hello Andrew, <br>
<br>
you are right, the fix may be less relaxing but still
conforming: <br>
if the height == 1, but the minY - sampleModelTranslateY >
0 the data buffer should still contain at least one scanline. <br>
Below is the updated fix. I've also added explicit check for
implicit (minY - sampleModelTranslateY >= 0) constraint.<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.02/">http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.02/</a><br>
<br>
Thank you!<br>
Anton.<br>
<br>
<div class="moz-cite-prefix">On 11.04.2014 15:06, Andrew Brygin
wrote:<br>
</div>
<blockquote cite="mid:5347CCA4.4090907@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hello Anton,<br>
<br>
we probably should use '(minY + height) > 1' as a
condition for the <br>
scanstride test, should not we?<br>
Otherwise, we are not taking into account the case when minY
is<br>
greater than 0, and too big scanstride may be potentially
dangerous.<br>
<br>
Thanks,<br>
Andrew <br>
<br>
<br>
On 4/10/2014 11:26 PM, anton nashatyrev wrote:<br>
</div>
<blockquote cite="mid:5346F064.9060600@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hello,<br>
<br>
could you please review a slightly update fix version (the
regression test upgraded) <br>
fix: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/">http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/</a><br>
bug: <a moz-do-not-send="true"
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 moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/">http://cr.openjdk.java.net/~anashaty/8038000/webrev.00/</a>
<br>
<a moz-do-not-send="true" 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 moz-do-not-send="true"
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>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic