[cairo] [PATCH] stroker: Check for scaling overflow in computing half line widths
Uli Schlachter
psychon at znc.in
Fri Mar 24 15:55:54 UTC 2017
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 at chris-wilson.co.uk>
> Cc: magomez at 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);
>
>
--
<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
More information about the cairo
mailing list