rtprtxreceive: fix crash when RTX payload has zero length

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

subprojects/gst-plugins-good/gst/rtpmanager/gstrtprtxreceive.c

@@ -473,12 +473,12 @@ _gst_rtp_buffer_new_from_rtx (GstRTPBuffer * rtp, guint32 ssrc1,
   }
 
   /* copy payload and remove OSN */
+  g_assert_cmpint (rtp->size[2], >, 1);
   payload_len = rtp->size[2] - 2;
   mem = gst_allocator_alloc (NULL, payload_len, NULL);
 
   gst_memory_map (mem, &map, GST_MAP_WRITE);
-  if (rtp->size[2])
+  memcpy (map.data, (guint8 *) rtp->data[2] + 2, payload_len);
-    memcpy (map.data, (guint8 *) rtp->data[2] + 2, payload_len);
   gst_memory_unmap (mem, &map);
   gst_buffer_append_memory (new_buffer, mem);
 
@@ -582,67 +582,76 @@ gst_rtp_rtx_receive_chain (GstPad * pad, GstObject * parent, GstBuffer * buffer)
     /* increase our statistic */
     ++rtx->num_rtx_packets;
 
-    /* read OSN in the rtx payload */
+    /* check if there enough data to read OSN from the paylaod,
-    orign_seqnum = GST_READ_UINT16_BE (gst_rtp_buffer_get_payload (&rtp));
+       we need at least two bytes
-    origin_payload_type =
+     */
-        GPOINTER_TO_UINT (g_hash_table_lookup (rtx->rtx_pt_map,
+    if (gst_rtp_buffer_get_payload_len (&rtp) > 1) {
-            GUINT_TO_POINTER (payload_type)));
+      /* read OSN in the rtx payload */
+      orign_seqnum = GST_READ_UINT16_BE (gst_rtp_buffer_get_payload (&rtp));
+      origin_payload_type =
+          GPOINTER_TO_UINT (g_hash_table_lookup (rtx->rtx_pt_map,
+              GUINT_TO_POINTER (payload_type)));
 
-    GST_DEBUG_OBJECT (rtx, "Got rtx packet: rtx seqnum %u, rtx ssrc %X, "
+      GST_DEBUG_OBJECT (rtx, "Got rtx packet: rtx seqnum %u, rtx ssrc %X, "
-        "rtx pt %u, orig seqnum %u, orig pt %u", seqnum, ssrc, payload_type,
+          "rtx pt %u, orig seqnum %u, orig pt %u", seqnum, ssrc, payload_type,
-        orign_seqnum, origin_payload_type);
+          orign_seqnum, origin_payload_type);
 
-    /* first we check if we already have associated this retransmission stream
+      /* first we check if we already have associated this retransmission stream
-     * to a master stream */
+       * to a master stream */
-    if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map,
+      if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map,
-            GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) {
+              GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) {
-      GST_TRACE_OBJECT (rtx,
+        GST_TRACE_OBJECT (rtx,
-          "packet is from retransmission stream %X already associated to "
+            "packet is from retransmission stream %X already associated to "
-          "master stream %X", ssrc, GPOINTER_TO_UINT (ssrc1));
+            "master stream %X", ssrc, GPOINTER_TO_UINT (ssrc1));
-      ssrc2 = ssrc;
-    } else {
-      SsrcAssoc *assoc;
-
-      /* the current retransmitted packet has its rtx stream not already
-       * associated to a master stream, so retrieve it from our request
-       * history */
-      if (g_hash_table_lookup_extended (rtx->seqnum_ssrc1_map,
-              GUINT_TO_POINTER (orign_seqnum), NULL, (gpointer *) & assoc)) {
-        GST_LOG_OBJECT (rtx,
-            "associating retransmitted stream %X to master stream %X thanks "
-            "to rtx packet %u (orig seqnum %u)", ssrc, assoc->ssrc, seqnum,
-            orign_seqnum);
-        ssrc1 = GUINT_TO_POINTER (assoc->ssrc);
         ssrc2 = ssrc;
-
-        /* just put a guard */
-        if (GPOINTER_TO_UINT (ssrc1) == ssrc2)
-          GST_WARNING_OBJECT (rtx, "RTX receiver ssrc2_ssrc1_map bad state, "
-              "master and rtx SSRCs are the same (%X)\n", ssrc);
-
-        /* free the spot so that this seqnum can be used to do another
-         * association */
-        g_hash_table_remove (rtx->seqnum_ssrc1_map,
-            GUINT_TO_POINTER (orign_seqnum));
-
-        /* actually do the association between rtx stream and master stream */
-        g_hash_table_insert (rtx->ssrc2_ssrc1_map, GUINT_TO_POINTER (ssrc2),
-            ssrc1);
-
-        /* also do the association between master stream and rtx stream
-         * every ssrc are unique so we can use the same hash table
-         * for both retrieving the ssrc1 from ssrc2 and also ssrc2 from ssrc1
-         */
-        g_hash_table_insert (rtx->ssrc2_ssrc1_map, ssrc1,
-            GUINT_TO_POINTER (ssrc2));
-
       } else {
-        /* we are not able to associate this rtx packet with a master stream */
+        SsrcAssoc *assoc;
-        GST_INFO_OBJECT (rtx,
+
-            "dropping rtx packet %u because its orig seqnum (%u) is not in our"
+        /* the current retransmitted packet has its rtx stream not already
-            " pending retransmission requests", seqnum, orign_seqnum);
+         * associated to a master stream, so retrieve it from our request
-        drop = TRUE;
+         * history */
+        if (g_hash_table_lookup_extended (rtx->seqnum_ssrc1_map,
+                GUINT_TO_POINTER (orign_seqnum), NULL, (gpointer *) & assoc)) {
+          GST_LOG_OBJECT (rtx,
+              "associating retransmitted stream %X to master stream %X thanks "
+              "to rtx packet %u (orig seqnum %u)", ssrc, assoc->ssrc, seqnum,
+              orign_seqnum);
+          ssrc1 = GUINT_TO_POINTER (assoc->ssrc);
+          ssrc2 = ssrc;
+
+          /* just put a guard */
+          if (GPOINTER_TO_UINT (ssrc1) == ssrc2)
+            GST_WARNING_OBJECT (rtx, "RTX receiver ssrc2_ssrc1_map bad state, "
+                "master and rtx SSRCs are the same (%X)\n", ssrc);
+
+          /* free the spot so that this seqnum can be used to do another
+           * association */
+          g_hash_table_remove (rtx->seqnum_ssrc1_map,
+              GUINT_TO_POINTER (orign_seqnum));
+
+          /* actually do the association between rtx stream and master stream */
+          g_hash_table_insert (rtx->ssrc2_ssrc1_map, GUINT_TO_POINTER (ssrc2),
+              ssrc1);
+
+          /* also do the association between master stream and rtx stream
+           * every ssrc are unique so we can use the same hash table
+           * for both retrieving the ssrc1 from ssrc2 and also ssrc2 from ssrc1
+           */
+          g_hash_table_insert (rtx->ssrc2_ssrc1_map, ssrc1,
+              GUINT_TO_POINTER (ssrc2));
+
+        } else {
+          /* we are not able to associate this rtx packet with a master stream */
+          GST_INFO_OBJECT (rtx,
+              "dropping rtx packet %u because its orig seqnum (%u) is not in our"
+              " pending retransmission requests", seqnum, orign_seqnum);
+          drop = TRUE;
+        }
       }
+    } else {
+      /* the rtx packet is empty */
+      GST_DEBUG_OBJECT (rtx, "drop rtx packet because it is empty");
+      drop = TRUE;
     }
   }
 

subprojects/gst-plugins-good/tests/check/elements/rtprtx.c

@@ -98,36 +98,98 @@ compare_rtp_packets (GstBuffer * a, GstBuffer * b)
   gst_rtp_buffer_unmap (&rtp_b);
 }
 
+static GstRTPBuffer *
+create_rtp_buffer_ex (guint32 ssrc, guint8 payload_type, guint16 seqnum,
+    guint32 timestamp, guint payload_size)
+{
+  GstRTPBuffer *ret = g_new0 (GstRTPBuffer, 1);
+  GstBuffer *buf = gst_rtp_buffer_new_allocate (payload_size, 0, 0);
+
+  gst_rtp_buffer_map (buf, GST_MAP_WRITE, ret);
+  gst_rtp_buffer_set_ssrc (ret, ssrc);
+  gst_rtp_buffer_set_payload_type (ret, payload_type);
+  gst_rtp_buffer_set_seq (ret, seqnum);
+  gst_rtp_buffer_set_timestamp (ret, (guint32) timestamp);
+  memset (gst_rtp_buffer_get_payload (ret), 0, payload_size);
+  return ret;
+}
+
 static GstBuffer *
 create_rtp_buffer (guint32 ssrc, guint8 payload_type, guint16 seqnum)
 {
-  GstRTPBuffer rtpbuf = GST_RTP_BUFFER_INIT;
   guint payload_size = 29;
   guint64 timestamp = gst_util_uint64_scale_int (seqnum, 90000, 30);
-  GstBuffer *buf = gst_rtp_buffer_new_allocate (payload_size, 0, 0);
+  GstRTPBuffer *rtpbuf = create_rtp_buffer_ex (ssrc, payload_type, seqnum,
+      (guint32) timestamp, payload_size);
+  GstBuffer *ret = rtpbuf->buffer;
 
-  gst_rtp_buffer_map (buf, GST_MAP_WRITE, &rtpbuf);
+  memset (gst_rtp_buffer_get_payload (rtpbuf), 0x29, payload_size);
-  gst_rtp_buffer_set_ssrc (&rtpbuf, ssrc);
+
-  gst_rtp_buffer_set_payload_type (&rtpbuf, payload_type);
+  gst_rtp_buffer_unmap (rtpbuf);
-  gst_rtp_buffer_set_seq (&rtpbuf, seqnum);
+  g_free (rtpbuf);
-  gst_rtp_buffer_set_timestamp (&rtpbuf, (guint32) timestamp);
+  return ret;
-  memset (gst_rtp_buffer_get_payload (&rtpbuf), 0x29, payload_size);
-  gst_rtp_buffer_unmap (&rtpbuf);
-  return buf;
 }
 
 static GstBuffer *
 create_rtp_buffer_with_timestamp (guint32 ssrc, guint8 payload_type,
     guint16 seqnum, guint32 timestamp)
 {
-  GstRTPBuffer rtpbuf = GST_RTP_BUFFER_INIT;
+  guint payload_size = 29;
-  GstBuffer *buf = create_rtp_buffer (ssrc, payload_type, seqnum);
+  GstRTPBuffer *rtpbuf = create_rtp_buffer_ex (ssrc, payload_type, seqnum,
-  gst_rtp_buffer_map (buf, GST_MAP_WRITE, &rtpbuf);
+      timestamp, payload_size);
-  gst_rtp_buffer_set_timestamp (&rtpbuf, timestamp);
+  GstBuffer *ret = rtpbuf->buffer;
-  gst_rtp_buffer_unmap (&rtpbuf);
+
-  return buf;
+  memset (gst_rtp_buffer_get_payload (rtpbuf), 0x29, payload_size);
+
+  gst_rtp_buffer_unmap (rtpbuf);
+  g_free (rtpbuf);
+  return ret;
 }
 
+GST_START_TEST (test_rtxreceive_empty_rtx_packet)
+{
+  guint rtx_ssrc = 7654321;
+  guint master_ssrc = 1234567;
+  guint master_pt = 96;
+  guint rtx_pt = 99;
+  GstStructure *pt_map;
+  GstRTPBuffer *rtp;
+  GstBuffer *rtp_buf;
+  GstHarness *h = gst_harness_new ("rtprtxreceive");
+  pt_map = gst_structure_new ("application/x-rtp-pt-map",
+      "96", G_TYPE_UINT, rtx_pt, NULL);
+  g_object_set (h->element, "payload-type-map", pt_map, NULL);
+  gst_harness_set_src_caps_str (h, "application/x-rtp, "
+      "media = (string)video, payload = (int)96, "
+      "ssrc = (uint)1234567, clock-rate = (int)90000, "
+      "encoding-name = (string)RAW");
+
+  /* Assosiating master stream & rtx stream */
+  gst_harness_push_upstream_event (h,
+      create_rtx_event (master_ssrc, master_pt, 100));
+  /* RTX packet with seqnum=200 containing master stream buffer with seqnum=100 */
+  rtp = create_rtp_buffer_ex (rtx_ssrc, rtx_pt, 200, 0, 2);
+  rtp_buf = rtp->buffer;
+  GST_WRITE_UINT16_BE (gst_rtp_buffer_get_payload (rtp), 100);
+  gst_rtp_buffer_unmap (rtp);
+  g_free (rtp);
+  gst_buffer_unref (gst_harness_push_and_pull (h, rtp_buf));
+
+  /* Creating empty RTX packet */
+  rtp = create_rtp_buffer_ex (rtx_ssrc, rtx_pt, 201, 0, 0);
+  rtp_buf = rtp->buffer;
+  gst_rtp_buffer_unmap (rtp);
+  g_free (rtp);
+  gst_harness_push (h, rtp_buf);
+
+  /* Empty RTX packet should be ignored */
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 0);
+  gst_structure_free (pt_map);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
 GST_START_TEST (test_rtxsend_rtxreceive)
 {
   const guint packets_num = 5;
@@ -726,6 +788,7 @@ rtprtx_suite (void)
 
   suite_add_tcase (s, tc_chain);
 
+  tcase_add_test (tc_chain, test_rtxreceive_empty_rtx_packet);
   tcase_add_test (tc_chain, test_rtxsend_rtxreceive);
   tcase_add_test (tc_chain, test_rtxsend_rtxreceive_with_packet_loss);
   tcase_add_test (tc_chain, test_multi_rtxsend_rtxreceive_with_packet_loss);