After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 787626 - pixels never get freed after gdk_pixbuf_get_pixels_with_length() call
pixels never get freed after gdk_pixbuf_get_pixels_with_length() call
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2017-09-13 13:05 UTC by Vitaly Kirsanov
Modified: 2018-04-25 14:56 UTC
See Also:
GNOME target: ---
GNOME version: 3.21/3.22

Attachments
Reproducer. Be carefull memory leaks very fast (601 bytes, text/plain)
2017-09-13 13:05 UTC, Vitaly Kirsanov 		  Details
Valgrind output (20.60 KB, text/plain)
2017-09-13 13:06 UTC, Vitaly Kirsanov 		  Details
The patch (533 bytes, patch)
2017-09-13 13:07 UTC, Vitaly Kirsanov 		committed Details | Review

Description Vitaly Kirsanov 2017-09-13 13:05:43 UTC 
Created attachment 359715 [details]
Reproducer. Be carefull memory leaks very fast

Steps to reproduce:

1. Create a GBytes with g_bytes_new_static () (say, 3MB size)
2. Create a GdkPixbuf with gdk_pixbuf_new_from_bytes() 
3. Call gdk_pixbuf_get_pixels_with_length()
4. Destroy GdkPixbuf
5. Go to step 2

If you run the algorithm endlessly you will see that RAM gets monotonously consumed. I've attached a C reproducer and some valgrind output. You can see that memory allocated in g_bytes_unref_to_data() is never freed.

==12342== 688,914,432 bytes in 219 blocks are definitely lost in loss record 312 of 312
==12342==    at 0x4C2BEDF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==12342==    by 0x5300C78: g_malloc (gmem.c:94)
==12342==    by 0x531B357: g_memdup (gstrfuncs.c:391)
==12342==    by 0x52D6842: g_bytes_unref_to_data (gbytes.c:466)
==12342==    by 0x4E3F7D0: gdk_pixbuf_get_pixels_with_length (gdk-pixbuf.c:716)
==12342==    by 0x4008E1: main (in /home/vkirsano/sandbit/gdk-pixbuf/a.out)

I guess to fix this we can use the fact that the memory (duplicated or not) which returned by g_bytes_unref_to_data() can always be freed by g_free(). Although I'm not sure if it's documented anywhere. So.. since we set mut_pixbuf->pixels and mut_pixbuf->bytes inside gdk_pixbuf_get_pixels_with_length() we also have to set mut_pixbuf->destroy_fn to something. And since we know (at least it can be seen in the implementation of g_bytes_unref_to_data()) that the new memory can be freed by g_free() it can be a safe destructor candidate.

I've also attached a sketch of a patch fixing this.
Comment 1 Vitaly Kirsanov 2017-09-13 13:06:19 UTC 
Created attachment 359716 [details]
Valgrind output
Comment 2 Vitaly Kirsanov 2017-09-13 13:07:12 UTC 
Created attachment 359717 [details] [review]
The patch
Comment 3 Emmanuele Bassi (:ebassi) 2017-09-13 16:05:32 UTC 
Review of attachment 359717 [details] [review]:

Please, don't change random fields in Bugzilla.

::: gdk-pixbuf/gdk-pixbuf.c
@@ +716,3 @@
                 mut_pixbuf->pixels = g_bytes_unref_to_data (pixbuf->bytes, &len);
                 mut_pixbuf->bytes = NULL;
+                mut_pixbuf->destroy_fn = free_buffer;

This is technically correct, but I'm a bit miffed as to why GBytes is copying the data if the bytes buffer is static.
Comment 4 Emmanuele Bassi (:ebassi) 2017-09-13 16:06:20 UTC 
The semantics of g_bytes_unref_to_data() are definitely too clever for its own good…
Comment 5 Emmanuele Bassi (:ebassi) 2018-04-25 14:55:54 UTC 
Thanks for your patience, and the patch.