After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766025 - rtpsession: race condition accessing ssrcs hash table
rtpsession: race condition accessing ssrcs hash table
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.8.2
Other Linux
: Normal normal
: 1.8.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-05 12:05 UTC by Miguel París Díaz
Modified: 2016-06-27 06:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtpsession: use critical section for creating stats (1.12 KB, patch)
2016-05-05 12:35 UTC, Miguel París Díaz
committed Details | Review
rtpsession: take the lock when changing stats (1.67 KB, patch)
2016-05-22 19:41 UTC, Miguel París Díaz
none Details | Review
rtpsession: take the lock when changing stats (1.35 KB, patch)
2016-05-23 08:19 UTC, Miguel París Díaz
committed Details | Review

Description Miguel París Díaz 2016-05-05 12:05:48 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
  • #0 g_logv
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./glib/gmessages.c line 324
  • #1 g_logv
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./glib/gmessages.c line 1081
  • #2 g_log
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./glib/gmessages.c line 1119
  • #3 g_return_if_fail_warning
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./glib/gmessages.c line 1128
  • #4 rtp_session_get_property
    at rtpsession.c line 736
  • #5 rtp_session_get_property
    at rtpsession.c line 899
  • #6 g_object_get_valist
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./gobject/gobject.c line 1375
  • #7 g_object_get_valist
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./gobject/gobject.c line 2230
  • #8 g_object_get
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./gobject/gobject.c line 2320
  • #9 gst_rtp_session_get_property
    at gstrtpsession.c line 1008
  • #10 gst_rtp_session_get_property
    at gstrtpsession.c line 986
  • #11 g_object_get_property
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./gobject/gobject.c line 1375
  • #12 g_object_get_property
    at /build/glib2.0-ajuDY6/glib2.0-2.46.1/./gobject/gobject.c line 2442
  • #13 debug_dump_get_object_params
    at gstdebugutils.c line 133

[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;
}
Comment 1 Miguel París Díaz 2016-05-05 12:35:10 UTC
Created attachment 327346 [details] [review]
rtpsession: use critical section for creating stats
Comment 2 Sebastian Dröge (slomo) 2016-05-06 06:25:21 UTC
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
Comment 3 Sebastian Dröge (slomo) 2016-05-06 06:29:20 UTC
I'll backport to 1.8 some time later
Comment 4 Sebastian Dröge (slomo) 2016-05-11 06:29:34 UTC
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
Comment 5 Miguel París Díaz 2016-05-11 07:52:40 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2016-05-11 08:20:12 UTC
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.
Comment 7 Miguel París Díaz 2016-05-12 09:01:45 UTC
OK,
thanks for the explanation!!
Comment 8 Sebastian Dröge (slomo) 2016-05-15 09:37:01 UTC
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
Comment 9 Miguel París Díaz 2016-05-16 13:40:42 UTC
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?
Comment 10 Sebastian Dröge (slomo) 2016-05-17 06:45:15 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2016-05-20 06:10:30 UTC
Miguel?
Comment 12 Miguel París Díaz 2016-05-22 19:41:10 UTC
Created attachment 328360 [details] [review]
rtpsession: take the lock when changing stats
Comment 13 Sebastian Dröge (slomo) 2016-05-23 07:08:42 UTC
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
Comment 14 Miguel París Díaz 2016-05-23 08:19:43 UTC
Created attachment 328371 [details] [review]
rtpsession: take the lock when changing stats
Comment 15 Sebastian Dröge (slomo) 2016-06-17 09:53:05 UTC
Attachment 328371 [details] pushed as 83f4c08 - rtpsession: take the lock when changing stats