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

Thu May 15 17:57:55 UTC 2014

Hello, Anton.
The fix looks good.
Thanks.

On 16.04.2014 18:32, Andrew Brygin wrote:
> 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.
>>>>>
>>>>
>>>
>>
>

-- 
Best regards, Sergey.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20140515/fc9f150c/attachment.html>