[cairo] Memory leak with image surfaces

[Victor Goya]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_memory_leak_in_image_snapshots.patch
Type: application/octet-stream
Size: 835 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20130614/27c29619/attachment-0001.obj>