[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