From 6675ed9aaebd6e7b973bc09fb2aca5c01b829649 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Laignel?= Date: Tue, 9 May 2023 17:28:49 +0200 Subject: 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: --- .../gst-plugins-good/gst/rtpmanager/rtpsession.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c b/subprojects/gst-plugins-good/gst/rtpmanager/rtpsession.c index 7dc5eb6106..a6fb73d2d3 100644 --- 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 */ -- cgit v1.2.1