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

[Andrea Canciani]

On Sat, Mar 25, 2017 at 8:55 AM, Carlos Garcia Campos <carlosgc at gnome.org>
wrote:

> Chris Wilson <chris at chris-wilson.co.uk> 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?
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 <chris at chris-wilson.co.uk>
> >> > Cc: magomez at igalia.com
> >> > ---
> >> >  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
> > cairo at cairographics.org
> > 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
> cairo at cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.cairographics.org/archives/cairo/attachments/20170325/6bdc667e/attachment.html>