rtpsession: don't act on suspicious BYE RTCP

Some endpoints (like Tandberg E20) can send BYE packet containing our
internal SSRC. I this case we would detect SSRC collision and get rid
of the source at some point. But because we are still sending packets
with that SSRC the source will be recreated immediately.
This brand new internal source will not have some variables incorrectly
set in its state. For example 'seqnum-base` and `clock-rate` values will be
-1.
The fix is not to act on BYE RTCP if it contains internal or unknown
SSRC.

https://bugzilla.gnome.org/show_bug.cgi?id=762219

3 files changed, 115 insertions, 16 deletions

diff --git a/gst/rtpmanager/rtpsession.c b/gst/rtpmanager/rtpsession.c
index ae34d67c1..620d623d2 100644
--- a/gst/rtpmanager/rtpsession.c
+++ b/gst/rtpmanager/rtpsession.c
@@ -2443,21 +2443,18 @@ rtp_session_process_bye (RTPSession * sess, GstRTCPPacket * packet,
   for (i = 0; i < count; i++) {
     guint32 ssrc;
     RTPSource *source;
-    gboolean created, prevactive, prevsender;
+    gboolean prevactive, prevsender;
     guint pmembers, members;
 
     ssrc = gst_rtcp_packet_bye_get_nth_ssrc (packet, i);
     GST_DEBUG ("SSRC: %08x", ssrc);
 
     /* find src and mark bye, no probation when dealing with RTCP */
-    source = obtain_source (sess, ssrc, &created, pinfo, FALSE);
-    if (!source)
-      return;
-
-    if (source->internal) {
-      /* our own source, something weird with this packet */
-      g_object_unref (source);
-      continue;
+    source = find_source (sess, ssrc);
+    if (!source || source->internal) {
+      GST_DEBUG ("Ignoring suspicious BYE packet (reason: %s)",
+          !source ? "can't find source" : "has internal source SSRC");
+      break;
     }
 
     /* store time for when we need to time out this source */
@@ -2515,12 +2512,7 @@ rtp_session_process_bye (RTPSession * sess, GstRTCPPacket * packet,
       }
     }
 
-    if (created)
-      on_new_ssrc (sess, source);
-
     on_bye_ssrc (sess, source);
-
-    g_object_unref (source);
   }
   if (reconsider) {
     RTP_SESSION_UNLOCK (sess);
@@ -2529,6 +2521,7 @@ rtp_session_process_bye (RTPSession * sess, GstRTCPPacket * packet,
       sess->callbacks.reconsider (sess, sess->reconsider_user_data);
     RTP_SESSION_LOCK (sess);
   }
+
   g_free (reason);
 }
 
diff --git a/tests/check/Makefile.am b/tests/check/Makefile.am
index bc78f9fc5..3aec98ce9 100644
--- a/tests/check/Makefile.am
+++ b/tests/check/Makefile.am
@@ -561,7 +561,7 @@ elements_rtprtx_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(CFLAGS) $(AM_CFLAGS)
 elements_rtprtx_LDADD = $(GST_PLUGINS_BASE_LIBS) -lgstrtp-$(GST_API_VERSION) $(LDADD)
 
 elements_rtpsession_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(CFLAGS) $(AM_CFLAGS)
-elements_rtpsession_LDADD = $(GST_PLUGINS_BASE_LIBS) -lgstrtp-$(GST_API_VERSION) $(LDADD)
+elements_rtpsession_LDADD = $(GST_PLUGINS_BASE_LIBS) -lgstnet-$(GST_API_VERSION) -lgstrtp-$(GST_API_VERSION) $(GIO_LIBS) $(LDADD)
 
 elements_rtpcollision_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(CFLAGS) $(AM_CFLAGS)
 elements_rtpcollision_LDADD = $(GST_PLUGINS_BASE_LIBS) -lgstnet-$(GST_API_VERSION) -lgstrtp-$(GST_API_VERSION) $(GIO_LIBS) $(LDADD)
diff --git a/tests/check/elements/rtpsession.c b/tests/check/elements/rtpsession.c
index 2f1955437..faeb653cc 100644
--- a/tests/check/elements/rtpsession.c
+++ b/tests/check/elements/rtpsession.c
@@ -20,6 +20,7 @@
  * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
+#define GLIB_DISABLE_DEPRECATION_WARNINGS
 
 #include <gst/check/gstharness.h>
 #include <gst/check/gstcheck.h>
@@ -27,6 +28,7 @@
 
 #include <gst/rtp/gstrtpbuffer.h>
 #include <gst/rtp/gstrtcpbuffer.h>
+#include <gst/net/gstnetaddressmeta.h>
 
 static const guint payload_size = 160;
 static const guint clock_rate = 8000;
@@ -702,6 +704,109 @@ GST_START_TEST (test_dont_lock_on_stats)
 }
 GST_END_TEST;
 
+static void
+suspicious_bye_cb (GObject * object, GParamSpec * spec, gpointer data)
+{
+  GValueArray *stats_arr;
+  GstStructure *stats, *internal_stats;
+  gboolean *cb_called = data;
+  gboolean internal = FALSE, sent_bye = TRUE;
+  guint ssrc = 0;
+  guint i;
+
+  g_assert (*cb_called == FALSE);
+  *cb_called = TRUE;
+
+  g_object_get (object, "stats", &stats, NULL);
+  stats_arr =
+      g_value_get_boxed (gst_structure_get_value (stats, "source-stats"));
+  g_assert (stats_arr != NULL);
+  fail_unless (stats_arr->n_values >= 1);
+
+  for (i = 0; i < stats_arr->n_values; i++) {
+    internal_stats = g_value_get_boxed (g_value_array_get_nth (stats_arr, i));
+    g_assert (internal_stats != NULL);
+
+    gst_structure_get (internal_stats,
+        "ssrc", G_TYPE_UINT, &ssrc,
+        "internal", G_TYPE_BOOLEAN, &internal,
+        "received-bye", G_TYPE_BOOLEAN, &sent_bye, NULL);
+
+    if (ssrc == 0xDEADBEEF) {
+      fail_unless (internal);
+      fail_unless (!sent_bye);
+      break;
+    }
+  }
+  fail_unless_equals_int (ssrc, 0xDEADBEEF);
+
+  gst_structure_free (stats);
+}
+
+static GstBuffer *
+create_bye_rtcp (guint32 ssrc)
+{
+  GstRTCPPacket packet;
+  GstRTCPBuffer rtcp = GST_RTCP_BUFFER_INIT;
+  GSocketAddress *saddr;
+  GstBuffer *buffer = gst_rtcp_buffer_new (1000);
+
+  fail_unless (gst_rtcp_buffer_map (buffer, GST_MAP_READWRITE, &rtcp));
+  fail_unless (gst_rtcp_buffer_add_packet (&rtcp, GST_RTCP_TYPE_BYE, &packet));
+  gst_rtcp_packet_bye_add_ssrc (&packet, ssrc);
+  gst_rtcp_buffer_unmap (&rtcp);
+
+  /* Need to add meta to trigger collision detection */
+  saddr = g_inet_socket_address_new_from_string ("127.0.0.1", 3490);
+  gst_buffer_add_net_address_meta (buffer, saddr);
+  g_object_unref (saddr);
+  return buffer;
+}
+
+GST_START_TEST (test_ignore_suspicious_bye)
+{
+  GstHarness *h_rtcp = NULL;
+  GstHarness *h_send = NULL;
+  gboolean cb_called = FALSE;
+  GstTestClock *testclock = GST_TEST_CLOCK (gst_test_clock_new ());
+
+  /* use testclock as the systemclock to capture the rtcp thread waits */
+  gst_system_clock_set_default (GST_CLOCK (testclock));
+
+  h_rtcp =
+      gst_harness_new_with_padnames ("rtpsession", "recv_rtcp_sink",
+      "send_rtcp_src");
+  h_send =
+      gst_harness_new_with_element (h_rtcp->element, "send_rtp_sink",
+      "send_rtp_src");
+
+  /* connect to the stats-reporting */
+  g_signal_connect (h_rtcp->element, "notify::stats",
+      G_CALLBACK (suspicious_bye_cb), &cb_called);
+
+  /* Push RTP buffer making our internal SSRC=0xDEADBEEF */
+  gst_harness_set_src_caps_str (h_send,
+      "application/x-rtp,ssrc=(uint)0xDEADBEEF,"
+      "clock-rate=90000,seqnum-offset=(uint)12345");
+  gst_harness_push (h_send,
+      generate_test_buffer (0, FALSE, 12345, 0, 0xDEADBEEF));
+
+  /* Push BYE RTCP with internal SSRC (0xDEADBEEF) */
+  gst_harness_set_src_caps_str (h_rtcp, "application/x-rtcp");
+  gst_harness_push (h_rtcp, create_bye_rtcp (0xDEADBEEF));
+
+  /* "crank" and check the stats */
+  g_assert (gst_test_clock_crank (testclock));
+  gst_buffer_unref (gst_harness_pull (h_rtcp));
+  fail_unless (cb_called);
+
+  gst_harness_teardown (h_send);
+  gst_harness_teardown (h_rtcp);
+  gst_object_unref (testclock);
+}
+
+GST_END_TEST;
+
 static Suite *
 rtpsession_suite (void)
 {
@@ -713,8 +818,9 @@ rtpsession_suite (void)
   tcase_add_test (tc_chain, test_multiple_senders_roundrobin_rbs);
   tcase_add_test (tc_chain, test_internal_sources_timeout);
   tcase_add_test (tc_chain, test_receive_rtcp_app_packet);
-
   tcase_add_test (tc_chain, test_dont_lock_on_stats);
+  tcase_add_test (tc_chain, test_ignore_suspicious_bye);
+
   return s;
 }