After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735348 - Memory leak in Acm components
Memory leak in Acm components
Status: RESOLVED WONTFIX
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal normal
: NONE
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-25 05:29 UTC by Vineeth
Modified: 2014-08-26 06:52 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
Patch for AcmEnc memory leak fix (895 bytes, patch)
2014-08-25 05:29 UTC, Vineeth 		none Details | Review
Patch for AcmMp3Dec memory leak (799 bytes, patch)
2014-08-25 05:30 UTC, Vineeth 		none Details | Review

Description Vineeth 2014-08-25 05:29:25 UTC 
On reviewing the code of AcmEnc.c and AcmMp3Dec.c
could see that there are possible memory leaks.

in *_setup functions, there are memory allocations for pbSrc and pbDst.
Now when there are no other errors, *->is_setup variable will be set to true.
Hence on calling setup again in *_sink_setcaps function, since is_setup variable is true it will teardown and call setup again. In this case there will be no leaks.

Now during *_setup call, if it fails to prepare ACM stream, the variable is_setup will not be set to true. Hence when we try to create the element, it will not teardown, and the previous allocations will not be freed.

Hence i added free functions when ACM stream fails to prepare.

And i feel it is always better to free memory in failure cases in the same function, rather than waiting for teardown to free them.
Comment 1 Vineeth 2014-08-25 05:29:56 UTC 
Created attachment 284370 [details] [review]
Patch for AcmEnc memory leak fix
Comment 2 Vineeth 2014-08-25 05:30:33 UTC 
Created attachment 284371 [details] [review]
Patch for AcmMp3Dec memory leak
Comment 3 Thiago Sousa Santos 2014-08-25 19:26:03 UTC 
FYI acm plugins were not ported to 1.0 and I'm not sure if it is worth porting them as DirectShow should be the recommended framework instead of ACM or VCM for Windows.
Comment 4 Vineeth 2014-08-26 04:23:11 UTC 
Hi Thiago,

 Yes i came to know that ACM is not ported to 1.0. Since i dont have a windows setup, i didnt try to port as well.

 But i just went through the code and found these leaks. Thought this might help anyone in future who tries to port the same.


PS: I am new to GStreamer, and this was the first element i looked into :)..

Regards,
Vineeth
Comment 5 Tim-Philipp Müller 2014-08-26 06:52:04 UTC 
Thanks for filing the bug and patch, but we don't usually modify source code of plugins which haven't been ported and which can't be compiled and tested. Best to leave them alone. The code will likely be rewritten quite a bit when it gets ported.