Submitted by Chris Wilson on March 24, 2017, 12:36 p.m.

Message ID | 20170324123641.28468-1-chris@chris-wilson.co.uk |
---|---|

State | New |

Series | "stroker: Check for scaling overflow in computing half line widths" |

Headers | show |

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) + #define CAIRO_FIXED_ERROR_DOUBLE (1. / (2 * CAIRO_FIXED_ONE_DOUBLE)) #define CAIRO_FIXED_FRAC_MASK ((cairo_fixed_t)(((cairo_fixed_unsigned_t)(-1)) >> (CAIRO_FIXED_BITS - CAIRO_FIXED_FRAC_BITS))) @@ -123,6 +126,16 @@ _cairo_fixed_from_double (double d) #endif } +static inline cairo_bool_t +_cairo_fixed_from_double_safe (cairo_fixed_t *f, double d) +{ + if (unlikely (d < CAIRO_FIXED_MIN || d > CAIRO_FIXED_MAX)) + return FALSE; + + *f = _cairo_fixed_from_double (d); + return TRUE; +} + #else # error Please define a magic number for your fixed point type! # error See cairo-fixed-private.h for details. diff --git a/src/cairo-path-stroke-boxes.c b/src/cairo-path-stroke-boxes.c index 6511d2383..c77b4a2cc 100644 --- a/src/cairo-path-stroke-boxes.c +++ b/src/cairo-path-stroke-boxes.c @@ -98,6 +98,8 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, cairo_antialias_t antialias, cairo_boxes_t *boxes) { + double half_line_width; + /* This special-case rectilinear stroker only supports * miter-joined lines (not curves) and a translation-only matrix * (though it could probably be extended to support a matrix with @@ -127,15 +129,22 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, if (! _cairo_matrix_is_scale (ctm)) return FALSE; + half_line_width = stroke_style->line_width / 2.0; + + if (! _cairo_fixed_from_double_safe (&stroker->half_line_x, + fabs(ctm->xx) * half_line_width)) + return FALSE; + assert (stroker->half_line_x > 0); + + if (! _cairo_fixed_from_double_safe (&stroker->half_line_y, + fabs(ctm->yy) * half_line_width)) + return FALSE; + assert (stroker->half_line_y > 0); + stroker->stroke_style = stroke_style; stroker->ctm = ctm; stroker->antialias = antialias; - stroker->half_line_x = - _cairo_fixed_from_double (fabs(ctm->xx) * stroke_style->line_width / 2.0); - stroker->half_line_y = - _cairo_fixed_from_double (fabs(ctm->yy) * stroke_style->line_width / 2.0); - stroker->open_sub_path = FALSE; stroker->segments = stroker->segments_embedded; stroker->segments_size = ARRAY_LENGTH (stroker->segments_embedded); @@ -287,6 +296,8 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) box.p1.x = b->x; box.p2.x = a->x; } + assert (box.p1.x < box.p2.x); + if (a->y < b->y) { box.p1.y = a->y; box.p2.y = b->y; @@ -294,6 +305,7 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) box.p1.y = b->y; box.p2.y = a->y; } + assert (box.p1.y < box.p2.y); status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); if (unlikely (status)) @@ -404,6 +416,8 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st box.p1.x = b->x; box.p2.x = a->x; } + assert (box.p1.x < box.p2.x); + if (a->y < b->y) { box.p1.y = a->y; box.p2.y = b->y; @@ -411,6 +425,7 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st box.p1.y = b->y; box.p2.y = a->y; } + assert (box.p1.y < box.p2.y); status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); if (unlikely (status)) diff --git a/src/cairo-path-stroke-polygon.c b/src/cairo-path-stroke-polygon.c index f9f70642f..1cbd3f8e7 100644 --- a/src/cairo-path-stroke-polygon.c +++ b/src/cairo-path-stroke-polygon.c @@ -1280,6 +1280,7 @@ _cairo_path_fixed_stroke_to_polygon (const cairo_path_fixed_t *path, for (i = 1; i < polygon->num_limits; i++) _cairo_box_add_box (&stroker.bounds, &polygon->limits[i]); + /* XXX check overflow */ _cairo_stroke_style_max_distance_from_path (style, path, ctm, &dx, &dy); fdx = _cairo_fixed_from_double (dx); fdy = _cairo_fixed_from_double (dy); diff --git a/src/cairo-path-stroke-traps.c b/src/cairo-path-stroke-traps.c index e183d1024..9eb60dda2 100644 --- a/src/cairo-path-stroke-traps.c +++ b/src/cairo-path-stroke-traps.c @@ -150,6 +150,7 @@ stroker_init (struct stroker *stroker, _cairo_stroke_style_max_line_distance_from_path (stroker->style, path, stroker->ctm, &dx, &dy); + /* XXX check overflow */ fdx = _cairo_fixed_from_double (dx); fdy = _cairo_fixed_from_double (dy); diff --git a/src/cairo-path-stroke.c b/src/cairo-path-stroke.c index a4e17fd8b..b8adf8107 100644 --- a/src/cairo-path-stroke.c +++ b/src/cairo-path-stroke.c @@ -110,6 +110,7 @@ _cairo_stroker_limit (cairo_stroker_t *stroker, _cairo_stroke_style_max_distance_from_path (&stroker->style, path, stroker->ctm, &dx, &dy); + /* XXX report overflow! */ fdx = _cairo_fixed_from_double (dx); fdy = _cairo_fixed_from_double (dy);

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. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: magomez@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)) and MIN = _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" > #define CAIRO_FIXED_ERROR_DOUBLE (1. / (2 * CAIRO_FIXED_ONE_DOUBLE)) > > #define CAIRO_FIXED_FRAC_MASK ((cairo_fixed_t)(((cairo_fixed_unsigned_t)(-1)) >> (CAIRO_FIXED_BITS - CAIRO_FIXED_FRAC_BITS))) > @@ -123,6 +126,16 @@ _cairo_fixed_from_double (double d) > #endif > } > > +static inline cairo_bool_t > +_cairo_fixed_from_double_safe (cairo_fixed_t *f, double d) > +{ > + if (unlikely (d < CAIRO_FIXED_MIN || d > CAIRO_FIXED_MAX)) > + return FALSE; > + > + *f = _cairo_fixed_from_double (d); > + return TRUE; > +} > #else > # error Please define a magic number for your fixed point type! > # error See cairo-fixed-private.h for details. > diff --git a/src/cairo-path-stroke-boxes.c b/src/cairo-path-stroke-boxes.c > index 6511d2383..c77b4a2cc 100644 > --- a/src/cairo-path-stroke-boxes.c > +++ b/src/cairo-path-stroke-boxes.c > @@ -98,6 +98,8 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, > cairo_antialias_t antialias, > cairo_boxes_t *boxes) > { > + double half_line_width; > + > /* This special-case rectilinear stroker only supports > * miter-joined lines (not curves) and a translation-only matrix > * (though it could probably be extended to support a matrix with > @@ -127,15 +129,22 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, > if (! _cairo_matrix_is_scale (ctm)) > return FALSE; > > + half_line_width = stroke_style->line_width / 2.0; > + > + if (! _cairo_fixed_from_double_safe (&stroker->half_line_x, > + fabs(ctm->xx) * half_line_width)) > + return FALSE; > + assert (stroker->half_line_x > 0); > + > + if (! _cairo_fixed_from_double_safe (&stroker->half_line_y, > + fabs(ctm->yy) * half_line_width)) > + return FALSE; > + assert (stroker->half_line_y > 0); > + > stroker->stroke_style = stroke_style; > stroker->ctm = ctm; > stroker->antialias = antialias; > > - stroker->half_line_x = > - _cairo_fixed_from_double (fabs(ctm->xx) * stroke_style->line_width / 2.0); > - stroker->half_line_y = > - _cairo_fixed_from_double (fabs(ctm->yy) * stroke_style->line_width / 2.0); > - > stroker->open_sub_path = FALSE; > stroker->segments = stroker->segments_embedded; > stroker->segments_size = ARRAY_LENGTH (stroker->segments_embedded); > @@ -287,6 +296,8 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) > box.p1.x = b->x; > box.p2.x = a->x; > } > + assert (box.p1.x < box.p2.x); > + > if (a->y < b->y) { > box.p1.y = a->y; > box.p2.y = b->y; > @@ -294,6 +305,7 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) > box.p1.y = b->y; > box.p2.y = a->y; > } > + assert (box.p1.y < box.p2.y); > > status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); > if (unlikely (status)) > @@ -404,6 +416,8 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st > box.p1.x = b->x; > box.p2.x = a->x; > } > + assert (box.p1.x < box.p2.x); > + > if (a->y < b->y) { > box.p1.y = a->y; > box.p2.y = b->y; > @@ -411,6 +425,7 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st > box.p1.y = b->y; > box.p2.y = a->y; > } > + assert (box.p1.y < box.p2.y); > > status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); > if (unlikely (status)) > diff --git a/src/cairo-path-stroke-polygon.c b/src/cairo-path-stroke-polygon.c > index f9f70642f..1cbd3f8e7 100644 > --- a/src/cairo-path-stroke-polygon.c > +++ b/src/cairo-path-stroke-polygon.c > @@ -1280,6 +1280,7 @@ _cairo_path_fixed_stroke_to_polygon (const cairo_path_fixed_t *path, > for (i = 1; i < polygon->num_limits; i++) > _cairo_box_add_box (&stroker.bounds, &polygon->limits[i]); > > + /* XXX check overflow */ > _cairo_stroke_style_max_distance_from_path (style, path, ctm, &dx, &dy); > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > diff --git a/src/cairo-path-stroke-traps.c b/src/cairo-path-stroke-traps.c > index e183d1024..9eb60dda2 100644 > --- a/src/cairo-path-stroke-traps.c > +++ b/src/cairo-path-stroke-traps.c > @@ -150,6 +150,7 @@ stroker_init (struct stroker *stroker, > _cairo_stroke_style_max_line_distance_from_path (stroker->style, path, > stroker->ctm, &dx, &dy); > > + /* XXX check overflow */ > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > > diff --git a/src/cairo-path-stroke.c b/src/cairo-path-stroke.c > index a4e17fd8b..b8adf8107 100644 > --- a/src/cairo-path-stroke.c > +++ b/src/cairo-path-stroke.c > @@ -110,6 +110,7 @@ _cairo_stroker_limit (cairo_stroker_t *stroker, > _cairo_stroke_style_max_distance_from_path (&stroker->style, path, > stroker->ctm, &dx, &dy); > > + /* XXX report overflow! */ > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > >

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 <chris@chris-wilson.co.uk> > > Cc: magomez@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

On 24.03.2017 17:24, Chris Wilson wrote: > 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 <chris@chris-wilson.co.uk> >>> Cc: magomez@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. Let's assume the maximum value is 12.34. I think the code from your original patch produces 12 as the maximum value while the one above would produce 12.34 (but expressed as a fixed point number). Anyway, this small difference likely does not make much of a difference. >> _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 > ? Sounds good to me, except that it should be made clear if the value is in fixed point form or just the integer equivalent of the fixed point. Uli

Chris Wilson <chris@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. > 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@chris-wilson.co.uk> >> > Cc: magomez@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@cairographics.org > https://lists.cairographics.org/mailman/listinfo/cairo

On Sat, Mar 25, 2017 at 8:55 AM, Carlos Garcia Campos <carlosgc@gnome.org> wrote: > Chris Wilson <chris@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@chris-wilson.co.uk> > >> > Cc: magomez@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@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@cairographics.org > https://lists.cairographics.org/mailman/listinfo/cairo >

Andrea Canciani <ranma42@gmail.com> writes: > On Sat, Mar 25, 2017 at 8:55 AM, Carlos Garcia Campos <carlosgc@gnome.org> > wrote: > >> Chris Wilson <chris@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? 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 <chris@chris-wilson.co.uk> >> >> > Cc: magomez@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@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@cairographics.org >> https://lists.cairographics.org/mailman/listinfo/cairo >>

Resubmitting this to the list as I wasn't subscribed. > > 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. Thanks for working on this guys!! This is the test I'm using to reproduce the crash: =============================================== #include <cairo.h> int main() { cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 64, 64); cairo_t* cr = cairo_create(surface); cairo_save(cr); cairo_matrix_t matrix; matrix.xx = 1.0; matrix.yx = 0; matrix.xy = 0; matrix.yy = 4294967295.0; matrix.x0 = 8.0; matrix.y0 = -49392123873.00; cairo_transform(cr, &matrix); cairo_rectangle(cr, 0.5, 0.5, 29.0, 22.0); cairo_set_line_width(cr, 1.0); cairo_stroke(cr); cairo_restore(cr); cairo_destroy(cr); cairo_surface_destroy(surface); return 0; } ===============================================

Hi! > 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. Any news about this? Is there anything we can do to help? This is causing a pretty serious security bug in Webkit that we would like to fix. Thanks for your help!! :)

On Fri, Mar 24, 2017 at 12:36:41PM +0000, 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 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: magomez@igalia.com The discussion thread seems to have died out on this. I went ahead and verified the testcase Miguel Gomez provided does indeed crash without this patch and doesn't crash with it applied. So in the interest of seeing the fix move ahead I've pushed this patch to trunk: Acked-by: Bryce Harrington <bryce@osg.samsung.com> Tested-by: Bryce Harrington <bryce@osg.samsung.com> To ssh://git.cairographics.org/git/cairo a3cc46d..91b2500 master -> master If there are further refinements in mind, that can be tackled as follow-on work. Bryce > --- > 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) > + > #define CAIRO_FIXED_ERROR_DOUBLE (1. / (2 * CAIRO_FIXED_ONE_DOUBLE)) > > #define CAIRO_FIXED_FRAC_MASK ((cairo_fixed_t)(((cairo_fixed_unsigned_t)(-1)) >> (CAIRO_FIXED_BITS - CAIRO_FIXED_FRAC_BITS))) > @@ -123,6 +126,16 @@ _cairo_fixed_from_double (double d) > #endif > } > > +static inline cairo_bool_t > +_cairo_fixed_from_double_safe (cairo_fixed_t *f, double d) > +{ > + if (unlikely (d < CAIRO_FIXED_MIN || d > CAIRO_FIXED_MAX)) > + return FALSE; > + > + *f = _cairo_fixed_from_double (d); > + return TRUE; > +} > + > #else > # error Please define a magic number for your fixed point type! > # error See cairo-fixed-private.h for details. > diff --git a/src/cairo-path-stroke-boxes.c b/src/cairo-path-stroke-boxes.c > index 6511d2383..c77b4a2cc 100644 > --- a/src/cairo-path-stroke-boxes.c > +++ b/src/cairo-path-stroke-boxes.c > @@ -98,6 +98,8 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, > cairo_antialias_t antialias, > cairo_boxes_t *boxes) > { > + double half_line_width; > + > /* This special-case rectilinear stroker only supports > * miter-joined lines (not curves) and a translation-only matrix > * (though it could probably be extended to support a matrix with > @@ -127,15 +129,22 @@ _cairo_rectilinear_stroker_init (cairo_rectilinear_stroker_t *stroker, > if (! _cairo_matrix_is_scale (ctm)) > return FALSE; > > + half_line_width = stroke_style->line_width / 2.0; > + > + if (! _cairo_fixed_from_double_safe (&stroker->half_line_x, > + fabs(ctm->xx) * half_line_width)) > + return FALSE; > + assert (stroker->half_line_x > 0); > + > + if (! _cairo_fixed_from_double_safe (&stroker->half_line_y, > + fabs(ctm->yy) * half_line_width)) > + return FALSE; > + assert (stroker->half_line_y > 0); > + > stroker->stroke_style = stroke_style; > stroker->ctm = ctm; > stroker->antialias = antialias; > > - stroker->half_line_x = > - _cairo_fixed_from_double (fabs(ctm->xx) * stroke_style->line_width / 2.0); > - stroker->half_line_y = > - _cairo_fixed_from_double (fabs(ctm->yy) * stroke_style->line_width / 2.0); > - > stroker->open_sub_path = FALSE; > stroker->segments = stroker->segments_embedded; > stroker->segments_size = ARRAY_LENGTH (stroker->segments_embedded); > @@ -287,6 +296,8 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) > box.p1.x = b->x; > box.p2.x = a->x; > } > + assert (box.p1.x < box.p2.x); > + > if (a->y < b->y) { > box.p1.y = a->y; > box.p2.y = b->y; > @@ -294,6 +305,7 @@ _cairo_rectilinear_stroker_emit_segments (cairo_rectilinear_stroker_t *stroker) > box.p1.y = b->y; > box.p2.y = a->y; > } > + assert (box.p1.y < box.p2.y); > > status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); > if (unlikely (status)) > @@ -404,6 +416,8 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st > box.p1.x = b->x; > box.p2.x = a->x; > } > + assert (box.p1.x < box.p2.x); > + > if (a->y < b->y) { > box.p1.y = a->y; > box.p2.y = b->y; > @@ -411,6 +425,7 @@ _cairo_rectilinear_stroker_emit_segments_dashed (cairo_rectilinear_stroker_t *st > box.p1.y = b->y; > box.p2.y = a->y; > } > + assert (box.p1.y < box.p2.y); > > status = _cairo_boxes_add (stroker->boxes, stroker->antialias, &box); > if (unlikely (status)) > diff --git a/src/cairo-path-stroke-polygon.c b/src/cairo-path-stroke-polygon.c > index f9f70642f..1cbd3f8e7 100644 > --- a/src/cairo-path-stroke-polygon.c > +++ b/src/cairo-path-stroke-polygon.c > @@ -1280,6 +1280,7 @@ _cairo_path_fixed_stroke_to_polygon (const cairo_path_fixed_t *path, > for (i = 1; i < polygon->num_limits; i++) > _cairo_box_add_box (&stroker.bounds, &polygon->limits[i]); > > + /* XXX check overflow */ > _cairo_stroke_style_max_distance_from_path (style, path, ctm, &dx, &dy); > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > diff --git a/src/cairo-path-stroke-traps.c b/src/cairo-path-stroke-traps.c > index e183d1024..9eb60dda2 100644 > --- a/src/cairo-path-stroke-traps.c > +++ b/src/cairo-path-stroke-traps.c > @@ -150,6 +150,7 @@ stroker_init (struct stroker *stroker, > _cairo_stroke_style_max_line_distance_from_path (stroker->style, path, > stroker->ctm, &dx, &dy); > > + /* XXX check overflow */ > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > > diff --git a/src/cairo-path-stroke.c b/src/cairo-path-stroke.c > index a4e17fd8b..b8adf8107 100644 > --- a/src/cairo-path-stroke.c > +++ b/src/cairo-path-stroke.c > @@ -110,6 +110,7 @@ _cairo_stroker_limit (cairo_stroker_t *stroker, > _cairo_stroke_style_max_distance_from_path (&stroker->style, path, > stroker->ctm, &dx, &dy); > > + /* XXX report overflow! */ > fdx = _cairo_fixed_from_double (dx); > fdy = _cairo_fixed_from_double (dy); > > -- > 2.11.0 > > -- > cairo mailing list > cairo@cairographics.org > https://lists.cairographics.org/mailman/listinfo/cairo

`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 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: magomez@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(-)`