GNOME Bugzilla – Bug 760821
tracerrecord: Fix memory leaks and mishandlings
Last modified: 2016-01-22 09:10:15 UTC
There are few memory leaks and mishandling in gsttracerrecord and the tests. Handling those in this bug.
Created attachment 319322 [details] [review] Fix spec structure invalid free. self->spec is got using g_value_get_boxed(), which does not transfer the memory and hence should not be freed. This is resulting in the below errors. ==24380== Invalid read of size 4 ==24380== at 0x40EDFCE: gst_structure_free (gststructure.c:376) ==24380== by 0x40FFC01: gst_tracer_record_dispose (gsttracerrecord.c:137) ==24380== by 0x41B2237: g_object_unref (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x8049794: serialize_message_logging (gsttracerrecord.c:94) ==24380== by 0x404FB6A: srunner_run (check_run.c:450) ==24380== by 0x404FC22: srunner_run_all (check_run.c:674) ==24380== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==24380== by 0x8048F14: main (gsttracerrecord.c:184) ==24380== Address 0x45fb608 is 8 bytes inside a block of size 16 free'd ==24380== at 0x402D3D8: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==24380== by 0x423ED2F: g_free (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==24380== by 0x425587A: g_slice_free1 (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==24380== by 0x40EE0E2: gst_structure_free (gststructure.c:392) ==24380== by 0x41D1D60: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x41AAD9D: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x41D3BF5: g_value_unset (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x41B2D15: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x41B4D96: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x41B4FEF: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==24380== by 0x40FFC55: gst_tracer_record_new (gsttracerrecord.c:227) ==24380== by 0x804970D: serialize_message_logging (gsttracerrecord.c:82) ==24380== by 0x404FB6A: srunner_run (check_run.c:450) ==24380== by 0x404FC22: srunner_run_all (check_run.c:674) ==24380== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==24380== by 0x8048F14: main (gsttracerrecord.c:184)
Created attachment 319323 [details] [review] initialize flags to avoid wrong comparision. since the flags is not initalized, while running tracerrecord tests, get the below error ==21050== Conditional jump or move depends on uninitialised value(s) ==21050== at 0x40FF75B: build_field_template (gsttracerrecord.c:86) ==21050== by 0x40F0BCA: gst_structure_foreach (gststructure.c:1126) ==21050== by 0x40FFA37: gst_tracer_record_set_property (gsttracerrecord.c:124) ==21050== by 0x41B2D97: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==21050== by 0x41B4D96: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==21050== by 0x41B4FEF: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==21050== by 0x40FFC45: gst_tracer_record_new (gsttracerrecord.c:227) ==21050== by 0x804970D: serialize_message_logging (gsttracerrecord.c:82) ==21050== by 0x404FB6A: srunner_run (check_run.c:450) ==21050== by 0x404FC22: srunner_run_all (check_run.c:674) ==21050== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==21050== by 0x8048F14: main (gsttracerrecord.c:184)
Created attachment 319324 [details] [review] tests: fix memory leaks Fix for below memory leaks tracer_record ==1272== 166 (64 direct, 102 indirect) bytes in 1 blocks are definitely lost in loss record 1,475 of 1,503 ==1272== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==1272== by 0x423EBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x4255281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x425579C: g_slice_alloc0 (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x41CFACD: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B28FD: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B4D96: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B4FEF: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x40FFC45: gst_tracer_record_new (gsttracerrecord.c:226) ==1272== by 0x8049706: serialize_message_logging (gsttracerrecord.c:78) ==1272== by 0x404FB6A: srunner_run (check_run.c:450) ==1272== by 0x404FC22: srunner_run_all (check_run.c:674) ==1272== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==1272== by 0x8048F14: main (gsttracerrecord.c:167) structures ==408== 152 (16 direct, 136 indirect) bytes in 1 blocks are definitely lost in loss record 1,471 of 1,499 ==408== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==408== by 0x423EBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==408== by 0x4255281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==408== by 0x40EDCE1: gst_structure_new_id_empty_with_size (gststructure.c:145) ==408== by 0x40F020B: gst_structure_new_valist (gststructure.c:281) ==408== by 0x40F026A: gst_structure_new (gststructure.c:253) ==408== by 0x804968F: serialize_message_logging (gsttracerrecord.c:75) ==408== by 0x404FB6A: srunner_run (check_run.c:450) ==408== by 0x404FC22: srunner_run_all (check_run.c:674) ==408== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==408== by 0x8048F14: main (gsttracerrecord.c:166) str_val string ==1436== 5 bytes in 1 blocks are definitely lost in loss record 39 of 1,495 ==1436== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==1436== by 0x423EBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1436== by 0x4256E1C: g_strdup (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1436== by 0x41D73C7: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1436== by 0x40F3189: gst_structure_get_valist (gststructure.c:3032) ==1436== by 0x40F37A7: gst_structure_get (gststructure.c:3161) ==1436== by 0x80492E9: serialize_static_record (gsttracerrecord.c:128) ==1436== by 0x404FB6A: srunner_run (check_run.c:450) ==1436== by 0x404FC22: srunner_run_all (check_run.c:674) ==1436== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==1436== by 0x8048F14: main (gsttracerrecord.c:167)
Created attachment 319325 [details] [review] tests: Fix messages glist memory leak
I hadn't seen this bug thread in my email inbox and fixed one of the small issues pointed out by Coverity. Sorry. commit 227c387b43b8a1f8c2de2b5b8a600b08d29c3408 Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Tue Jan 19 12:04:16 2016 +0000 tracerrecord: avoid overwriting value res value is overwritten, remove the assignment. priv__gst_structure_append_template_to_gstring () always returns TRUE anyway. CID 1349645
Review of attachment 319323 [details] [review]: Thanks! I've slightly modified this to also initialialize the GType, so that we get a warning if the "type" field was missing in the structure.
For the structure, I'd like to avoid the need to manually free them. This makes the code like here: http://cgit.freedesktop.org/gstreamer/gstreamer/tree/plugins/tracers/gstlatency.c#n204 quite ugly. We have a simillar case in: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/playback/gstdecodebin2.c#n4409 Maybe I should revert https://bug760821.bugzilla-attachments.gnome.org/attachment.cgi?id=319322 and actually use g_value_take_boxed() and when freeing the structure, also free all sub structures recursively. Any opinions?
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #7) > For the structure, I'd like to avoid the need to manually free them. This > makes the code like here: > http://cgit.freedesktop.org/gstreamer/gstreamer/tree/plugins/tracers/ > gstlatency.c#n204 > quite ugly. > > We have a simillar case in: > http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/playback/ > gstdecodebin2.c#n4409 > > Maybe I should revert > https://bug760821.bugzilla-attachments.gnome.org/attachment.cgi?id=319322 > and actually use g_value_take_boxed() and when freeing the structure, also > free all sub structures recursively. Any opinions? It might work out.. I am not exactly sure how get the memory location of the sub structures to free the same. Maybe there is a way. Other maintainers should be able to answer this. In the meantime i will split the other 2 leaks, so that we can work on this seperately
Created attachment 319472 [details] [review] Fix tracer record leak tracer_record ==1272== 166 (64 direct, 102 indirect) bytes in 1 blocks are definitely lost in loss record 1,475 of 1,503 ==1272== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==1272== by 0x423EBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x4255281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x425579C: g_slice_alloc0 (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1272== by 0x41CFACD: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B28FD: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B4D96: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x41B4FEF: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1272== by 0x40FFC45: gst_tracer_record_new (gsttracerrecord.c:226) ==1272== by 0x8049706: serialize_message_logging (gsttracerrecord.c:78) ==1272== by 0x404FB6A: srunner_run (check_run.c:450) ==1272== by 0x404FC22: srunner_run_all (check_run.c:674) ==1272== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==1272== by 0x8048F14: main (gsttracerrecord.c:167)
Created attachment 319473 [details] [review] fix str_val string leak str_val string ==1436== 5 bytes in 1 blocks are definitely lost in loss record 39 of 1,495 ==1436== at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==1436== by 0x423EBE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1436== by 0x4256E1C: g_strdup (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==1436== by 0x41D73C7: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4002.0) ==1436== by 0x40F3189: gst_structure_get_valist (gststructure.c:3032) ==1436== by 0x40F37A7: gst_structure_get (gststructure.c:3161) ==1436== by 0x80492E9: serialize_static_record (gsttracerrecord.c:128) ==1436== by 0x404FB6A: srunner_run (check_run.c:450) ==1436== by 0x404FC22: srunner_run_all (check_run.c:674) ==1436== by 0x4042A57: gst_check_run_suite (gstcheck.c:824) ==1436== by 0x8048F14: main (gsttracerrecord.c:167)
Created attachment 319484 [details] [review] free sub structures I am a bit puzzled. I can 'fix' the leaks on the test (see patch), but if I try to do the same on the gsttracerrecord (see code in the patch that is #if 0'ed) then valgrind complains about illegal memory reads.
Vineeth, I push already a small patch that already frees the tracerrecord objects and the string: commit a72368ebb388c22ff68a084a12b09327d857b34c
How should we deal with this one: ==6293== 16 bytes in 1 blocks are definitely lost in loss record 584 of 1,490 ==6293== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==6293== by 0x5619610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x562F22D: g_slice_alloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x5630005: g_slist_prepend (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) ==6293== by 0x50B93A7: _priv_gst_debug_init (gstinfo.c:324) ==6293== by 0x50816B1: init_pre.part.2 (gst.c:484) ==6293== by 0x50818B4: init_pre (gst.c:534) ==6293== by 0x561EACF: g_option_context_parse (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x5081E8E: gst_init_check (gst.c:354) ==6293== by 0x5081ED6: gst_init (gst.c:400) ==6293== by 0x4E40642: gst_check_init (gstcheck.c:127) ==6293== by 0x40133D: main (gsttracerrecord.c:187) ==6293== ==6293== 32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost in loss record 821 of 1,490 ==6293== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==6293== by 0x5619610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x562F22D: g_slice_alloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x5630005: g_slist_prepend (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) ==6293== by 0x401CEE: setup (gsttracerrecord.c:52) ==6293== by 0x4E4BA27: srunner_run_setup (check_run.c:288) ==6293== by 0x4E4C537: tcase_run_checked_setup (check_run.c:317) ==6293== by 0x4E4C537: tcase_run_tfun_fork (check_run.c:447) ==6293== by 0x4E4C537: srunner_iterate_tcase_tfuns (check_run.c:222) ==6293== by 0x4E4C537: srunner_run_tcase (check_run.c:362) ==6293== by 0x4E4C537: srunner_iterate_suites (check_run.c:195) ==6293== by 0x4E4C537: srunner_run (check_run.c:706) ==6293== by 0x4E4177D: gst_check_run_suite (gstcheck.c:824) ==6293== by 0x4013E4: main (gsttracerrecord.c:187) ==6293== add a suppression? The gstinfo test suffers from the same problem though (make gst/gstinfo.valgrind).
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #12) > Vineeth, I push already a small patch that already frees the tracerrecord > objects and the string: commit a72368ebb388c22ff68a084a12b09327d857b34c okies (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #11) > Created attachment 319484 [details] [review] [review] > free sub structures > > I am a bit puzzled. I can 'fix' the leaks on the test (see patch), but if I > try to do the same on the gsttracerrecord (see code in the patch that is #if > 0'ed) then valgrind complains about illegal memory reads. I think the problem here is usage of self->spec = g_value_get_boxed (value); get_boxed is actually transfer none. So we should not be freeing the memory acquired using it. This is leading to the illegal memory access i guess. I am not sure if there is any other option to be used here. g_value_take_boxed does not accept const value g_value_dup_boxed creates duplicate memory. So freeing it wont free the actual memory.. (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #13) > How should we deal with this one: > ==6293== 16 bytes in 1 blocks are definitely lost in loss record 584 of 1,490 > ==6293== at 0x4C2AB80: malloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6293== by 0x5619610: g_malloc (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x562F22D: g_slice_alloc (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x5630005: g_slist_prepend (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) > ==6293== by 0x50B93A7: _priv_gst_debug_init (gstinfo.c:324) > ==6293== by 0x50816B1: init_pre.part.2 (gst.c:484) > ==6293== by 0x50818B4: init_pre (gst.c:534) > ==6293== by 0x561EACF: g_option_context_parse (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x5081E8E: gst_init_check (gst.c:354) > ==6293== by 0x5081ED6: gst_init (gst.c:400) > ==6293== by 0x4E40642: gst_check_init (gstcheck.c:127) > ==6293== by 0x40133D: main (gsttracerrecord.c:187) > ==6293== > ==6293== 32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost > in loss record 821 of 1,490 > ==6293== at 0x4C2AB80: malloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6293== by 0x5619610: g_malloc (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x562F22D: g_slice_alloc (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x5630005: g_slist_prepend (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) > ==6293== by 0x401CEE: setup (gsttracerrecord.c:52) > ==6293== by 0x4E4BA27: srunner_run_setup (check_run.c:288) > ==6293== by 0x4E4C537: tcase_run_checked_setup (check_run.c:317) > ==6293== by 0x4E4C537: tcase_run_tfun_fork (check_run.c:447) > ==6293== by 0x4E4C537: srunner_iterate_tcase_tfuns (check_run.c:222) > ==6293== by 0x4E4C537: srunner_run_tcase (check_run.c:362) > ==6293== by 0x4E4C537: srunner_iterate_suites (check_run.c:195) > ==6293== by 0x4E4C537: srunner_run (check_run.c:706) > ==6293== by 0x4E4177D: gst_check_run_suite (gstcheck.c:824) > ==6293== by 0x4013E4: main (gsttracerrecord.c:187) > ==6293== > > add a suppression? The gstinfo test suffers from the same problem though > (make gst/gstinfo.valgrind). Adding a suppression for the time being.. There is a FIXME in gstinfo.c.. /* FIXME: we leak the old list here - other threads might access it right now * in gst_debug_logv. Another solution is to lock the mutex in gst_debug_logv, * but that is waaay costly. * It'd probably be clever to use some kind of RCU here, but I don't know * anything about that. */ Maybe fixing this will remove this leak. Did not go through it completely yet.
(In reply to Vineeth from comment #14) > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #12) > > Vineeth, I push already a small patch that already frees the tracerrecord > > objects and the string: commit a72368ebb388c22ff68a084a12b09327d857b34c > > okies > > > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #11) > > Created attachment 319484 [details] [review] [review] [review] > > free sub structures > > > > I am a bit puzzled. I can 'fix' the leaks on the test (see patch), but if I > > try to do the same on the gsttracerrecord (see code in the patch that is #if > > 0'ed) then valgrind complains about illegal memory reads. > > I think the problem here is > usage of > self->spec = g_value_get_boxed (value); > > get_boxed is actually transfer none. So we should not be freeing the memory > acquired using it. This is leading to the illegal memory access i guess. > > I am not sure if there is any other option to be used here. > g_value_take_boxed does not accept const value > g_value_dup_boxed creates duplicate memory. So freeing it wont free the > actual memory.. > Since this is construct-only we can document that we take-ownership. As you say g_value_dup_boxed() won't help and g_value_take_boxed() is actually the opposite (belongs to g_value_set_boxed(), g_value_set_static_boxed() ). We'd need to able to say to g_object_new() that the structure should be passed via g_value_take_boxed(), but I don't see any way. I'll did a bit further ... > > > (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #13) > > How should we deal with this one: > > ==6293== 16 bytes in 1 blocks are definitely lost in loss record 584 of 1,490 > > ==6293== at 0x4C2AB80: malloc (in > > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > ==6293== by 0x5619610: g_malloc (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x562F22D: g_slice_alloc (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x5630005: g_slist_prepend (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) > > ==6293== by 0x50B93A7: _priv_gst_debug_init (gstinfo.c:324) > > ==6293== by 0x50816B1: init_pre.part.2 (gst.c:484) > > ==6293== by 0x50818B4: init_pre (gst.c:534) > > ==6293== by 0x561EACF: g_option_context_parse (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x5081E8E: gst_init_check (gst.c:354) > > ==6293== by 0x5081ED6: gst_init (gst.c:400) > > ==6293== by 0x4E40642: gst_check_init (gstcheck.c:127) > > ==6293== by 0x40133D: main (gsttracerrecord.c:187) > > ==6293== > > ==6293== 32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost > > in loss record 821 of 1,490 > > ==6293== at 0x4C2AB80: malloc (in > > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > ==6293== by 0x5619610: g_malloc (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x562F22D: g_slice_alloc (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x5630005: g_slist_prepend (in > > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > > ==6293== by 0x50B8796: gst_debug_add_log_function (gstinfo.c:1188) > > ==6293== by 0x401CEE: setup (gsttracerrecord.c:52) > > ==6293== by 0x4E4BA27: srunner_run_setup (check_run.c:288) > > ==6293== by 0x4E4C537: tcase_run_checked_setup (check_run.c:317) > > ==6293== by 0x4E4C537: tcase_run_tfun_fork (check_run.c:447) > > ==6293== by 0x4E4C537: srunner_iterate_tcase_tfuns (check_run.c:222) > > ==6293== by 0x4E4C537: srunner_run_tcase (check_run.c:362) > > ==6293== by 0x4E4C537: srunner_iterate_suites (check_run.c:195) > > ==6293== by 0x4E4C537: srunner_run (check_run.c:706) > > ==6293== by 0x4E4177D: gst_check_run_suite (gstcheck.c:824) > > ==6293== by 0x4013E4: main (gsttracerrecord.c:187) > > ==6293== > > > > add a suppression? The gstinfo test suffers from the same problem though > > (make gst/gstinfo.valgrind). > > Adding a suppression for the time being.. > There is a FIXME in gstinfo.c.. > > /* FIXME: we leak the old list here - other threads might access it right > now > * in gst_debug_logv. Another solution is to lock the mutex in > gst_debug_logv, > * but that is waaay costly. > * It'd probably be clever to use some kind of RCU here, but I don't know > * anything about that. > */ > Maybe fixing this will remove this leak. Did not go through it completely > yet. Yes, I know, but that seems to be not trivial :/
Something is completely borked here: http://fpaste.org/313348/14533997/ I can 'fix' it on the top-level, by changing the param_spec from boxed to pointer, but that won't work for nested structures :/
Problem is that this code is wrong and is bound/expected to leak: gst_structure_new ("test.class", "string", GST_TYPE_STRUCTURE, gst_structure_new (...), NULL); just like this would leak: gst_structure_new ("test.class", "string", G_TYPE_STRING, g_strdup ("hello"), NULL); When the varargs are collected, copies are made and refs are taken by the collector, so you need to free the newly-allocated struct here.
It would certainly be possible to write a gst_structure_new_and_take() function that takes ownership of structures passed though (by using G_VALUE_COLLECT* with the G_VALUE_NOCOPY_CONTENTS flag). You could also change gst_tracer_record_new() to this: gst_tracer_record_new (const gchar * classname, const gchar *first_field, ...) and then make it collect things like GstStructure but taking ownership.
I like the gst_tracer_record_new (const gchar * classname, const gchar *first_field, ...) suggestion, will definitely do this. Then I can drop the property. Lets see if it also allows me to fix the nested structure issue.
Created attachment 319528 [details] [review] don't leak the sub structures
Created attachment 319529 [details] [review] don't leak the sub structures
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #21) > Created attachment 319529 [details] [review] [review] > don't leak the sub structures Tested this and it works well...
Comment on attachment 319529 [details] [review] don't leak the sub structures >Subject: [PATCH] tracerrecord: don't leak the spec structures > >Change the gst_tracer_record_new() api to take the parameters the make the >spec structure directly. This allows us to own the top-level structure and >also collect the args so that we can take ownership of the sub-structures. > >Fixes #760821 For what it's worth, these days we just put the full bug url to the bug there :) >+ while (firstfield) { >+ GValue val = { 0, }; >+ >+ id = g_quark_from_string (firstfield); >+ type = va_arg (varargs, GType); >+ >+ G_VALUE_COLLECT_INIT (&val, type, varargs, G_VALUE_NOCOPY_CONTENTS, &err); >+ if (G_UNLIKELY (err)) { >+ g_critical ("%s", err); >+ break; >+ } >+ gst_structure_id_set_value (structure, id, &val); >+ gst_structure_free (g_value_get_boxed (&val)); >+ g_value_unset (&val); >+ >+ firstfield = va_arg (varargs, gchar *); >+ } >+ } gst_structure_id_set_value() + g_value_unset() could be simplified to gst_structure_id_take_value(). We'd also need to clear the G_VALUE_NOCOPY_CONTENTS flag from value.data[1].v_uint then, so the struct gets freed properly with the value later, and the gst_structure_free() is not required anymore then.
Clever! All suggested changes applied and pushed: commit ec75b68984d5ae57722791ea97d292b862e149cd Author: Stefan Sauer <ensonic@users.sf.net> Date: Thu Jan 21 08:12:01 2016 +0100 tracerrecord: don't leak the spec structures Change the gst_tracer_record_new() api to take the parameters the make the spec structure directly. This allows us to own the top-level structure and also collect the args so that we can take ownership of the sub-structures. https://bugzilla.gnome.org/show_bug.cgi?id=760821
Comment on attachment 319529 [details] [review] don't leak the sub structures Pushed with small mods as sugested.