Make sure the surfaces own their pixels

When creating a surface for pixel data, make sure the surface keeps
a reference to the pixels, either by directly owning them or by
keeping a reference to the GdkPixbuf owning them.

This fixes a crash when rendering some SVG files (e.g. [1]) to a
recording or pdf surface.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=30071 .

==27565== Unaddressable byte(s) found during client check request
==27565==    at 0x427E2C0: _cairo_debug_check_image_surface_is_defined
(cairo-debug.c:125)
==27565==    by 0x42B5749: _cairo_surface_acquire_source_image
(cairo-surface.c:1447)
==27565==    by 0x42BC119: _cairo_surface_snapshot_copy_on_write
(cairo-surface-snapshot.c:125)
==27565==    by 0x42B407E: _cairo_surface_detach_snapshot (cairo-surface.c:329)
==27565==    by 0x42B3FE9: _cairo_surface_detach_snapshots
(cairo-surface.c:314)
==27565==    by 0x42B49D0: cairo_surface_finish (cairo-surface.c:715)
==27565==    by 0x42B48EF: cairo_surface_destroy (cairo-surface.c:645)
==27565==    by 0x42A16DA: _cairo_pattern_fini (cairo-pattern.c:346)
==27565==    by 0x42A21D2: cairo_pattern_destroy (cairo-pattern.c:828)
==27565==    by 0x4281FD8: _cairo_gstate_fini (cairo-gstate.c:229)
==27565==    by 0x428211F: _cairo_gstate_restore (cairo-gstate.c:290)
==27565==    by 0x4276D86: cairo_restore (cairo.c:583)
==27565==    by 0x40390B0: rsvg_cairo_pop_discrete_layer
(rsvg-cairo-draw.c:1003)
==27565==    by 0x40380CD: rsvg_cairo_render_path (rsvg-cairo-draw.c:639)
==27565==    by 0x4035C4D: rsvg_render_path (rsvg-base.c:2067)
==27565==    by 0x40287FE: _rsvg_node_rect_draw (rsvg-shapes.c:445)
==27565==    by 0x4029E89: rsvg_node_draw (rsvg-structure.c:69)
==27565==    by 0x4029F34: _rsvg_node_draw_children (rsvg-structure.c:87)
==27565==    by 0x4029E89: rsvg_node_draw (rsvg-structure.c:69)
==27565==    by 0x402A9A9: rsvg_node_svg_draw (rsvg-structure.c:326)
==27565==    by 0x4029E89: rsvg_node_draw (rsvg-structure.c:69)
==27565==    by 0x4039D49: rsvg_handle_render_cairo_sub
(rsvg-cairo-render.c:234)
==27565==    by 0x4039DA1: rsvg_handle_render_cairo (rsvg-cairo-render.c:256)
==27565==    by 0x804A06A: main (rsvg-convert.c:319)
==27565==  Address 0x6c6b028 is not stack'd, malloc'd or (recently) free'd

[1] http://websvn.kde.org/*checkout*/trunk/KDE/kdegames/libkdegames/carddecks/svg-oxygen-white/oxygen-white.svgz?revision=896352

1 files changed, 59 insertions, 33 deletions

diff --git a/rsvg-cairo-draw.c b/rsvg-cairo-draw.c
index f15589a6..e42b2985 100644
--- a/rsvg-cairo-draw.c
+++ b/rsvg-cairo-draw.c
@@ -42,6 +42,8 @@
 
 #include <pango/pangocairo.h>
 
+static const cairo_user_data_key_t surface_pixel_data_key;
+
 static void
 _rsvg_cairo_set_shape_antialias (cairo_t * cr, ShapeRenderingProperty aa)
 {
@@ -655,13 +657,16 @@ rsvg_cairo_render_image (RsvgDrawingCtx * ctx, const GdkPixbuf * pixbuf,
     guchar *cairo_pixels;
     cairo_format_t format;
     cairo_surface_t *surface;
-    static const cairo_user_data_key_t key;
     int j;
     RsvgBbox bbox;
 
     if (pixbuf == NULL)
         return;
 
+    cairo_pixels = g_try_malloc (4 * width * height);
+    if (!cairo_pixels)
+        return;
+
     rsvg_bbox_init (&bbox, state->affine);
     bbox.x = pixbuf_x;
     bbox.y = pixbuf_y;
@@ -679,13 +684,9 @@ rsvg_cairo_render_image (RsvgDrawingCtx * ctx, const GdkPixbuf * pixbuf,
     else
         format = CAIRO_FORMAT_ARGB32;
 
-    cairo_pixels = g_try_malloc (4 * width * height);
-	if (!cairo_pixels)
-		return;
-
     surface = cairo_image_surface_create_for_data ((unsigned char *) cairo_pixels,
                                                    format, width, height, 4 * width);
-    cairo_surface_set_user_data (surface, &key, cairo_pixels, (cairo_destroy_func_t) g_free);
+    cairo_surface_set_user_data (surface, &surface_pixel_data_key, cairo_pixels, (cairo_destroy_func_t) g_free);
 
     for (j = height; j; j--) {
         guchar *p = gdk_pixels;
@@ -777,6 +778,10 @@ rsvg_cairo_generate_mask (cairo_t * cr, RsvgMask * self, RsvgDrawingCtx * ctx, R
     double sx, sy, sw, sh;
     gboolean nest = cr != render->initial_cr;
 
+    pixels = g_try_malloc0 (height * rowstride);
+    if (pixels == NULL)
+          return;
+
     if (self->maskunits == objectBoundingBox)
         _rsvg_push_view_box (ctx, 1, 1);
 
@@ -788,9 +793,9 @@ rsvg_cairo_generate_mask (cairo_t * cr, RsvgMask * self, RsvgDrawingCtx * ctx, R
     if (self->maskunits == objectBoundingBox)
         _rsvg_pop_view_box (ctx);
 
-    pixels = g_new0 (guint8, height * rowstride);
     surface = cairo_image_surface_create_for_data (pixels,
                                                    CAIRO_FORMAT_ARGB32, width, height, rowstride);
+    cairo_surface_set_user_data (surface, &surface_pixel_data_key, pixels, (cairo_destroy_func_t) g_free);
 
     mask_cr = cairo_create (surface);
     save_cr = render->cr;
@@ -847,7 +852,6 @@ rsvg_cairo_generate_mask (cairo_t * cr, RsvgMask * self, RsvgDrawingCtx * ctx, R
                         nest ? 0 : render->offset_x,
                         nest ? 0 : render->offset_y);
     cairo_surface_destroy (surface);
-    g_free (pixels);
 }
 
 static void
@@ -887,22 +891,31 @@ rsvg_cairo_push_render_stack (RsvgDrawingCtx * ctx)
     else {
         guchar *pixels;
         int rowstride = render->width * 4;
-        pixels = g_new0 (guint8, render->width * render->height * 4);
+        GdkPixbuf *pixbuf;
+
+        pixels = g_try_malloc0 (render->width * render->height * 4);
+        if (pixels == NULL)
+            return; /* not really correct, but the best we can do here */
+
+        /* The pixbuf takes ownership of @pixels */
+        pixbuf = gdk_pixbuf_new_from_data (pixels,
+                                           GDK_COLORSPACE_RGB,
+                                           TRUE,
+                                           8,
+                                           render->width,
+                                           render->height,
+                                           rowstride,
+                                           (GdkPixbufDestroyNotify) rsvg_pixmap_destroy,
+                                           NULL);
+        render->pixbuf_stack = g_list_prepend (render->pixbuf_stack, pixbuf);
 
         surface = cairo_image_surface_create_for_data (pixels,
                                                        CAIRO_FORMAT_ARGB32,
                                                        render->width, render->height, rowstride);
-        render->pixbuf_stack =
-            g_list_prepend (render->pixbuf_stack,
-                            gdk_pixbuf_new_from_data (pixels,
-                                                      GDK_COLORSPACE_RGB,
-                                                      TRUE,
-                                                      8,
-                                                      render->width,
-                                                      render->height,
-                                                      rowstride,
-                                                      (GdkPixbufDestroyNotify) rsvg_pixmap_destroy,
-                                                      NULL));
+        /* Also keep a reference to the pixbuf which owns the pixels */
+        cairo_surface_set_user_data (surface, &surface_pixel_data_key,
+                                     g_object_ref (pixbuf),
+                                     (cairo_destroy_func_t) g_object_unref);
     }
     child_cr = cairo_create (surface);
     cairo_surface_destroy (surface);
@@ -929,7 +942,6 @@ rsvg_cairo_pop_render_stack (RsvgDrawingCtx * ctx)
     RsvgCairoRender *render = (RsvgCairoRender *) ctx->render;
     cairo_t *child_cr = render->cr;
     gboolean lateclip = FALSE;
-    GdkPixbuf *output = NULL;
     cairo_surface_t *surface = NULL;
     RsvgState *state = rsvg_current_state (ctx);
     gboolean nest;
@@ -945,6 +957,7 @@ rsvg_cairo_pop_render_stack (RsvgDrawingCtx * ctx)
 
     if (state->filter) {
         GdkPixbuf *pixbuf = render->pixbuf_stack->data;
+        GdkPixbuf *output;
 
         render->pixbuf_stack = g_list_remove (render->pixbuf_stack, pixbuf);
 
@@ -957,6 +970,10 @@ rsvg_cairo_pop_render_stack (RsvgDrawingCtx * ctx)
                                                        gdk_pixbuf_get_width (output),
                                                        gdk_pixbuf_get_height (output),
                                                        gdk_pixbuf_get_rowstride (output));
+        cairo_surface_set_user_data (surface, &surface_pixel_data_key,
+                                     g_object_ref (output),
+                                     (cairo_destroy_func_t) g_object_unref);
+
     } else
         surface = cairo_get_target (child_cr);
 
@@ -991,7 +1008,6 @@ rsvg_cairo_pop_render_stack (RsvgDrawingCtx * ctx)
     render->bb_stack = g_list_delete_link (render->bb_stack, render->bb_stack);
 
     if (state->filter) {
-        g_object_unref (output);
         cairo_surface_destroy (surface);
     }
 }
@@ -1031,9 +1047,29 @@ rsvg_cairo_get_image_of_node (RsvgDrawingCtx * ctx,
     RsvgCairoRender *render;
 
     rowstride = width * 4;
-    pixels = g_new0 (guint8, width * height * 4);
+    pixels = g_try_malloc0 (width * height * 4);
+    if (pixels == NULL)
+        return NULL;
+
+    /* no colorspace conversion necessary. this is just a convenient
+       holder of ARGB data with a width, height, & stride */
+    img = gdk_pixbuf_new_from_data (pixels,
+                                    GDK_COLORSPACE_RGB,
+                                    TRUE,
+                                    8,
+                                    width,
+                                    height,
+                                    rowstride,
+                                    (GdkPixbufDestroyNotify) rsvg_pixmap_destroy,
+                                    NULL);
+
     surface = cairo_image_surface_create_for_data (pixels,
                                                    CAIRO_FORMAT_ARGB32, width, height, rowstride);
+    /* Also keep a reference to the pixbuf which owns the pixels */
+    cairo_surface_set_user_data (surface, &surface_pixel_data_key,
+                                 g_object_ref (img),
+                                 (cairo_destroy_func_t) g_object_unref);
+
     cr = cairo_create (surface);
     cairo_surface_destroy (surface);
 
@@ -1044,16 +1080,6 @@ rsvg_cairo_get_image_of_node (RsvgDrawingCtx * ctx,
     rsvg_node_draw (drawable, ctx, 0);
     rsvg_state_pop (ctx);
 
-    /* no colorspace conversion necessary. this is just a convenient
-       holder of ARGB data with a width, height, & stride */
-    img = gdk_pixbuf_new_from_data (pixels,
-                                    GDK_COLORSPACE_RGB,
-                                    TRUE,
-                                    8,
-                                    width,
-                                    height,
-                                    rowstride, (GdkPixbufDestroyNotify) rsvg_pixmap_destroy, NULL);
-
     cairo_destroy (cr);
 
     rsvg_render_free (ctx->render);