Bug 78339

Summary:	Certain rectilienar path+clip combinations crash the image backend at stroke
Product:	cairo	Reporter:	Zoltan Turanyi <teknos>
Component:	image backend	Assignee:	Chris Wilson <chris>
Status:	RESOLVED MOVED	QA Contact:	cairo-bugs mailing list <cairo-bugs>
Severity:	normal
Priority:	medium	CC:	mkasik
Version:	1.12.14
Hardware:	All
OS:	All
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	68382
Attachments:	The provided test case in a form suitable for cairo-test-suite A quick patch fixing the crash

Description Zoltan Turanyi 2014-05-06 13:21:04 UTC

If we have a complex, non-pixel aligned, rectilinear clip region and a rectilinear path, the image backend crashes at a stroke. (See offending code example below.) I saw this with 1.12.14, have not checked with 1.12.16, but none of the commits seem to address this.

The problem happens in composite_aligned_boxes() (file src\cairo-spans-compositor.c), where the local variable 'need_clip_mask' gets initialized to true via _clip_is_region() returning false. The latter returns false, because some of the clip boxes are not pixel aligned. (These are such boxes that do not intersect with the stroke (or the path of the stroke would not become pixel-aligned and composite_aligned_boxes() would not be called), but are kept because the fall within the stroke. See the third rectangle added to the clip in the below example. If you remove that third rectangle or make it pixel-aligned, the code runs fine.)

The code in composite_aligned_boxes() later assumes that if 'need_clip_mask' is true then the clip has a path (clip->path!=NULL) and calls get_clip_surface(), which asserts that clip->path is nonzero. Here we crash.

My proposal for a fix (without knowing all the complexity) would be to test for clip->path before setting need_clip_mask or calling get_clip_surface().

See below the code that causes the crash for me. I am not sure how to rate the severity of a bug causing a crash on rare situations, so I set it to normal. I use cairo on Windows, compiled with Visual Studio. 

Cairo is a great library folks, thanks a lot for the effort!


cairo_surface_t *surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 100, 100);
cairo_t *cr = cairo_create(surface);
cairo_rectangle(cr, 10, 10, 80, 80);
cairo_new_sub_path(cr);
cairo_move_to(cr, 20, 20);
cairo_line_to(cr, 20, 50);
cairo_line_to(cr, 50, 50);
cairo_line_to(cr, 50, 20);
cairo_line_to(cr, 20, 20);
cairo_new_sub_path(cr);
cairo_move_to(cr, 31.5, 31.5);
cairo_line_to(cr, 32.5, 31.5);
cairo_line_to(cr, 32.5, 32.5);
cairo_line_to(cr, 31.5, 32.5);
cairo_line_to(cr, 31.5, 31.5);
cairo_clip(cr);
cairo_rectangle(cr, 5.5, 30.5, 90, 5);
cairo_set_line_width(cr, 1.);
cairo_stroke(cr);
cairo_destroy(cr);
cairo_surface_destroy (surface);

Comment 1 Uli Schlachter 2014-05-11 19:01:19 UTC

Created attachment 98868 [details]
The provided test case in a form suitable for cairo-test-suite

Thanks for that excellent test case, Zoltan. I hacked it into something suitable for cairo-test-suite and I can confirm that this still dies from the failed assertion on current git/master. And nope, sorry, no quick suggestions for a fix (but I am not sure about ignoring the clip for extents->clip->path == NULL is the right thing unless something somewhere already intersects the drawn boxes with the clip's boxes).

Are you ok with the following header being added to your test case?

/*
 * Copyright © 2014 Zoltan Turanyi
 *
 * Permission is hereby granted, free of charge, to any person
 * obtaining a copy of this software and associated documentation
 * files (the "Software"), to deal in the Software without
 * restriction, including without limitation the rights to use, copy,
 * modify, merge, publish, distribute, sublicense, and/or sell copies
 * of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be
 * included in all copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
 * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
 * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
 * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 *
 * Author: Zoltan Turanyi <teknos@gmail.com>
 */

Comment 2 Zoltan Turanyi 2014-05-12 05:01:35 UTC

Thanks Uli, please go ahead adding the header.

Comment 3 Marek Kasik 2015-03-23 12:26:43 UTC

Created attachment 114548 [details] [review]
A quick patch fixing the crash

Hi,

I've observed the same crash when I was looking at this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1203907.
Attached is a quick patch which fixes the crash for me although I'm not sure whether it is correct. It was inspired by the function "_cairo_clip_get_polygon()" which accepts 'clip->path == NULL' if there are some boxes in the clip.

Regards

Marek

Comment 4 GitLab Migration User 2018-08-25 13:36:05 UTC

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/90.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.