GNOME Bugzilla – Bug 766025
rtpsession: race condition accessing ssrcs hash table
Last modified: 2016-06-27 06:27:14 UTC
Hello, I have noticed that there may be a race condition when managing SSRCs hash table in the RtpSession object. Specifically, I have found the problem iterating the hash table when the stats are created for dumping the object params: GLib-CRITICAL **: g_hash_table_foreach: assertion 'version == hash_table->version' failed Analysing the GDB[1] output we can see that the problem is in function [2]. Refs [1] (gdb) bt
+ Trace 236230
[2] static GstStructure * rtp_session_create_stats (RTPSession * sess) { GstStructure *s; GValueArray *source_stats; GValue source_stats_v = G_VALUE_INIT; guint size; s = gst_structure_new ("application/x-rtp-session-stats", "rtx-drop-count", G_TYPE_UINT, sess->stats.nacks_dropped, "sent-nack-count", G_TYPE_UINT, sess->stats.nacks_sent, "recv-nack-count", G_TYPE_UINT, sess->stats.nacks_received, NULL); size = g_hash_table_size (sess->ssrcs[sess->mask_idx]); source_stats = g_value_array_new (size); g_hash_table_foreach (sess->ssrcs[sess->mask_idx], (GHFunc) create_source_stats, source_stats); g_value_init (&source_stats_v, G_TYPE_VALUE_ARRAY); g_value_take_boxed (&source_stats_v, source_stats); gst_structure_take_value (s, "source-stats", &source_stats_v); return s; }
Created attachment 327346 [details] [review] rtpsession: use critical section for creating stats
commit 2e960e70750a0cb7e1117d0c09d08597866a29ee Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Thu May 5 14:18:21 2016 +0200 rtpsession: Take session lock when creating stats The access to the session hash table must happen while the session lock is taken, otherwise another thread might modify the hash table while we're creating the stats. https://bugzilla.gnome.org/show_bug.cgi?id=766025
I'll backport to 1.8 some time later
This caused a deadlock commit 204a86af97d039ddc71cc61bb8bf8a30827c1db1 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed May 11 09:28:13 2016 +0300 rtpsession: Don't notify about stats property changes while taking the session lock The signal handlers might want to actually get the value of the stats property, which would take the session lock again and deadlock. This was introduced by 2e960e70750a0cb7e1117d0c09d08597866a29ee. https://bugzilla.gnome.org/show_bug.cgi?id=766025
Good point @slomo ;), sorry but I didn't realize about this possible deadlock. How did you notice it? In addition, the link of the commit is broken, so people won't see it from here.
Yes, GNOME bugzilla assumes that all commits are in their GIT :) I noticed it by running a pipeline with an rtpsession somewhere (rtpbin, rtspsrc, ...) with -v in gst-launch. The deep-notify::stats signal is printed by gst-launch and causes this.
OK, thanks for the explanation!!
commit fe34f46f32db15b62b445e82d3c7191f682ddf8b Author: Sebastian Dröge <sebastian@centricular.com> Date: Sun May 15 12:30:50 2016 +0300 rtpsession: Take the lock already when reading the other stats, not just for the hash table https://bugzilla.gnome.org/show_bug.cgi?id=766025
Hello @slomo, I though about this, but the point is that these other stats are modified without locking the sess lock, so this patch makes not sense. We should modify these values into the critical section too, or using atomic counters for simplicity. What do you think?
These should also be modified with the lock then. Can you prepare a patch? Using atomic counters does not really help, I think it's generally useful to get a consistent snapshot of all the stats and not a version of the stats where some values are modified already and some are the old ones.
Miguel?
Created attachment 328360 [details] [review] rtpsession: take the lock when changing stats
Review of attachment 328360 [details] [review]: ::: gst/rtpmanager/rtpsession.c @@ +2673,3 @@ GstClockTime current_time) { + RTP_SESSION_LOCK (sess); I didn't look at the other parts, but this one is deadlocking: rtp_session_process_nack() is called by rtp_session_process_feedback(), which is called by rtp_session_process_rtcp() with the lock being held
Created attachment 328371 [details] [review] rtpsession: take the lock when changing stats
Attachment 328371 [details] pushed as 83f4c08 - rtpsession: take the lock when changing stats