rtpbin: Make cleaning up sources in rtp_session_on_timeout MT safe

Using _foreach_remove on the hashtable, while releasing the lock protecting
that table inside the callback is not a good idea. The hashtable might
then change (a source removed or added) while signals like on_timeout
are being sent.

This solution makes a copy of the table, performs the _foreach without
actually removing any sources, but marks them for removal on a second
iteration with the real list, but this time not letting go of the lock.

Fixes #630452

diff --git a/gst/rtpmanager/rtpsession.c b/gst/rtpmanager/rtpsession.c
index 59b16db..e763a71 100644 (file)
--- a/gst/rtpmanager/rtpsession.c
+++ b/gst/rtpmanager/rtpsession.c
@@ -2360,7 +2360,7 @@ session_report_blocks (const gchar * key, RTPSource * source, ReportData * data)
 }
 
 /* perform cleanup of sources that timed out */
-static gboolean
+static void
 session_cleanup (const gchar * key, RTPSource * source, ReportData * data)
 {
   gboolean remove = FALSE;
@@ -2428,7 +2428,8 @@ session_cleanup (const gchar * key, RTPSource * source, ReportData * data)
     if (sendertimeout)
       on_sender_timeout (sess, source);
   }
-  return remove;
+
+  source->closing = remove;
 }
 
 static void
@@ -2556,6 +2557,18 @@ is_rtcp_time (RTPSession * sess, GstClockTime current_time, ReportData * data)
   return result;
 }
 
+static void
+clone_ssrcs_hashtable (gchar * key, RTPSource * source, GHashTable * hash_table)
+{
+  g_hash_table_insert (hash_table, key, g_object_ref (source));
+}
+
+static gboolean
+remove_closing_sources (const gchar * key, RTPSource * source, gpointer * data)
+{
+  return source->closing;
+}
+
 /**
  * rtp_session_on_timeout:
  * @sess: an #RTPSession
@@ -2581,6 +2594,7 @@ rtp_session_on_timeout (RTPSession * sess, GstClockTime current_time,
   GstFlowReturn result = GST_FLOW_OK;
   ReportData data;
   RTPSource *own;
+  GHashTable *table_copy;
   gboolean notify = FALSE;
 
   g_return_val_if_fail (RTP_IS_SESSION (sess), GST_FLOW_ERROR);
@@ -2602,9 +2616,21 @@ rtp_session_on_timeout (RTPSession * sess, GstClockTime current_time,
   /* get a new interval, we need this for various cleanups etc */
   data.interval = calculate_rtcp_interval (sess, TRUE, sess->first_rtcp);
 
-  /* first perform cleanups */
+  /* Make a local copy of the hashtable. We need to do this because the
+   * cleanup stage below releases the session lock. */
+  table_copy = g_hash_table_new_full (NULL, NULL, NULL,
+      (GDestroyNotify) g_object_unref);
+  g_hash_table_foreach (sess->ssrcs[sess->mask_idx],
+      (GHFunc) clone_ssrcs_hashtable, table_copy);
+
+  /* Clean up the session, mark the source for removing, this might release the
+   * session lock. */
+  g_hash_table_foreach (table_copy, (GHFunc) session_cleanup, &data);
+  g_hash_table_destroy (table_copy);
+
+  /* Now remove the marked sources */
   g_hash_table_foreach_remove (sess->ssrcs[sess->mask_idx],
-      (GHRFunc) session_cleanup, &data);
+      (GHRFunc) remove_closing_sources, NULL);
 
   /* see if we need to generate SR or RR packets */
   if (is_rtcp_time (sess, current_time, &data)) {

diff --git a/gst/rtpmanager/rtpsource.c b/gst/rtpmanager/rtpsource.c
index b142d60..e0432b7 100644 (file)
--- a/gst/rtpmanager/rtpsource.c
+++ b/gst/rtpmanager/rtpsource.c
@@ -158,6 +158,7 @@ rtp_source_init (RTPSource * src)
   src->validated = FALSE;
   src->internal = FALSE;
   src->probation = RTP_DEFAULT_PROBATION;
+  src->closing = FALSE;
 
   src->sdes = gst_structure_new ("application/x-rtp-source-sdes", NULL);
 

diff --git a/gst/rtpmanager/rtpsource.h b/gst/rtpmanager/rtpsource.h
index 49c6528..54a86ac 100644 (file)
--- a/gst/rtpmanager/rtpsource.h
+++ b/gst/rtpmanager/rtpsource.h
@@ -131,6 +131,7 @@ struct _RTPSource {
   gboolean      internal;
   gboolean      is_csrc;
   gboolean      is_sender;
+  gboolean      closing;
 
   GstStructure  *sdes;