GNOME Bugzilla – Bug 789358
playbin: Memory not freed in failure case in gst_playback_utils_get_n_common_capsfeatures()
Last modified: 2018-01-17 14:18:14 UTC
Possible memory leak is as below:- File: gstplaybackutils.c : Line No. 83 Function: gst_playback_utils_get_n_common_capsfeatures ... fact1_tmpl_caps = get_template_caps (fact1, GST_PAD_SRC); fact2_tmpl_caps = get_template_caps (fact2, GST_PAD_SINK); if (!fact1_tmpl_caps || !fact2_tmpl_caps) { GST_ERROR ("Failed to get template caps from decoder or sink"); return 0; } ... fact1_tmpl_caps and fact2_tmpl_caps must be freed before returning.
Created attachment 362112 [details] [review] [PATCH] Gst-plugins-base: Fix for possible memory leak in failure case Please review the patch attached to fix the issue and share the feedback.
Review of attachment 362112 [details] [review]: Somehow seems to make sense, but I didn't go further since there is issues in the submission. - Patches should be against git master - Title in this case should be in the form "playback-utils: Fix memory leak on failure" ::: gst-plugins-base-1.12.3/gst/playback/gstplaybackutils.c @@ +84,3 @@ GST_ERROR ("Failed to get template caps from decoder or sink"); + if (fact1_tmpl_caps) gst_caps_unref (fact1_tmpl_caps); + else if (fact2_tmpl_caps) gst_caps_unref (fact2_tmpl_caps); Wrong coding style, you need to place the unref on the following line, with 2 space indentation.
Created attachment 362606 [details] [review] [PATCH] playback-utils: Fix memory leak on failure Review comments implemented.
Both caps could be empty.
Yes, that is why the patch has if { } else if { } and not if { } else { }
nevermind, misread the code
Comment on attachment 362606 [details] [review] [PATCH] playback-utils: Fix memory leak on failure > fact1_tmpl_caps = get_template_caps (fact1, GST_PAD_SRC); > fact2_tmpl_caps = get_template_caps (fact2, GST_PAD_SINK); > > if (!fact1_tmpl_caps || !fact2_tmpl_caps) { > ... >+ if (fact1_tmpl_caps) >+ gst_caps_unref (fact1_tmpl_caps); >+ else if (fact2_tmpl_caps) >+ gst_caps_unref (fact2_tmpl_caps); > return 0; > } Why the else ? Why not just 2x if (caps) unref(caps) ?
Because control goes inside : if (!fact1_tmpl_caps || !fact2_tmpl_caps) { ... } only if at least one of 'fact1_tmpl_caps' and 'fact2_tmpl_caps' is missing. Inside if clause if 'if (fact1_tmpl_caps)' is true then 'if (fact2_tmpl_caps)' has to be false and vice versa. So we need not to check for both. If both fact1_tmpl_caps and fact2_tmpl_caps are defined the control will not go inside : if (!fact1_tmpl_caps || !fact2_tmpl_caps) { ... }
Yes, you're quite right. Pushed: commit b927d60371a14c968c9a8d19c577646427534074 (HEAD -> master) Author: Ashish Kumar <kr.ashish@samsung.com> Date: Tue Oct 31 15:04:47 2017 +0530 playback-utils: Fix caps leak on failure https://bugzilla.gnome.org/show_bug.cgi?id=789358