<div dir="ltr">On Sat, Mar 25, 2017 at 8:55 AM, Carlos Garcia Campos <<a href="mailto:carlosgc@gnome.org" target="_blank">carlosgc@gnome.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> writes: > On Fri, Mar 24, 2017 at 04:55:54PM +0100, Uli Schlachter wrote: >> On <a href="tel:24.03.2017%2013" value="+12403201713">24.03.2017 13</a>: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: <a href="https://bugs.webkit.org/show_bug.cgi?id=16793" rel="noreferrer" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=16793</a> >> >> 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 <a href="http://bugs.webkit.org/show_bug.cgi?id=167932" rel="noreferrer" target="_blank">http://bugs.webkit.org/show_bug.cgi?id=167932</a> 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 <a href="https://bugs.freedesktop.org/show_bug.cgi?id=90120" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=90120</a> Right, same bt and same cause, negative height passed to pixman. </blockquote><div> <div>Would it be possible to a test that reproduces the issue? </div><div>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. </div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="gmail-HOEnZb"><div class="gmail-h5"> > 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 <<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> >> > Cc: <a href="mailto:magomez@igalia.com">magomez@igalia.com</a> >> > --- >> > 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 > <a href="mailto:cairo@cairographics.org">cairo@cairographics.org</a> > <a href="https://lists.cairographics.org/mailman/listinfo/cairo" rel="noreferrer" target="_blank">https://lists.cairographics.org/mailman/listinfo/cairo</a> </div></div>-- Carlos Garcia Campos PGP key: <a href="http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462" rel="noreferrer" target="_blank">http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462</a> -- cairo mailing list <a href="mailto:cairo@cairographics.org">cairo@cairographics.org</a> <a href="https://lists.cairographics.org/mailman/listinfo/cairo" rel="noreferrer" target="_blank">https://lists.cairographics.org/mailman/listinfo/cairo</a> </blockquote></div> </div></div>