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 789358 - playbin: Memory not freed in failure case in gst_playback_utils_get_n_common_capsfeatures()
playbin: Memory not freed in failure case in gst_playback_utils_get_n_common_...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.12.3
Other All
: Normal minor
: 1.12.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-10-23 16:27 UTC by Ashish Kumar
Modified: 2018-01-17 14:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] Gst-plugins-base: Fix for possible memory leak in failure case (1.02 KB, patch)
2017-10-23 16:28 UTC, Ashish Kumar
none Details | Review
[PATCH] playback-utils: Fix memory leak on failure (920 bytes, patch)
2017-10-31 09:42 UTC, Ashish Kumar
committed Details | Review

Description Ashish Kumar 2017-10-23 16:27:19 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.
Comment 1 Ashish Kumar 2017-10-23 16:28:38 UTC
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.
Comment 2 Nicolas Dufresne (ndufresne) 2017-10-30 19:23:07 UTC
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.
Comment 3 Ashish Kumar 2017-10-31 09:42:18 UTC
Created attachment 362606 [details] [review]
[PATCH] playback-utils: Fix memory leak on failure

Review comments implemented.
Comment 4 Edward Hervey 2017-10-31 10:44:31 UTC
Both caps could be empty.
Comment 5 Ashish Kumar 2017-10-31 10:53:25 UTC
Yes, that is why the patch has
if {
}
else if {
}

and not 

if { 
}
else {
}
Comment 6 Edward Hervey 2017-10-31 11:06:14 UTC
nevermind, misread the code
Comment 7 Tim-Philipp Müller 2018-01-12 12:11:34 UTC
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) ?
Comment 8 Ashish Kumar 2018-01-17 11:59:56 UTC
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) {
...
}
Comment 9 Tim-Philipp Müller 2018-01-17 12:21:18 UTC
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