Draft: Fix some GLib possible memory leaks (!2135) · Merge requests · GNOME / GLib

The source project of this merge request has been removed.

SASU OKFT requested to merge (removed):master into master Jun 04, 2021

Hi everyone,

I'm developing an application that depends on a library (libvips) using GLib internally. I regularly check potential memory leaks of my program thanks to valgrind and started seeing a bunch of leaks when I added this dependency. I already sent a MR to libvips which got accepted and now, I'm trying to fix GLib leaks.

I know there is a debate when it comes to free statically allocated data but I found this MR and since it got merged, I'd think I'm right.

My test program hello.c is as follow:

#include <glib.h>

int main(int argc, char **argv)
{
  g_set_prgname(argv[0]);
  return 0;
}

I'm calling g_set_prgname because libvips does that and without that call, my test program is not linked to GLib. I don't think you expect people using GLib to call this function with NULL when their program exits so I guess it has to be done on GLib side.

I compiled it using

clang -g -O0 hello.c `pkg-config --cflags --libs glib-2.0` -o hello

I then ran valgring using the following command:

GLIBCXX_FORCE_NEW=1 G_DEBUG=gc-friendly G_SLICE=always-malloc valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes ./hello

I got 10 records of lost data:

==10307== HEAP SUMMARY:
==10307==     in use at exit: 18,812 bytes in 10 blocks
==10307==   total heap usage: 10 allocs, 0 frees, 18,812 bytes allocated
==10307== 
==10307== 4 bytes in 1 blocks are still reachable in loss record 1 of 10
==10307==    at 0x4842839: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x493D425: g_private_impl_new (gthread-posix.c:1027)
==10307==    by 0x493C5F6: g_private_get_impl (gthread-posix.c:1055)
==10307==    by 0x493C594: g_private_get (gthread-posix.c:1082)
==10307==    by 0x48EE7A3: thread_memory_from_self (gslice.c:552)
==10307==    by 0x48EE680: g_slice_alloc (gslice.c:1047)
==10307==    by 0x48AB811: g_hash_table_new_full (ghash.c:1071)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48DD898: g_quark_init (gquark.c:61)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307== 
==10307== 8 bytes in 1 blocks are still reachable in loss record 2 of 10
==10307==    at 0x4842839: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFF6F: g_malloc (gmem.c:106)
==10307==    by 0x48D02B2: g_malloc_n (gmem.c:344)
==10307==    by 0x48F2645: g_strdup (gstrfuncs.c:364)
==10307==    by 0x491B28C: g_set_prgname (gutils.c:1090)
==10307==    by 0x401151: main (hello.c:5)
==10307== 
==10307== 32 bytes in 1 blocks are still reachable in loss record 3 of 10
==10307==    at 0x4847A25: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFFF4: g_malloc0 (gmem.c:136)
==10307==    by 0x48D0342: g_malloc0_n (gmem.c:368)
==10307==    by 0x48AB9E3: g_hash_table_setup_storage (ghash.c:591)
==10307==    by 0x48AB8A4: g_hash_table_new_full (ghash.c:1083)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48DD898: g_quark_init (gquark.c:61)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 32 bytes in 1 blocks are still reachable in loss record 4 of 10
==10307==    at 0x4847A25: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFFF4: g_malloc0 (gmem.c:136)
==10307==    by 0x48D0342: g_malloc0_n (gmem.c:368)
==10307==    by 0x48AB9E3: g_hash_table_setup_storage (ghash.c:591)
==10307==    by 0x48AB8A4: g_hash_table_new_full (ghash.c:1083)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48A643F: g_error_init (gerror.c:523)
==10307==    by 0x48C0063: glib_init (glib-init.c:340)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 64 bytes in 1 blocks are still reachable in loss record 5 of 10
==10307==    at 0x4842749: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48D0087: g_realloc (gmem.c:171)
==10307==    by 0x48ADDD7: g_hash_table_realloc_key_or_value_array (ghash.c:380)
==10307==    by 0x48AB9B7: g_hash_table_setup_storage (ghash.c:589)
==10307==    by 0x48AB8A4: g_hash_table_new_full (ghash.c:1083)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48DD898: g_quark_init (gquark.c:61)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 64 bytes in 1 blocks are still reachable in loss record 6 of 10
==10307==    at 0x4842749: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48D0087: g_realloc (gmem.c:171)
==10307==    by 0x48ADDD7: g_hash_table_realloc_key_or_value_array (ghash.c:380)
==10307==    by 0x48AB9B7: g_hash_table_setup_storage (ghash.c:589)
==10307==    by 0x48AB8A4: g_hash_table_new_full (ghash.c:1083)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48A643F: g_error_init (gerror.c:523)
==10307==    by 0x48C0063: glib_init (glib-init.c:340)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 96 bytes in 1 blocks are still reachable in loss record 7 of 10
==10307==    at 0x4842839: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFF6F: g_malloc (gmem.c:106)
==10307==    by 0x48EE75A: g_slice_alloc (gslice.c:1069)
==10307==    by 0x48AB811: g_hash_table_new_full (ghash.c:1071)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48DD898: g_quark_init (gquark.c:61)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 96 bytes in 1 blocks are still reachable in loss record 8 of 10
==10307==    at 0x4842839: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFF6F: g_malloc (gmem.c:106)
==10307==    by 0x48EE75A: g_slice_alloc (gslice.c:1069)
==10307==    by 0x48AB811: g_hash_table_new_full (ghash.c:1071)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48A643F: g_error_init (gerror.c:523)
==10307==    by 0x48C0063: glib_init (glib-init.c:340)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== 2,032 bytes in 1 blocks are still reachable in loss record 9 of 10
==10307==    at 0x4847A25: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFFF4: g_malloc0 (gmem.c:136)
==10307==    by 0x49010B8: g_private_set_alloc0 (gthread.c:541)
==10307==    by 0x48EE814: thread_memory_from_self (gslice.c:564)
==10307==    by 0x48EE680: g_slice_alloc (gslice.c:1047)
==10307==    by 0x48AB811: g_hash_table_new_full (ghash.c:1071)
==10307==    by 0x48AB7E3: g_hash_table_new (ghash.c:1034)
==10307==    by 0x48DD898: g_quark_init (gquark.c:61)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307== 
==10307== 16,384 bytes in 1 blocks are still reachable in loss record 10 of 10
==10307==    at 0x4842839: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==10307==    by 0x48CFF6F: g_malloc (gmem.c:106)
==10307==    by 0x48D02B2: g_malloc_n (gmem.c:344)
==10307==    by 0x48DD8AE: g_quark_init (gquark.c:62)
==10307==    by 0x48C005E: glib_init (glib-init.c:339)
==10307==    by 0x48C0158: glib_init_ctor (glib-init.c:402)
==10307==    by 0x401196D: call_init.part.0 (dl-init.c:74)
==10307==    by 0x4011A57: call_init (dl-init.c:37)
==10307==    by 0x4011A57: _dl_init (dl-init.c:121)
==10307==    by 0x4001109: ??? (in /usr/lib/x86_64-linux-gnu/ld-2.33.so)
==10307== 
==10307== LEAK SUMMARY:
==10307==    definitely lost: 0 bytes in 0 blocks
==10307==    indirectly lost: 0 bytes in 0 blocks
==10307==      possibly lost: 0 bytes in 0 blocks
==10307==    still reachable: 18,812 bytes in 10 blocks
==10307==         suppressed: 0 bytes in 0 blocks

Since we allocate things in GLib constructor (glib_init), I created a destructor (glib_deinit) : commit 1. I then cleaned up that memory in commits 2 and 3.

There was still memory leak even after clearing program name (fixed in commit 7). I found there was some private thread data and slice allocator data to free which is why I created commits 4, 5 and 6.

To sum it up :

g_set_prgname (NULL); in glib_deinit fixes loss record 2
g_error_deinit fixes loss record 4 6 8
g_quark_deinit fixes loss record 3 5 7 10
g_private_replace in g_slice_cleanup fixes loss record 9
g_private_cleanup in g_slice_cleanup fixes loss record 1

I tried updating glib.supp file. I initially wanted to remove parts related to thread leaks but I realized I only fixed main thread private data and user is still responsible to clean up things it allocates so I didn't remove them. This file is big I may have missed some entries

Concerning g_set_prgname, I'm not sure if calling it with NULL is a good idea. Ideally we would create a function named gutils_cleanup to cleanup all resources allocated in gutils.c, not just g_prgname. Well, 7 commits to merge is a good start, let's see what do you think and we can add more cleanup functions in follow-up merge requests.

Notes:

I tried cross-compiling GLib and my test-program but failed to do so, I don't know how to use pkg-config in such case. So I didn't implement private thread data cleanup for Windows target.
I'm marking this merge request as Draft because it introduces new public functions and I'm not sure that's desired
I'm not updating glib-sections.txt because I don't know if it's autogenerated or not

Best regards

Draft: Fix some GLib possible memory leaks

Merge request reports