From 4f73d3cf800359b4a30abd4b68e0a880397bd116 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Tim-Philipp=20M=C3=BCller?= Date: Sun, 24 Mar 2019 20:45:03 +0000 Subject: [PATCH] decodebin2: fix pad leak and problem with GWeakRef code Follow-up to !160 and commit 6a99ad2c. Firstly, don't leak the sinkpad. g_weak_ref_get() returns a strong reference (unless it returns NULL), so that must be unrefed, as detected by the playbin-complex and discoverer unit tests. Next, if we do that we get invalid memory access when the final pad ref is dropped a few lines below after the request pad is released. The reason for this is that GWeakRefs are not movable once they're in use, because their address will be stored inside the object. In this case the GWeakRef was embedded inside the GstDemuxerPad struct which in turn was embedded inside the GArray data section, and when the GArray gets resized, the structs move. Just KISS and use a list with individual allocations for each DemuxerPad instead. --- gst/playback/gstdecodebin2.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/gst/playback/gstdecodebin2.c b/gst/playback/gstdecodebin2.c index 4f558ed..38ef398 100644 --- a/gst/playback/gstdecodebin2.c +++ b/gst/playback/gstdecodebin2.c @@ -446,7 +446,7 @@ struct _GstDecodeGroup gboolean drained; /* TRUE if the all children are drained */ GList *children; /* List of GstDecodeChains in this group */ - GArray *demuxer_pad_probe_ids; + GList *demuxer_pad_probe_ids; GList *reqpads; /* List of RequestPads for multiqueue, there is * exactly one RequestPad per child chain */ @@ -3571,25 +3571,26 @@ multi_queue_overrun_cb (GstElement * queue, GstDecodeGroup * group) static void gst_decode_group_free_internal (GstDecodeGroup * group, gboolean hide) { - gint i; GList *l; GST_DEBUG_OBJECT (group->dbin, "%s group %p", (hide ? "Hiding" : "Freeing"), group); if (!hide) { - for (i = 0; i < group->demuxer_pad_probe_ids->len; i++) { - GstDemuxerPad *demuxer_pad = - &g_array_index (group->demuxer_pad_probe_ids, GstDemuxerPad, i); + for (l = group->demuxer_pad_probe_ids; l != NULL; l = l->next) { + GstDemuxerPad *demuxer_pad = l->data; GstPad *sinkpad = g_weak_ref_get (&demuxer_pad->weakPad); - if (!sinkpad) - continue; - - gst_pad_remove_probe (sinkpad, demuxer_pad->event_probe_id); - gst_pad_remove_probe (sinkpad, demuxer_pad->query_probe_id); + if (sinkpad != NULL) { + gst_pad_remove_probe (sinkpad, demuxer_pad->event_probe_id); + gst_pad_remove_probe (sinkpad, demuxer_pad->query_probe_id); + g_weak_ref_clear (&demuxer_pad->weakPad); + gst_object_unref (sinkpad); + } + g_free (demuxer_pad); } - g_array_unref (group->demuxer_pad_probe_ids); + g_list_free (group->demuxer_pad_probe_ids); + group->demuxer_pad_probe_ids = NULL; } for (l = group->children; l; l = l->next) { @@ -3843,8 +3844,7 @@ gst_decode_group_new (GstDecodeBin * dbin, GstDecodeChain * parent) group->overrunsig = g_signal_connect (mq, "overrun", G_CALLBACK (multi_queue_overrun_cb), group); - group->demuxer_pad_probe_ids = - g_array_new (FALSE, TRUE, sizeof (GstDemuxerPad)); + group->demuxer_pad_probe_ids = NULL; gst_element_set_state (mq, GST_STATE_PAUSED); gst_bin_add (GST_BIN (dbin), gst_object_ref (mq)); @@ -3910,18 +3910,18 @@ gst_decode_group_control_demuxer_pad (GstDecodeGroup * group, GstPad * pad) } CHAIN_MUTEX_LOCK (group->parent); - g_array_set_size (group->demuxer_pad_probe_ids, - group->demuxer_pad_probe_ids->len + 1); - demuxer_pad = - &g_array_index (group->demuxer_pad_probe_ids, GstDemuxerPad, - group->demuxer_pad_probe_ids->len - 1); + /* Note: GWeakRefs can't be moved in memory once they're in use, so do a + * dedicated alloc for the GstDemuxerPad struct that contains it */ + demuxer_pad = g_new0 (GstDemuxerPad, 1); demuxer_pad->event_probe_id = gst_pad_add_probe (sinkpad, GST_PAD_PROBE_TYPE_EVENT_UPSTREAM, sink_pad_event_probe, group, NULL); demuxer_pad->query_probe_id = gst_pad_add_probe (sinkpad, GST_PAD_PROBE_TYPE_QUERY_UPSTREAM, sink_pad_query_probe, group, NULL); g_weak_ref_set (&demuxer_pad->weakPad, sinkpad); + group->demuxer_pad_probe_ids = + g_list_prepend (group->demuxer_pad_probe_ids, demuxer_pad); group->reqpads = g_list_prepend (group->reqpads, gst_object_ref (sinkpad)); CHAIN_MUTEX_UNLOCK (group->parent); -- 2.7.4