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

Andrea Canciani <[email protected]> writes:

> On Sat, Mar 25, 2017 at 8:55 AM, Carlos Garcia Campos <[email protected]>
> wrote:
>
>> 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.
>>
>
> Would it be possible to a test that reproduces the issue?

Yes, Miguel has a code snippet to reproduce the crash, it could
perfectly be a test.

> Ideally the test could be based on the librsvg bug report and avoid
> exposing (new?) security-critical information about how to trigger the
> crash in WebKit.
> Instead of referencing the security bug, we could just mention that the
> patch fixes that test.
>
>
>> > 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
>>

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

iF0EARECAB0WIQTl43UAGnWfWBrFbDGPEE5qUj5kYgUCWNZYlQAKCRCPEE5qUj5k
YmYbAKCF09P+CzC6Suw3AJNzSLbwb9vhcACfbPqVV1Ef2yrhPuit3fRojpRI3cs=
=ZV38
-----END PGP SIGNATURE-----