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

Chris Wilson <[email protected]> writes:

> 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.

Hey Chris, thanks for the patch!

>  +2 http://bugs.webkit.org/show_bug.cgi?id=167932

I'm not sure it's useful to include a URL of a security bug in the
commit message, though. Maybe it would be better to just mention that
it's causing crashes in WebKit.

> Actually looks like https://bugs.freedesktop.org/show_bug.cgi?id=90120

Right, same bt and same cause, negative height passed to pixman.

> 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.
>
>> _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
> ?
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> -- 
> cairo mailing list
> [email protected]
> https://lists.cairographics.org/mailman/listinfo/cairo

-- 
Carlos Garcia Campos
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462

-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo

-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQTl43UAGnWfWBrFbDGPEE5qUj5kYgUCWNYiYQAKCRCPEE5qUj5k
YjjWAJ4znA6ex/6V3MZsDDBh4VRQ22ycaQCgjgSzXCd7ZT0ojl1wodoVFzcwnTU=
=GPjR
-----END PGP SIGNATURE-----