[cairo] Segfault at sweep_line_delete on video playback
Bryce Harrington
bryce at osg.samsung.com
Fri Jul 25 16:01:18 PDT 2014
On Fri, Mar 21, 2014 at 05:54:10AM -0400, James Cloos wrote:
> This diff avoids the segfault for me, but only papers over the bug...
>
> diff --git a/src/cairo-bentley-ottmann-rectangular.c b/src/cairo-bentley-ottmann-rectangular.c
> index 5541bdc..c3607cf 100644
> --- a/src/cairo-bentley-ottmann-rectangular.c
> +++ b/src/cairo-bentley-ottmann-rectangular.c
> @@ -564,7 +564,9 @@ sweep_line_delete (sweep_line_t *sweep, rectangle_t *rectangle)
> cairo_bool_t update;
>
> update = TRUE;
> +
> if (sweep->fill_rule == CAIRO_FILL_RULE_WINDING &&
> + rectangle->left.prev &&
> rectangle->left.prev->dir == rectangle->left.dir)
> {
> update = rectangle->left.next != &rectangle->right;
What I'm unsure about here, is that if rectangle->left.prev is not
defined, it causes an update to happen, and I'm wondering if it should
*not* cause an update. So like, should we test rectangle->left.prev
earlier, and bail out (return FALSE) early?
> diff --git a/src/cairo-clip.c b/src/cairo-clip.c
> index 0df9b06..736838f 100644
> --- a/src/cairo-clip.c
> +++ b/src/cairo-clip.c
> @@ -88,7 +88,11 @@ _cairo_clip_path_reference (cairo_clip_path_t *clip_path)
> void
> _cairo_clip_path_destroy (cairo_clip_path_t *clip_path)
> {
> + /*
> assert (CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&clip_path->ref_count));
> + */
> + if (!clip_path)
> + return;
I hear you about asserts being inappropriate for library code, but in
this case it looks like it's not the assert's fault specifically, but
rather the dereferencing of the clip_path input argument.
Looking through the cairo-clip.c, it looks like the intention is that
callers check for NULL clip_path before calling the routine. However,
in some of the compositors it's called without such a check.
Is such a check needed? Well, I spot two functions that deliberately
set it to NULL: _cairo_clip_create() and _cairo_clip_translate(). So,
NULL seems to be a valid value, thus a check is probably appropriate.
See below for my take on the patch. I'm making the compositors check
before calling, and forcing the precondition check with an assert() to
be certain. This is still papering over things rather than solving the
root cause, and don't feel this is quite ready to go in, but it's moving
the check a bit higher up the stack. I'd like to hear confirmation as
to whether it also stops the crashes.
I've done some additional research and found several more bug reports
that all seem to be the same problem (or related):
1. "Crash in src/cairo-bentley-ottmann-rectangular.c:sweep_line_delete()"
Webkit: https://bugs.webkit.org/show_bug.cgi?id=124842
Cairo: https://bugs.freedesktop.org/show_bug.cgi?id=72244
2. "Segfault at sweep_line_delete on video playback"
Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=739262#50
Cairo: https://bugs.freedesktop.org/show_bug.cgi?id=76272
This one proposes a patch with a check for rectangle-left.prev like
our current WIP, but also has checks to avoid copying NULL pointers
in sweep_line_delete_edge():
- edge->prev->next = edge->next;
- edge->next->prev = edge->prev;
+ if (edge->prev)
+ edge->prev->next = edge->next;
+ if (edge->next)
+ edge->next->prev = edge->prev;
I can't tell if that's appropriate to do or not.
3. "Segfault sweep_line_delete on video playback (2)"
Gentoo: https://forums.gentoo.org/viewtopic-t-995484-start-0-postdays-0-postorder-asc-highlight-.html?sid=c5c15b4431456821dcc93bbbc74b9ace
Cairo: https://bugs.freedesktop.org/show_bug.cgi?id=81699
There are two other bugs, which aren't duplicates but also involve
crashing with path+clip oddities. Both include useful looking test
cases:
1. Crash in cairo_stroke for certain clipped rectilinear path combos
https://bugs.freedesktop.org/show_bug.cgi?id=78339
2. Crash in active_edges with multiple sub-paths used for clip and stroke
https://bugs.freedesktop.org/show_bug.cgi?id=74779
Bryce
>From 39ff9f057266aab01a031854c081892254c563c1 Mon Sep 17 00:00:00 2001
From: Bryce Harrington <b.harrington at samsung.com>
Date: Fri, 25 Jul 2014 14:37:07 -0700
Subject: [PATCH] Fix segfault at sweep_line_delete on video playback
Avoid destroying clip path if it's already NULL. The clip path is NULL
when initialized, and can also become NULL if the clip is translated.
The callers of _cairo_clip_path_destroy() from in cairo-clip.c already
pre-check, so do similarly for calls coming from the compositors.
Add an assert to _cairo_clip_path_destroy() to define the precondition
for future callers.
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=739262
Signed-off-by: Bryce Harrington <b.harrington at samsung.com>
---
src/cairo-bentley-ottmann-rectangular.c | 1 +
src/cairo-clip.c | 1 +
src/cairo-spans-compositor.c | 6 ++++--
src/cairo-traps-compositor.c | 6 ++++--
src/cairo-xcb-surface-render.c | 6 ++++--
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/cairo-bentley-ottmann-rectangular.c b/src/cairo-bentley-ottmann-rectangular.c
index 5541bdc..82740ed 100644
--- a/src/cairo-bentley-ottmann-rectangular.c
+++ b/src/cairo-bentley-ottmann-rectangular.c
@@ -565,6 +565,7 @@ sweep_line_delete (sweep_line_t *sweep, rectangle_t *rectangle)
update = TRUE;
if (sweep->fill_rule == CAIRO_FILL_RULE_WINDING &&
+ rectangle->left.prev &&
rectangle->left.prev->dir == rectangle->left.dir)
{
update = rectangle->left.next != &rectangle->right;
diff --git a/src/cairo-clip.c b/src/cairo-clip.c
index 0df9b06..c9d196c 100644
--- a/src/cairo-clip.c
+++ b/src/cairo-clip.c
@@ -88,6 +88,7 @@ _cairo_clip_path_reference (cairo_clip_path_t *clip_path)
void
_cairo_clip_path_destroy (cairo_clip_path_t *clip_path)
{
+ assert (clip_path);
assert (CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&clip_path->ref_count));
if (! _cairo_reference_count_dec_and_test (&clip_path->ref_count))
diff --git a/src/cairo-spans-compositor.c b/src/cairo-spans-compositor.c
index efbae25..9e1b0af 100644
--- a/src/cairo-spans-compositor.c
+++ b/src/cairo-spans-compositor.c
@@ -858,8 +858,10 @@ clip_and_composite_boxes (const cairo_spans_compositor_t *compositor,
status = _cairo_clip_get_polygon (clip, &polygon,
&fill_rule, &antialias);
- _cairo_clip_path_destroy (clip->path);
- clip->path = NULL;
+ if (clip->path != NULL) {
+ _cairo_clip_path_destroy (clip->path);
+ clip->path = NULL;
+ }
if (likely (status == CAIRO_INT_STATUS_SUCCESS)) {
cairo_clip_t *saved_clip = extents->clip;
extents->clip = clip;
diff --git a/src/cairo-traps-compositor.c b/src/cairo-traps-compositor.c
index 8d67965..9948fa1 100644
--- a/src/cairo-traps-compositor.c
+++ b/src/cairo-traps-compositor.c
@@ -1761,8 +1761,10 @@ clip_and_composite_boxes (const cairo_traps_compositor_t *compositor,
status = _cairo_clip_get_polygon (clip, &polygon,
&fill_rule, &antialias);
- _cairo_clip_path_destroy (clip->path);
- clip->path = NULL;
+ if (clip->path != NULL) {
+ _cairo_clip_path_destroy (clip->path);
+ clip->path = NULL;
+ }
if (likely (status == CAIRO_INT_STATUS_SUCCESS)) {
cairo_clip_t *saved_clip = extents->clip;
extents->clip = clip;
diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c
index 3f2fc43..a282fa6 100644
--- a/src/cairo-xcb-surface-render.c
+++ b/src/cairo-xcb-surface-render.c
@@ -3126,8 +3126,10 @@ _clip_and_composite_boxes (cairo_xcb_surface_t *dst,
status = _cairo_clip_get_polygon (clip, &polygon,
&fill_rule, &antialias);
- _cairo_clip_path_destroy (clip->path);
- clip->path = NULL;
+ if (clip->path != NULL) {
+ _cairo_clip_path_destroy (clip->path);
+ clip->path = NULL;
+ }
if (likely (status == CAIRO_INT_STATUS_SUCCESS)) {
cairo_clip_t *saved_clip = extents->clip;
extents->clip = clip;
--
1.7.9.5
More information about the cairo
mailing list