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

[anton nashatyrev]

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/fee9ae1c/attachment.html>