libcairo segfaults when rendering video streams, e.g. youtube videos in webkit based browsers

Hi,

  My web browser crashes when i try to watch a youtube video or open certain web
pages with any webkit browser(that uses cairo).
  I found other peoples like me with this bug but i can't found a solution.
  This error already happened with me while using these web browsers: jumanji,
surf, uzbl, vimprobrable2, dwb. And using two diferents linux distributions.
Arch Linux and Gentoo, both in the same computer.

There is this problem archived on this mailing list
http://lists.cairographics.org/archives/cairo/2014-March/025089.html , but the
"solution" apresented in the end, as a patch
http://lists.cairographics.org/archives/cairo/2014-March/025103.html
didn't solved my problem. He says about the patch "This diff avoids the segfault
for me, but only papers over the bug..." So it's not the real solution.

I posted on Arch Linux and on Gentoo forum, the last have more information.
Gentoo forum post:
https://forums.gentoo.org/viewtopic-t-995484-start-0-postdays-0-postorder-asc-highlight-.html?sid=c5c15b4431456821dcc93bbbc74b9ace

This bug are kiling me, i can't watch any youtube video and lot of times
my browser suddenly close. Also the web browsers tha i most like uses
cairo.

There is the complete message from gdb while run the browser with all
dependencies compiled with debug flag.

---gdb.log---

Temporary breakpoint 1 at 0x407750: file jumanji.c, line 687.
Starting program: /usr/bin/jumanji
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
Starting program: /usr/bin/jumanji
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
Quit
A debugging session is active.
        Inferior 1 [process 25926] will be killed.
Quit anyway? (y or n) Starting program: /usr/bin/jumanji
warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffe7271700 (LWP 25999)]
[New Thread 0x7fffa6941700 (LWP 26000)]
[New Thread 0x7fffa5aec700 (LWP 26001)]
[New Thread 0x7fffa52eb700 (LWP 26002)]
[New Thread 0x7fffa4acb700 (LWP 26003)]
[New Thread 0x7fff97fff700 (LWP 26004)]
[New Thread 0x7fff977fe700 (LWP 26005)]
[New Thread 0x7fff96ffd700 (LWP 26006)]
[New Thread 0x7fff967fc700 (LWP 26007)]
[New Thread 0x7fff95ffb700 (LWP 26008)]
[New Thread 0x7fff953fa700 (LWP 26009)]
[New Thread 0x7fff7bfff700 (LWP 26010)]
[New Thread 0x7fff7b7fe700 (LWP 26011)]
[New Thread 0x7fff7affd700 (LWP 26012)]
[New Thread 0x7fff7a7fc700 (LWP 26013)]
[New Thread 0x7fff79ffb700 (LWP 26014)]
[Thread 0x7fff953fa700 (LWP 26009) exited]
[Thread 0x7fff7b7fe700 (LWP 26011) exited]
[Thread 0x7fff7affd700 (LWP 26012) exited]
[Thread 0x7fff7a7fc700 (LWP 26013) exited]
[Thread 0x7fff79ffb700 (LWP 26014) exited]
[New Thread 0x7fff79ffb700 (LWP 26015)]
[Thread 0x7fff79ffb700 (LWP 26015) exited]
[New Thread 0x7fff79ffb700 (LWP 26017)]
[New Thread 0x7fff7a7fc700 (LWP 26018)]
[New Thread 0x7fff7affd700 (LWP 26019)]
[New Thread 0x7fff7b7fe700 (LWP 26020)]
[Thread 0x7fff7a7fc700 (LWP 26018) ...

Created attachment 104383
quick hack

The problem here seems to be that with small areas to fill
the boxes tessellator receives empty boxes which are
evidently unexpected. The attached patch is derived going
one step back to the source of these empty boxes,
a comment there seems to imply a choice to mimic pixman.

Obviously the real problem could be earlier than there.

A different solution could be to impose a minimum 1 width/height
for the boxes generated.

Created attachment 104551
testcase

I cairo-traced a visit to youtube using 'surf' and reduced the trace
to the minimum still reproducing the crash in the form of a
cairo test-suite C file.

So, after applying the patch and running make -C test, if you
execute the test with:

> CAIRO_TEST_TARGET=xlib gdb -ex r -ex bt --args test/.libs/cairo-test-suite sweep-line-delete

you'll have a similar backtrace to the one in the bug report.

(In reply to comment #2)
> Created attachment 104551 [details] [review]
> testcase
>
> I cairo-traced a visit to youtube using 'surf' and reduced the trace
> to the minimum still reproducing the crash in the form of a
> cairo test-suite C file.

Would it be ok with you if this test case were included with Cairo? Would the following license header be suitable? Do you want some copyright notice to be included? Which one?

/*
 * 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.
 */

Created attachment 104568
alternative solution

An alternative patch that would make the rectangular tessellator
work also with empty boxes, so intrinsically safe also for
other (future) callers.

Obviously only valid if I understood what it's doing and it
is possible to effectively ignore empty boxes.

> Would it be ok with you if this test case were included with Cairo? Would the > following license header be suitable? Do you want some copyright notice to be > included? Which one?

it is ok with me, it is suitable, don't want any copyright notice, whatever.

Hi Massimo. Looks like you last patch with only 3 lines of code solved the problem!
I applied here and no more segfault.
How i don't understand and i know nothing about cairo, i don't know if what this patch do is the right thing or if is safe use it. All i know it is that it solved, i think.

Should i mark as solved?

I apllyied here and
(In reply to comment #4)
> Created attachment 104568 [details] [review]
> alternative solution
>
> An alternative patch that would make the rectangular tessellator
> work also with empty boxes, so intrinsically safe also for
> other (future) callers.
>
> Obviously only valid if I understood what it's doing and it
> is possible to effectively ignore empty boxes.
>
> > Would it be ok with you if this test case were included with Cairo? Would the > following license header be suitable? Do you want some copyright notice to be > included? Which one?
>
> it is ok with me, it is suitable, don't want any copyright notice, whatever.

(In reply to comment #5)
> Hi Massimo. Looks like you last patch with only 3 lines of code solved the
> problem!
> I applied here and no more segfault.
> How i don't understand and i know nothing about cairo, i don't know if what
> this patch do is the right thing or if is safe use it. All i know it is that
> it solved, i think.
>

To me it seems correct and safe, it only drops empty
boxes from the list of boxes to be tessellated.

Empty boxes do not alter the insideness of any pixel
because if a ray from the pixel intersects the top edge
it also intersects the bottom edge, one from the left
and the other from the right, so it should be correct
for both (EVEN_ODD, WINDING) fill rules that cairo implements.

OTOH these empty boxes are problematic as they possibly
lead to a segfault.

It is possible that there are better places to discard
these boxes.

> Should i mark as solved?

I'm not a cairo developer, but I'd say not until a fix
has been included in the official source tree. So people
experiencing the same problem can find a solution and report
shortcomings.

There are already many duplicates in many
distribution/application/library bugzilla

(In reply to comment #6)
> (In reply to comment #5)
> > Hi Massimo. Looks like you last patch with only 3 lines of code solved the
> > problem!
> > I applied here and no more segfault.
> > How i don't understand and i know nothing about cairo, i don't know if what
> > this patch do is the right thing or if is safe use it. All i know it is that
> > it solved, i think.
> >
>
> To me it seems correct and safe, it only drops empty
> boxes from the list of boxes to be tessellated.

It is. Could you please write a nice commit log, adding

Reported-by: Henrique Lengler <email address hidden>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81699

and submit. Ideally we would love to have a test case to hit this problem as well. If you can capture it using cairo-trace that would be a good start.

> Empty boxes do not alter the insideness of any pixel
> because if a ray from the pixel intersects the top edge
> it also intersects the bottom edge, one from the left
> and the other from the right, so it should be correct
> for both (EVEN_ODD, WINDING) fill rules that cairo implements.
>
> OTOH these empty boxes are problematic as they possibly
> lead to a segfault.
>
> It is possible that there are better places to discard
> these boxes.

That's my only worry, I am trying to remember all the call paths that enter here and why we have empty boxes in the first place. In this case, the empty boxes seem to be part of the clip, which is worrisome. All the zero height boxes should have been prefiltered...

diff --git a/src/cairo-boxes.c b/src/cairo-boxes.c
index 63b68dd..90afdbd 100644
--- a/src/cairo-boxes.c
+++ b/src/cairo-boxes.c
@@ -139,6 +139,8 @@ _cairo_boxes_add_internal (cairo_boxes_t *boxes,
     if (unlikely (boxes->status))
        return;

+ assert(box->x2 > box->x1 && box->y2 > box->y1);
+
     chunk = boxes->tail;
     if (unlikely (chunk->count == chunk->size)) {
        int size;

So yes, this suggests a far deeper problem than just the tesselate failure.

Created attachment 105050
proposed patch

Created attachment 105051
cairo script

This is the minimum cairo-trace/script that I obtained.

To crash it I compiled util/cairo-script/csi-replay.c after
changing the #define SINGLE_SURFACE to 0

To derive a test case from it probably it is possible
to use a smaller surface size.

Enabling xlib-xcb prevents the crash, probably another code path
is executed. (Valgrind (--enable-valgrind=no) reports an invalid
read though)

(In reply to comment #7)
[...]
> That's my only worry, I am trying to remember all the call paths that enter
> here and why we have empty boxes in the first place. In this case, the empty
> boxes seem to be part of the clip, which is worrisome. All the zero height
> boxes should have been prefiltered...
>
> diff --git a/src/cairo-boxes.c b/src/cairo-boxes.c
> index 63b68dd..90afdbd 100644
> --- a/src/cairo-boxes.c
> +++ b/src/cairo-boxes.c
> @@ -139,6 +139,8 @@ _cairo_boxes_add_internal (cairo_boxes_t *boxes,
> if (unlikely (boxes->status))
> return;
>
> + assert(box->x2 > box->x1 && box->y2 > box->y1);
> +
> chunk = boxes->tail;
> if (unlikely (chunk->count == chunk->size)) {
> int size;
>
> So yes, this suggests a far deeper problem than just the tesselate failure.

I guess you meant this: assert(box->p2.x > box->p1.x && box->p2.y > box->p1.y);

That assert triggers for 61 test cases in the test suite. Most of these are due to boxes likes this (this code appears in different places inside of cairo, e.g. _cairo_xcb_surface_fixup_unbounded_boxes and the span compositor's fixup_unbounded_boxes):

    box.p1.x = _cairo_fixed_from_int (extents->unbounded.x + extents->unbounded.width);
    box.p1.y = _cairo_fixed_from_int (extents->unbounded.y);
    box.p2.x = _cairo_fixed_from_int (extents->unbounded.x);
    box.p2.y = _cairo_fixed_from_int (extents->unbounded.y + extents->unbounded.height);

I guess that means that this code is wrong and should be fixed? Perhaps we should even commit this assert to cairo?

At least I didn't find anything generating zero-height boxes.

List of tests: big-empty-box big-empty-triangle big-little-box bug-40410 bug-bo-collins bug-bo-rectangular clip-complex-bug61492 clip-complex-shape-eo-aa clip-complex-shape-eo-mono clip-fill clip-fill-eo-unbounded clip-fill-nz-unbounded clip-group-shapes-unaligned-rectangles clip-mixed-antialias clip-nesting clip-operator clip-shape clip-stroke-unbounded clip-text clip-twice copy-disjoint fill-disjoint get-path-extents hatchings image-surface-source mask operator operator-alpha operator-alpha-alpha paint-with-alpha-clip-mask pdf-surface-source ps-surface-source random-clip record-paint-alpha-clip-mask record-self-intersecting record1414x-self-intersecting record2x-self-intersecting record90-self-intersecting recordflip-self-intersecting rectilinear-fill rotated-clip self-copy self-copy-overlap self-intersecting subsurface-image-repeat subsurface-modify-child subsurface-modify-parent subsurface-pad subsurface-reflect subsurface-repeat surface-pattern-operator svg-surface-source text-glyph-range tighten-bounds trap-clip unantialiased-shapes unbounded-operator white-in-noop xcb-surface-source xlib-surface-source zero-mask

Oh and the assert does not trigger for the test case attached to this bug report (except for the "test-traps" (pseudo-)backend, which doesn't count, I guess). But that test case doesn't crash here either...?

Hmm, nope. We only reject the empty boxes, but allow negative boxes to represent counter winding. That's cunning.

Ok, found the problem. It's the traps-to-boxes routine that doesn't prefilter zero height traps/boxes.

commit 13a09526d2120c244471e03b6ae979016ef88e83
Author: Chris Wilson <email address hidden>
Date: Sat Aug 23 14:16:55 2014 +0100

    traps,xcb: Prefilter zero-area boxes when converting traps

    The rectangular tesselation routines rely on the presuming that all the
    boxes it has to handle are already filtered to remove empty boxes.

    << /width 800 /height 600 >> surface context
    0.0848671 0 0 0.0848671 39.907812 5.608896 matrix transform
    8 0 m 12.417969 0 16 3.582031 16 8 c 16 12.417969 12.417969 16 8 16 c
    3.582031 16 0 12.417969 0 8 c 0 3.582031 3.582031 0 8 0 c h
    clip
    16 0 m 8 8 l 16 16 l h
    clip
    0 0 16 16 rectangle
    fill

    Triggers the error given a traps tesselator like cairo-xlib.

    Reported-by: Henrique Lengler <email address hidden>
    Analyzed-by: Massimo <email address hidden>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81699
    Signed-off-by: Chris Wilson <email address hidden>

Fixed, but leaving open to add the testcase.

*** Bug 72244 has been marked as a duplicate of this bug. ***

*** Bug 76272 has been marked as a duplicate of this bug. ***

Created attachment 105218
Crash backtrace

Hi Chris,

thanks for the fix, it greatly increased stability.

But unfortunately sometimes there are still crashes.

Attached is a backtrace for the following (this is Debian 1.12.16-2 plus commit 13a09526 from master):

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff2cb0f98 in sweep_line_delete_edge (edge=0x7fffffff7b00,
    sweep=0x7fffffff77f0)
    at /tmp/cairo-1.12.16/src/cairo-bentley-ottmann-rectangular.c:558
558 edge->next->prev = edge->prev;

/me hangs head in shame

commit a5f51588afd9d5629b03297eb29ff46350b6ba50
Author: Chris Wilson <email address hidden>
Date: Mon Aug 25 08:55:24 2014 +0100

    traps,xcb: Set the box count after filtering

    After converting, the number of boxes should only count the number of
    non-zero boxes and forget about the zero-sized boxes we skipped over.

    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81699
    Signed-off-by: Chris Wilson <email address hidden>

Hi Chris,

thanks a lot, it no longer crashes for me.

Can these commits also go to the 1.12 branch?

Cherry-picked 28 commits into the 1.12 branch. I just did a quick search through the git history since 1.12 was branched off master (just after 1.12.16) and took everything which sounded harmless enough.

These two commits are commit 3bb80aa2c3f97c071f434e0fbb6704fbef963352 and commit 4b65497231d1859e03762949896da94ffde389b on the branch.

CONFIRMING for cairo v1.13.1

$ cat /etc/system-release && uname -a
Fedora release 20 (Heisenbug)
Linux localhost.localdomain 3.17.7-200.fc20.x86_64 #1 SMP Wed Dec 17 03:35:33 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

$ yum info installed cairo
Loaded plugins: langpacks, priorities, refresh-packagekit
Installed Packages
Name : cairo
Arch : x86_64
Version : 1.13.1
Release : 0.1.git337ab1f.fc20
Size : 1.7 M
Repo : installed
From repo : fedora
Summary : A 2D graphics library
URL : http://cairographics.org
License : LGPLv2 or MPLv1.1

If I understand correctly, this issue has been resolved as of the two commits mentioned by Uli in comment #18, which I've confirmed are included in trunk.

If there are other related changes needed (e.g. test cases?) please re-open and clarify what the remaining tasks are.