[Bug 760821] tracerrecord: Fix memory leaks and mishandlings

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=760821

--- Comment #15 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> ---
(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]
> > 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 :/

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.