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

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.

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