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 776334 - Memory leak fixed in usage of gst_element_get_request_pad() API
Memory leak fixed in usage of gst_element_get_request_pad() API
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.9.1
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-21 04:40 UTC by Garima
Modified: 2018-11-03 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Memory leak fixed in usage of gst_element_get_request() API (2.03 KB, patch)
2016-12-21 04:40 UTC, Garima
none Details | Review
Memory leak fixed in usage of gst_element_get_request_pad() API (1.23 KB, patch)
2016-12-21 08:21 UTC, Garima
committed Details | Review

Description Garima 2016-12-21 04:40:44 UTC
Created attachment 342296 [details] [review]
Memory leak fixed in usage of gst_element_get_request() API

Memory leak fixed in usage of gst_element_get_request() API.

gst_element_get_request() returns requested GstPad which need to be released after its usage.

Changes done in following file:
ext/resindvd/resindvdbin.c 
gst/sdp/gstsdpdemux.c 
sys/dvb/dvbbasebin.c

Added gst_object_unref() to release the memory.

Find the attached patch.
Comment 1 Sebastian Dröge (slomo) 2016-12-21 08:02:15 UTC
Review of attachment 342296 [details] [review]:

::: ext/resindvd/resindvdbin.c
@@ +719,3 @@
 
+  if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK) {
+    gst_object_unref (mq_sink);

This looks wrong. The pad is stored above in the list, and that's probably what should own the reference. And later when the list is emptied (and the pads are released), the unref should happen
Comment 2 Garima 2016-12-21 08:21:54 UTC
Created attachment 342307 [details] [review]
Memory leak fixed in usage of gst_element_get_request_pad() API

Yes. I understood your point. I have attached the latest patch. Please review the patch.
Comment 3 Sebastian Dröge (slomo) 2016-12-21 08:28:49 UTC
Comment on attachment 342307 [details] [review]
Memory leak fixed in usage of gst_element_get_request_pad() API

commit 0fdd4e2539cc8c2c794419b13594ed7761d5fccd
Author: Garima Gaur <garima.g@samsung.com>
Date:   Wed Dec 21 13:41:16 2016 +0530

    gst: Fix memory leaks in usage of gst_element_get_request_pad() API
    
    The return value has to be unreffed at some point.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776334
Comment 4 Sebastian Dröge (slomo) 2016-12-21 08:29:39 UTC
What you found in resindvd is actually a leak, as said above, you would have to unref the pads when the list is emptied. Which currently does not happen.
Comment 5 Sebastian Dröge (slomo) 2016-12-22 11:02:50 UTC
Are you planning to provide a new patch for that?
Comment 6 Garima 2016-12-22 11:18:54 UTC
Do you mean that following fix for resindvd was correct in earlier patch:

--- a/ext/resindvd/resindvdbin.c
+++ b/ext/resindvd/resindvdbin.c
@@ -717,8 +717,10 @@ connect_thru_mq (RsnDvdBin * dvdbin, GstPad * pad)
     return FALSE;
   dvdbin->mq_req_pads = g_list_prepend (dvdbin->mq_req_pads, mq_sink);
 
-  if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK)
+  if (gst_pad_link (pad, mq_sink) != GST_PAD_LINK_OK) {
+    gst_object_unref (mq_sink);
     return FALSE;
+  }
 
   sinkname = gst_pad_get_name (mq_sink);
   tmp = sinkname + 5;
@@ -727,6 +729,7 @@ connect_thru_mq (RsnDvdBin * dvdbin, GstPad * pad)
   mq_src = gst_element_get_static_pad (dvdbin->pieces[DVD_ELEM_MQUEUE],
       srcname);
 
+  gst_object_unref (mq_sink);
   g_free (sinkname);
   g_free (srcname);
Comment 7 Sebastian Dröge (slomo) 2016-12-22 11:22:44 UTC
No it wasn't. But the pads will have to be unreffed later after they are released from the multiqueue, and that part is missing.
Comment 8 Garima 2016-12-22 11:47:13 UTC
I understood. But I not found the point where to release the memory for mq_sink. Please consider my patch for other 2 changes. Thanks.
Comment 9 Sebastian Dröge (slomo) 2016-12-22 12:03:20 UTC
That's done here: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/resindvd/resindvdbin.c#n660

Just search for "gst_element_release_request_pad". As part of that loop, it should also unref the pads.

(In reply to Garima from comment #8)
> Please consider my patch for other 2 changes. Thanks.

The other two are merged already.
Comment 10 GStreamer system administrator 2018-11-03 14:02:15 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/499.