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

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


-- 
<alanc> I think someone had a Xprint version of glxgears at one point,
    but benchmarking how many GL pages you can print per second
    was deemed too silly to merge
-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo