GNOME Bugzilla – Bug 762843
gstinfo: Refactor code to remove memory leak and FIXME.
Last modified: 2016-06-20 10:44:33 UTC
In gst_debug_add_log_function, we are leaking old list. Refactor code a bit to fix the memory leak. This memory leak can be found when running gsttracerrecord or gstinfo tests using valgrind ==8192== 16 (8 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 706 of 1,487 ==8192== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==8192== by 0x423FBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==8192== by 0x4256281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==8192== by 0x425707A: g_slist_prepend (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==8192== by 0x40B8184: gst_debug_add_log_function (gstinfo.c:1188) ==8192== by 0x80498BC: cleanup (gsttracerrecord.c:62) ==8192== by 0x404EF7E: srunner_run_teardown (check_run.c:341) ==8192== by 0x404FF1C: tcase_run_checked_teardown (check_run.c:355) ==8192== by 0x404FF1C: tcase_run_tfun_fork (check_run.c:452) ==8192== by 0x404FF1C: srunner_iterate_tcase_tfuns (check_run.c:222) ==8192== by 0x404FF1C: srunner_run_tcase (check_run.c:362) ==8192== by 0x404FF1C: srunner_iterate_suites (check_run.c:195) ==8192== by 0x404FF1C: srunner_run (check_run.c:706) ==8192== by 0x404FFB2: srunner_run_all (check_run.c:674) ==8192== by 0x4042BD7: gst_check_run_suite (gstcheck.c:824) ==8192== by 0x8048F14: main (gsttracerrecord.c:169)
Created attachment 322619 [details] [review] gstinfo fix memory leak proposed patch to refactor code and remove memory leak
Review of attachment 322619 [details] [review]: This seems safe, but please add comments in the code and commit message *why* this is safe to do. Why we could never try to access freed memory here or run into pointers pointing to freed memory. ::: gst/gstinfo.c @@ +498,3 @@ G_VA_COPY (message.arguments, args); + handler = g_slist_copy (__log_functions); This would mean that the list is copied every single time you do some debug output, and then freed again. Doesn't seem ideal performance-wise :)
Clearly the leak here is intentional and for performance reasons. Either just leave it as is or you need to find a way that doesn't impact performance. Perhaps we should just add additional API to disable/re-enable an existing log function, then they don't have to be removed/re-added.
Marking as enhancement since it's an intentional leak.
No follow-up for 3 months, let's close this. Feel free to re-open if you find a better solution, thanks!