Memory leak with image surfaces

Hello!

The following test produces a memory leak in the read_png() method:

#include <cairo/cairo.h>
#include <stdlib.h>

int main() {
    int              w, h;
    cairo_surface_t *image;
    cairo_surface_t *pdf;
    cairo_t *        cr;
    int i;

    pdf = cairo_pdf_surface_create("/tmp/lol.pdf", 1500, 1500);
    cr = cairo_create(pdf);

    image = cairo_image_surface_create_from_png ("./test.png");
    w = cairo_image_surface_get_width (image);
    h = cairo_image_surface_get_height (image);

    cairo_scale (cr, 256.0/w, 256.0/h);

    cairo_set_source_surface (cr, image, 0, 0);
    cairo_paint (cr);
    cairo_surface_destroy (image);

    cairo_show_page(cr);

    cairo_surface_destroy(pdf);
    cairo_destroy (cr);
}

Valgrind output :

==29797== HEAP SUMMARY:
==29797==     in use at exit: 172,738 bytes in 13 blocks
==29797==   total heap usage: 84 allocs, 71 frees, 997,686 bytes allocated
==29797==
==29797== 160,000 bytes in 1 blocks are definitely lost in loss record 13 of 13
==29797==    at 0x4C28BED: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29797==    by 0x4EDC7D1: read_png (cairo-png.c:661)
==29797==    by 0x4EDD1ED: cairo_image_surface_create_from_png (cairo-png.c:779)
==29797==    by 0x400AAC: main (test.c:14)
==29797==
==29797== LEAK SUMMARY:
==29797==    definitely lost: 160,000 bytes in 1 blocks
==29797==    indirectly lost: 0 bytes in 0 blocks
==29797==      possibly lost: 0 bytes in 0 blocks
==29797==    still reachable: 12,738 bytes in 12 blocks
==29797==         suppressed: 0 bytes in 0 blocks

I ran into the same problem with larger PDF files, the decompressed
images are never freed.

The issue seems to be related to the "COW snapshots" feature :
"_cairo_image_surface_snapshot" create a clone of the surface but set
the "owns_data" flag of both the surface and its clone to FALSE. This
can lead in a situation where neither the original surface nor its
clone own the data, this results in a memory leak because none of them
take the responsability to free it.

My guess is that the clone should copy the "owns_data" flag of the
original surface before setting the "owns_data" flag of the original
surface to FALSE. This way the clone is responsible to free the data
only if the original surface owned it. I'm not quite sure about all
the implications of the attached patch, but it fixes the leak for my
use case.

--- a/src/cairo-image-surface.c
+++ b/src/cairo-image-surface.c
@@ -762,12 +762,13 @@ _cairo_image_surface_snapshot (void *abstract_surface)
     return &clone->base;

  image->pixman_image = NULL;
+
+ clone->owns_data = image->owns_data;
  image->owns_data = FALSE;

  clone->transparency = image->transparency;
  clone->color = image->color;

- clone->owns_data = FALSE;
  return &clone->base;
}

Thanks!

-- 
Victor Goya

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo

From 8a512b1dc4fff4251a33e26175bd31663e68296d Mon Sep 17 00:00:00 2001
From: Victor Goya <[email protected]>
Date: Fri, 14 Jun 2013 20:19:57 +0200
Subject: [PATCH] Fix possible memory leak in the image snapshot creation.

---
 src/cairo-image-surface.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c
index 49f6e18..c5292f8 100644
--- a/src/cairo-image-surface.c
+++ b/src/cairo-image-surface.c
@@ -762,12 +762,13 @@ _cairo_image_surface_snapshot (void *abstract_surface)
 	    return &clone->base;
 
 	image->pixman_image = NULL;
+
+	clone->owns_data = image->owns_data;
 	image->owns_data = FALSE;
 
 	clone->transparency = image->transparency;
 	clone->color = image->color;
 
-	clone->owns_data = FALSE;
 	return &clone->base;
     }
 
-- 
1.7.10.4