[OpenJDK 2D-Dev] [9] Review request for 8038000: java.awt.image.RasterFormatException: Incorrect scanline stride
Andrew Brygin
andrew.brygin at oracle.com
Wed Apr 16 14:32:16 UTC 2014
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.
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20140416/4f89825b/attachment.html>
More information about the 2d-dev
mailing list