[cairo] [cairo-commit] src/cairo-fixed-private.h src/cairo-path-stroke-boxes.c src/cairo-path-stroke.c src/cairo-path-stroke-polygon.c src/cairo-path-stroke-traps.c

[Uli Schlachter]

On 05.05.2017 02:47, Bryce Harrington wrote:
>  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(-)
> 
> New commits:
> commit 91b25005d62fe4ca178f45d349374e42c29a5e11
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Mar 24 12:36:41 2017 +0000
> 
>     stroker: Check for scaling overflow in computing half line widths
>     
>     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 at chris-wilson.co.uk>
>     Cc: magomez at igalia.com
>     Tested-by: Bryce Harrington <bryce at osg.samsung.com>
>     Acked-by: Bryce Harrington <bryce at osg.samsung.com>

Hi Chris and Bryce,

[0] might not win the price for the best bug report, but he is right.
Running the test suite with CAIRO_TEST_TARGET=image on current master
results in the following, which did not happen before:

image (argb32): 9 crashed! - dash-offset dash-zero-length
degenerate-dash degenerate-path degenerate-solid-dash leaky-dash
leaky-dashed-rectangle line-width-large-overlap-dashed
line-width-overlap-dashed

When looking at dash-offset GDB says:

> (gdb) frame 4
> #4  0x00007ffff7ad5ccd in _cairo_rectilinear_stroker_emit_segments_dashed (stroker=stroker at entry=0x7fffffffc620)
>     at cairo-path-stroke-boxes.c:419
> 419		assert (box.p1.x < box.p2.x);
> (gdb) print box
> $1 = {p1 = {x = 9728, y = 1280}, p2 = {x = 9728, y = 1792}}

So I guess the assert should be "<=" instead of "<". Looking into
_cairo_boxes_add() (the function which is called in the above code with
the box) confirms this: It is a no-op for empty boxes and explicitly
handles this case.

Could you investigate and handle this? And please, not just this
instance, but also double-check all the other asserts that were added?

Cheers,
Uli

P.S.: Do we have a unit test for the overflow crash bug that the above
commit wants to fix?

[0]: https://bugs.freedesktop.org/show_bug.cgi?id=100952
-- 
"Do you know that books smell like nutmeg or some spice from a foreign
land?"
                                             -- Faber in Fahrenheit 451