rtpmanager/rtsession: data race leading to critical warnings

This is a fix for a data race leading to:

> GLib-CRITICAL: g_hash_table_foreach:
>   assertion 'version == hash_table->version' failed

Identified sequence:

* `rtp_session_on_timeout` acquires the lock on `session` and proceeds with its
  processing.
* `rtp_session_process_rtcp` is called (debug log : received RTCP packet) and
  attempts to acquire the lock on `session`, which is still held by
  `rtp_session_on_timeout`.
* as part of an hash table iterator, `rtp_session_on_timeout` transitively
  invokes `source_caps` which releases the lock on `session` so as to call
  `session->callbacks.caps`.
* Since `rtp_session_process_rtcp` was waiting for the lock to be released, it
  succeeds in acquiring it and proceeds with `rtp_session_process_rr` which
  transitively calls `g_hash_table_insert` via `add_source`.
* After `source_caps` re-acquires the lock and gives the control flow back to
  `rtp_session_on_timeout`, the hash table iterator is changed, resulting in the
  assertion failure.

This commits copies `sess->ssrcs[sess->mask_idx]` and iterates on the copy so
the iterator is not affected by a concurrent change due to the lock being
released in the `source_caps` callback.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4585>

diff --git a/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c b/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c
index d27933f..a73954e 100644 (file)
--- a/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c
+++ b/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c
@@ -4712,20 +4712,27 @@ rtp_session_on_timeout (RTPSession * sess, GstClockTime current_time,
   /* check if all the buffers are empty after generation */
   all_empty = TRUE;
 
+  /* Make a local copy of the hashtable. We need to do this because the
+   * generate_rtcp 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);
+
   GST_DEBUG
       ("doing RTCP generation %u for %u sources, early %d, may suppress %d",
       sess->generation, data.num_to_report, data.is_early, data.may_suppress);
 
-  /* generate RTCP for all internal sources */
-  g_hash_table_foreach (sess->ssrcs[sess->mask_idx],
-      (GHFunc) generate_rtcp, &data);
+  /* generate RTCP for all internal sources, this might release the
+   * session lock. */
+  g_hash_table_foreach (table_copy, (GHFunc) generate_rtcp, &data);
 
-  g_hash_table_foreach (sess->ssrcs[sess->mask_idx],
-      (GHFunc) generate_twcc, &data);
+  g_hash_table_foreach (table_copy, (GHFunc) generate_twcc, &data);
 
   /* update the generation for all the sources that have been reported */
-  g_hash_table_foreach (sess->ssrcs[sess->mask_idx],
-      (GHFunc) update_generation, &data);
+  g_hash_table_foreach (table_copy, (GHFunc) update_generation, &data);
+
+  g_hash_table_destroy (table_copy);
 
   /* we keep track of the last report time in order to timeout inactive
    * receivers or senders */