GNOME Bugzilla – Bug 385311
Memory leak initializing lame mp3 encoder plugin (gstlame.c)
Last modified: 2006-12-14 10:30:56 UTC
Please describe the problem: gst_lame_init() creates an encoder context so that it can retrieve default encoding parameters from LAME. However, it does not call lame_global_flags(), which is required according to the LAME API docs, see below. The effect of this is that LAME does not set the ClassID field of the global flags internal flags structure. lame_close() then presumes that the structure is invalid and does not free it; resulting in the memory leak below. Docs for lame_global_flags() from lame.h: /* * REQUIRED: * initialize the encoder. sets default for all encoder parameters, * returns NULL if some malloc()'s failed * otherwise returns pointer to structure needed for all future * API calls. */ lame_global_flags * CDECL lame_init(void); /* obsolete version */ int CDECL lame_init_old(lame_global_flags *); lame_close() definition: int lame_close(lame_global_flags * gfp) { lame_internal_flags *gfc = gfp->internal_flags; if (gfc->Class_ID != LAME_ID) <--- THIS EVALUATES TO FALSE. return -3; if (gfp->exp_nspsytune2.pointer[0]) { fclose((FILE *)gfp->exp_nspsytune2.pointer[0]); gfp->exp_nspsytune2.pointer[0] = NULL; } gfc->Class_ID = 0; /* this routien will free all malloc'd data in gfc, and then free gfc: */ freegfc(gfc); <--- SO THIS NEVER GETS CALLED. gfp->internal_flags = NULL; if (gfp->lame_allocated_gfp) { gfp->lame_allocated_gfp = 0; free(gfp); } return 0; } Steps to reproduce: 1. Play http://samples.mplayerhq.hu/real/AC-sipr/autahi-vox.rm in any app that uses GStreamer under valgrind. Actual results: ==26636== 148032 (516 direct, 147516 indirect) bytes in 1 blocks are definitely lost in loss record 22 of 23 ==26636== at 0x1B900B88: calloc (in /usr/lib/valgrind/vgpreload_memcheck.so) ==26636== by 0x1C1CE2F3: lame_init (in /usr/local/pkgs/lame-3.97/lib/libmp3lame.so.0.0.0) ==26636== by 0x1BF8E446: gst_lame_init (gstlame.c:522) ==26636== by 0x1B9CC9E5: g_type_create_instance (in /opt/gnome/lib/libgobject-2.0.so.0.800.1) ==26636== by 0x1B9B1E51: (within /opt/gnome/lib/libgobject-2.0.so.0.800.1) ==26636== by 0x1B9B2ADE: g_object_newv (in /opt/gnome/lib/libgobject-2.0.so.0.800.1) ==26636== by 0x1B9B3696: g_object_new_valist (in /opt/gnome/lib/libgobject-2.0.so.0.800.1) ==26636== by 0x1B9B384F: g_object_new (in /opt/gnome/lib/libgobject-2.0.so.0.800.1) ==26636== by 0x1B92FB52: gst_element_factory_create (gstelementfactory.c:381) ==26636== by 0x1B92FDA5: gst_element_factory_make (gstelementfactory.c:444) ==26636== by 0x804A02C: make_xc_pipeline_mp3 (gstxc.c:582) ==26636== by 0x804A2FF: mp3_perform (gstxc.c:682) ==26636== by 0x804993B: gstxc_perform (gstxc.c:329) ==26636== by 0x80494BF: main (xcfile.c:42) Expected results: Does this happen every time? Other information: One could take this up with the LAME developers. However, we are violating their API docs at the moment and the fix is trivial; see below.
Created attachment 78257 [details] [review] Corrective patch
On a related point, lame_init() can return NULL in the event that it cannot allocate required memory. Thus, it might be nice to add a call to g_error() similar to: g_error ("%s: failed to allocate %lu bytes", G_STRLOC, n_bytes); This would give an informative message rather than just seg faulting and would be more consistent with GStreamer's handling of out of memory conditions.
Created attachment 78274 [details] [review] updated patch, includes handling gst_lame_init() returning NULL Thanks for the patch, will go in after the freeze. > On a related point, lame_init() can return NULL in the event that it cannot > allocate required memory. Thus, it might be nice to add a call to g_error() > similar to: > > g_error ("%s: failed to allocate %lu bytes", G_STRLOC, n_bytes); > > This would give an informative message rather than just seg faulting and would > be more consistent with GStreamer's handling of out of memory conditions. I think it would be better to just return FALSE from gst_lame_setup() so that an error message is posted on the bus (whether that will succeed in a situation like that is a different question, but we can at least try) and set a flag if it fails in _init() so we can error out later.
OK. Just as long as it's handled; how is not so important.
2006-12-14 Tim-Philipp Müller <tim at centricular dot net> Based on patch by: Roland Kay <roland.kay at ox compsoc net> * ext/lame/gstlame.c: (gst_lame_init), (gst_lame_chain), (gst_lame_setup): * ext/lame/gstlame.h: Fix leak (by calling lame_init_params() before lame_close()); handle NULL return from lame_init() more gracefully. Fixes #385311.