From a3933ea53d2a11ed40c288122a2eb065d5ce7069 Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Fri, 3 Apr 2020 00:16:10 +1100 Subject: [PATCH] flvmux: Fix invalid padlist accesses. Request pads can released at any time, so make sure to hold the object lock when iterating the element sinkpads list where that's safe, or to use other safe pad iteration patterns in other places. When choosing a best pad, return a reference to the pad to make sure it stays alive for output in the aggregator srcpad task. Should fix a spurious valgrind error in the CI flvmux tests and some other potential problems if the request sink pads are released while the element is running.. Fixes https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/714 --- gst/flv/gstflvmux.c | 134 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 97 insertions(+), 37 deletions(-) diff --git a/gst/flv/gstflvmux.c b/gst/flv/gstflvmux.c index 9678205..11ee8b6 100644 --- a/gst/flv/gstflvmux.c +++ b/gst/flv/gstflvmux.c @@ -131,6 +131,7 @@ static GstFlowReturn gst_flv_mux_rewrite_header (GstFlvMux * mux); static gboolean gst_flv_mux_are_all_pads_eos (GstFlvMux * mux); static GstFlowReturn gst_flv_mux_update_src_caps (GstAggregator * aggregator, GstCaps * caps, GstCaps ** ret); +static guint64 gst_flv_mux_query_upstream_duration (GstFlvMux * mux); static GstFlowReturn gst_flv_mux_pad_flush (GstAggregatorPad * pad, GstAggregator * aggregator) @@ -946,20 +947,7 @@ gst_flv_mux_create_metadata (GstFlvMux * mux) } if (!mux->streamable && mux->duration == GST_CLOCK_TIME_NONE) { - GList *l; - guint64 dur; - - for (l = GST_ELEMENT_CAST (mux)->sinkpads; l; l = l->next) { - GstFlvMuxPad *pad = GST_FLV_MUX_PAD (l->data); - - if (gst_pad_peer_query_duration (GST_PAD (pad), GST_FORMAT_TIME, - (gint64 *) & dur) && dur != GST_CLOCK_TIME_NONE) { - if (mux->duration == GST_CLOCK_TIME_NONE) - mux->duration = dur; - else - mux->duration = MAX (dur, mux->duration); - } - } + mux->duration = gst_flv_mux_query_upstream_duration (mux); } if (!mux->streamable && mux->duration != GST_CLOCK_TIME_NONE) { @@ -1386,6 +1374,7 @@ gst_flv_mux_prepare_src_caps (GstFlvMux * mux, GstBuffer ** header_buf, video_codec_data = NULL; audio_codec_data = NULL; + GST_OBJECT_LOCK (mux); for (l = GST_ELEMENT_CAST (mux)->sinkpads; l != NULL; l = l->next) { GstFlvMuxPad *pad = l->data; @@ -1406,6 +1395,7 @@ gst_flv_mux_prepare_src_caps (GstFlvMux * mux, GstBuffer ** header_buf, gst_flv_mux_codec_data_buffer_to_tag (mux, pad->codec_data, pad); } } + GST_OBJECT_UNLOCK (mux); /* mark buffers that will go in the streamheader */ GST_BUFFER_FLAG_SET (header, GST_BUFFER_FLAG_HEADER); @@ -1628,6 +1618,7 @@ gst_flv_mux_determine_duration (GstFlvMux * mux) GST_DEBUG_OBJECT (mux, "trying to determine the duration " "from pad timestamps"); + GST_OBJECT_LOCK (mux); for (l = GST_ELEMENT_CAST (mux)->sinkpads; l != NULL; l = l->next) { GstFlvMuxPad *pad = GST_FLV_MUX_PAD (l->data); @@ -1638,21 +1629,60 @@ gst_flv_mux_determine_duration (GstFlvMux * mux) duration = MAX (duration, pad->last_timestamp); } } + GST_OBJECT_UNLOCK (mux); return duration; } +struct DurationData +{ + GstClockTime duration; +}; + +static gboolean +duration_query_cb (GstElement * element, GstPad * pad, + struct DurationData *data) +{ + guint64 dur; + + if (gst_pad_peer_query_duration (GST_PAD (pad), GST_FORMAT_TIME, + (gint64 *) & dur) && dur != GST_CLOCK_TIME_NONE) { + if (data->duration == GST_CLOCK_TIME_NONE) + data->duration = dur; + else + data->duration = MAX (dur, data->duration); + } + + return TRUE; +} + +static guint64 +gst_flv_mux_query_upstream_duration (GstFlvMux * mux) +{ + struct DurationData cb_data = { GST_CLOCK_TIME_NONE }; + + gst_element_foreach_sink_pad (GST_ELEMENT (mux), + (GstElementForeachPadFunc) (duration_query_cb), &cb_data); + + return cb_data.duration; +} + + static gboolean gst_flv_mux_are_all_pads_eos (GstFlvMux * mux) { GList *l; + GST_OBJECT_LOCK (mux); for (l = GST_ELEMENT_CAST (mux)->sinkpads; l; l = l->next) { GstFlvMuxPad *pad = GST_FLV_MUX_PAD (l->data); - if (!gst_aggregator_pad_is_eos (GST_AGGREGATOR_PAD (pad))) + if (!gst_aggregator_pad_is_eos (GST_AGGREGATOR_PAD (pad))) { + GST_OBJECT_UNLOCK (mux); return FALSE; + } } + GST_OBJECT_UNLOCK (mux); return TRUE; } @@ -1807,35 +1837,62 @@ gst_flv_mux_rewrite_header (GstFlvMux * mux) return gst_flv_mux_push (mux, rewrite); } +/* Returns NULL, or a reference to the pad with the + * buffer with lowest running time */ static GstFlvMuxPad * gst_flv_mux_find_best_pad (GstAggregator * aggregator, GstClockTime * ts) { - GstAggregatorPad *apad; - GstFlvMuxPad *pad, *best = NULL; - GList *l; + GstFlvMuxPad *best = NULL; GstBuffer *buffer; GstClockTime best_ts = GST_CLOCK_TIME_NONE; - - for (l = GST_ELEMENT_CAST (aggregator)->sinkpads; l; l = l->next) { - apad = GST_AGGREGATOR_PAD (l->data); - pad = GST_FLV_MUX_PAD (l->data); - buffer = gst_aggregator_pad_peek_buffer (GST_AGGREGATOR_PAD (pad)); - if (!buffer) - continue; - if (best_ts == GST_CLOCK_TIME_NONE) { - best = pad; - best_ts = gst_flv_mux_segment_to_running_time (&apad->segment, - GST_BUFFER_DTS_OR_PTS (buffer)); - } else if (GST_BUFFER_DTS_OR_PTS (buffer) != GST_CLOCK_TIME_NONE) { - gint64 t = gst_flv_mux_segment_to_running_time (&apad->segment, - GST_BUFFER_DTS_OR_PTS (buffer)); - if (t < best_ts) { - best = pad; - best_ts = t; + GstIterator *pads; + GValue padptr = { 0, }; + gboolean done = FALSE; + + pads = gst_element_iterate_sink_pads (GST_ELEMENT (aggregator)); + + while (!done) { + switch (gst_iterator_next (pads, &padptr)) { + case GST_ITERATOR_OK:{ + + GstAggregatorPad *apad = g_value_get_object (&padptr); + buffer = gst_aggregator_pad_peek_buffer (apad); + if (!buffer) + continue; + if (best_ts == GST_CLOCK_TIME_NONE) { + gst_object_replace ((GstObject **) & best, GST_OBJECT (apad)); + best_ts = gst_flv_mux_segment_to_running_time (&apad->segment, + GST_BUFFER_DTS_OR_PTS (buffer)); + } else if (GST_BUFFER_DTS_OR_PTS (buffer) != GST_CLOCK_TIME_NONE) { + gint64 t = gst_flv_mux_segment_to_running_time (&apad->segment, + GST_BUFFER_DTS_OR_PTS (buffer)); + if (t < best_ts) { + gst_object_replace ((GstObject **) & best, GST_OBJECT (apad)); + best_ts = t; + } + } + gst_buffer_unref (buffer); + g_value_reset (&padptr); + break; } + case GST_ITERATOR_DONE: + done = TRUE; + break; + case GST_ITERATOR_RESYNC: + gst_iterator_resync (pads); + /* Clear the best pad and start again. It might have disappeared */ + gst_object_replace ((GstObject **) best, NULL); + best_ts = GST_CLOCK_TIME_NONE; + break; + case GST_ITERATOR_ERROR: + /* This can't happen if the parameters to gst_iterator_next() are valid */ + g_assert_not_reached (); + break; } - gst_buffer_unref (buffer); } + g_value_unset (&padptr); + gst_iterator_free (pads); + GST_DEBUG_OBJECT (aggregator, "Best pad found with %" GST_TIME_FORMAT ": %" GST_PTR_FORMAT, GST_TIME_ARGS (best_ts), best); @@ -1919,11 +1976,14 @@ gst_flv_mux_aggregate (GstAggregator * aggregator, gboolean timeout) gst_buffer_unref (buffer); buffer = NULL; } + gst_object_unref (best); best = NULL; } if (best) { - return gst_flv_mux_write_buffer (mux, best, buffer); + GstFlowReturn ret = gst_flv_mux_write_buffer (mux, best, buffer); + gst_object_unref (best); + return ret; } else { if (gst_flv_mux_are_all_pads_eos (mux)) { gst_flv_mux_write_eos (mux); -- 2.7.4