[cairo] crash copying recording surface to PDF surface with tags

[Uli Schlachter]

Hi Ben,

thanks for this great report. It is always great to read:

Am 26.12.20 um 05:59 schrieb Ben Pfaff:
> but I have
> appended to this email a simple test program (greatly simplified from
> my real program) that reproduces the problem.

I put your report here:
https://gitlab.freedesktop.org/cairo/cairo/-/issues/448

I just noticed that [1] says to discuss bugs on the mailing list, but I
think that we should reconsider that, but that's another discussion...

[1]: https://www.cairographics.org/contact/

Anyway, I feel like this might be multiple bug reports in one, so I'd be
happy if you could test my patches. This should fix the crashes (at
least those that I saw so far):

diff --git a/src/cairo-recording-surface.c b/src/cairo-recording-surface.c
index 6df8b0821..e6050706b 100644
--- a/src/cairo-recording-surface.c
+++ b/src/cairo-recording-surface.c
@@ -1764,6 +1765,9 @@ _cairo_recording_surface_merge_source_attributes
(cairo_recording_surface_t  *su
 	if (_cairo_surface_is_snapshot (surf))
 	    free_me = surf = _cairo_surface_snapshot_get_target (surf);

+	if (unlikely (surf->status))
+	    return;
+
 	if (surf->type == CAIRO_SURFACE_TYPE_RECORDING) {
 	    cairo_recording_surface_t *rec_surf = (cairo_recording_surface_t
*) surf;

diff --git a/src/cairo-surface-snapshot.c b/src/cairo-surface-snapshot.c
index a8b8c0e45..a1532b5b5 100644
--- a/src/cairo-surface-snapshot.c
+++ b/src/cairo-surface-snapshot.c
@@ -71,7 +71,8 @@ _cairo_surface_snapshot_flush (void *abstract_surface,
unsigned flags)
     cairo_status_t status;

     target = _cairo_surface_snapshot_get_target (&surface->base);
-    status = _cairo_surface_flush (target, flags);
+    if (!target->status)
+	status = _cairo_surface_flush (target, flags);
     cairo_surface_destroy (target);

     return status;


[...]
> == Extents on recording surface ==
> 
> The problem is related to specifying extents on the recording surface.
> If you run it with --no-extents to avoid specifying the extents, there
> is no problem.
> 
> However, "valgrind --leak-check=full ./cairo-test --no-extents"
> does report a leak:
> 
>     384 bytes in 1 blocks are definitely lost in loss record 4 of 11
>        at 0x483877F: malloc (vg_replace_malloc.c:307)
>        by 0x48EBAF9: _cairo_surface_snapshot (cairo-surface-snapshot.c:264)
>        by 0x48C986B: _cairo_pattern_init_snapshot (cairo-pattern.c:422)
>        by 0x48DA700: _cairo_recording_surface_paint (cairo-recording-surface.c:743)
>        by 0x48F0277: _cairo_surface_paint (cairo-surface.c:2198)
>        by 0x48F0277: _cairo_surface_paint (cairo-surface.c:2171)
>        by 0x48F0277: _cairo_surface_paint (cairo-surface.c:2198)
>        by 0x48F0277: _cairo_surface_paint (cairo-surface.c:2171)
>        by 0x48A76D5: _cairo_gstate_paint (cairo-gstate.c:1061)
>        by 0x48FD044: cairo_paint (cairo.c:2220)
>        by 0x10930C: copy_surface (cairo-test.c:25)
>        by 0x1094CA: main (cairo-test.c:57)

I did not investigate this so far.

[...]
> I'd very much appreciate your guidance!

I saw that copying the recording surface fails internally, but I did not
yet investigate the root cause. Instead, I only tried to fix the
resulting crashes from the unexpected error surface. So this is another
"to do" item.

Cheers,
Uli
-- 
This can be a, a little complicated. Listen, my advice is... ask
somebody else for advice, at least someone who's... got more experience
at...  giving advice.