Re: [PATCH] stroker: Check for scaling overflow in computing half line widths

On 24.03.2017 17:24, Chris Wilson wrote:
> On Fri, Mar 24, 2017 at 04:55:54PM +0100, Uli Schlachter wrote:
>> On 24.03.2017 13:36, Chris Wilson wrote:
>>> Given a combination of a large scaling matrix and a large line, we can
>>> easily generate a half line width that is unrepresentable in our 24.8
>>> fixed-point. This leads to spurious errors later, such as generating
>>> negative height boxes, and so asking pixman to fill to infinity. To
>>> avoid this, we can check for overflow in calculating the half line with,
>>> though we still lack adequate range checking on the final stroke path.
>>>
>>> References: https://bugs.webkit.org/show_bug.cgi?id=16793
>>
>> Are you sure that URL is the right one? That one has been closed as
>> invalid 9 years ago.
> 
>  +2 http://bugs.webkit.org/show_bug.cgi?id=167932
> 
> Actually looks like https://bugs.freedesktop.org/show_bug.cgi?id=90120
> 
> For which I wrote a different validation patch, which we should also
> investigate. There is likely merit in combining/using both the
> validation of the stroker ctm and also double checking against the
> overflow in fixed-point conversion.
> 
>>
>>> Signed-off-by: Chris Wilson <[email protected]>
>>> Cc: [email protected]
>>> ---
>>>  src/cairo-fixed-private.h       | 13 +++++++++++++
>>>  src/cairo-path-stroke-boxes.c   | 25 ++++++++++++++++++++-----
>>>  src/cairo-path-stroke-polygon.c |  1 +
>>>  src/cairo-path-stroke-traps.c   |  1 +
>>>  src/cairo-path-stroke.c         |  1 +
>>>  5 files changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/cairo-fixed-private.h b/src/cairo-fixed-private.h
>>> index 9ff8f7503..8ee895b92 100644
>>> --- a/src/cairo-fixed-private.h
>>> +++ b/src/cairo-fixed-private.h
>>> @@ -53,6 +53,9 @@
>>>  #define CAIRO_FIXED_ONE_DOUBLE ((double)(1 << CAIRO_FIXED_FRAC_BITS))
>>>  #define CAIRO_FIXED_EPSILON    ((cairo_fixed_t)(1))
>>>  
>>> +#define CAIRO_FIXED_MAX		(~0u >> (CAIRO_FIXED_FRAC_BITS + 1))
>>> +#define CAIRO_FIXED_MIN		(-(int)CAIRO_FIXED_MAX)
>>> +
>>
>> So... the above is basically (int)_cairo_fixed_to_double(0x7fffffffu),
>> right? "~0u >> 1" is supposed to be INT32_MAX (maximum fixed value) and
>> this value is then converted to an integer via >> CAIRO_FIXED_FRAC_BITS.
>>
>> How about replacing "~0u" with this constant, so that systems with
>> sizeof(unsigned int) != 4 will also work fine?
>>
>> Also, I do not like the name. CAIRO_FIXED_ONE is the number 1 as a fixed
>> number, while CAIRO_FIXED_MAX is the maximum value as an integer. Could
>> you name it CAIRO_FIXED_MAX_INT? (and same for MIN)
>>
>> Oh and: This is not really the maximum value, since you shifted off some
>> bits. Don't you need something like MAX =
>> _cairo_fixed_to_double((cairo_fixed_t)(~0u >> 1))
> 
> That's exactly equivalent to the above, prior to the (double) cast.

Let's assume the maximum value is 12.34. I think the code from your
original patch produces 12 as the maximum value while the one above
would produce 12.34 (but expressed as a fixed point number). Anyway,
this small difference likely does not make much of a difference.

>> _cairo_fixed_to_double((cairo_fixed_t)(~0u)) (the later assuming
>> two-complement's arithmetic and ignoring that signed overflow is
>> undefined behaviour).
>> I guess this difference is not all that important in practice. How about
>> mentioning it in a comment next to the definition? "These are not the
>> real limits, but just the closest whole numbers"
> 
> Yes, we could add one to upper and use >= (and vice version for the
> lower).
> 
> CAIRO_FIXED_INT_UPPER_BOUND
> CAIRO_FIXED_INT_LOWER_BOUND
> ?

Sounds good to me, except that it should be made clear if the value is
in fixed point form or just the integer equivalent of the fixed point.

Uli
-- 
<alanc> I think someone had a Xprint version of glxgears at one point,
    but benchmarking how many GL pages you can print per second
    was deemed too silly to merge
-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo