stroker: Check for scaling overflow in computing half line widths

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

Details

Message ID 20170324123641.28468-1-chris@chris-wilson.co.uk
State New
Headers show
Series "stroker: Check for scaling overflow in computing half line widths" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Chris Wilson March 24, 2017, 12:36 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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