| Summary: | [need testcase] Segfault sweep_line_delete on video playback (2) | ||
|---|---|---|---|
| Product: | cairo | Reporter: | Henrique Lengler <henriqueleng> |
| Component: | general | Assignee: | Chris Wilson <chris> |
| Status: | RESOLVED FIXED | QA Contact: | cairo-bugs mailing list <cairo-bugs> |
| Severity: | critical | ||
| Priority: | high | CC: | bunk, henriqueleng, marc, sidicas2 |
| Version: | 1.12.16 | ||
| Hardware: | x86-64 (AMD64) | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
quick hack
testcase alternative solution proposed patch cairo script Crash backtrace |
||
|
Description
Henrique Lengler
2014-07-24 04:52:55 UTC
Created attachment 104383 [details] [review] 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 [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. 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] [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 [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. 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] [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 <henriqueleng@openmailbox.org> 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 [details] [review] proposed patch Created attachment 105051 [details]
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 <chris@chris-wilson.co.uk> 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 <henriqueleng@openmailbox.org> Analyzed-by: Massimo <sixtysix@inwind.it> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81699 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> 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 [details]
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 <chris@chris-wilson.co.uk> 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 <chris@chris-wilson.co.uk> 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. |
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.