From 815d279f2e48865c05af89620806efefae26b48e Mon Sep 17 00:00:00 2001 From: Mikhail Fludkov Date: Tue, 23 Aug 2016 19:06:49 +0200 Subject: [PATCH] rtprtxreceive: fix crash when RTX payload has zero length Part-of: --- .../gst/rtpmanager/gstrtprtxreceive.c | 129 +++++++++++---------- .../gst-plugins-good/tests/check/elements/rtprtx.c | 95 ++++++++++++--- 2 files changed, 148 insertions(+), 76 deletions(-) diff --git a/subprojects/gst-plugins-good/gst/rtpmanager/gstrtprtxreceive.c b/subprojects/gst-plugins-good/gst/rtpmanager/gstrtprtxreceive.c index 2a4cfc5..7373eda 100644 --- a/subprojects/gst-plugins-good/gst/rtpmanager/gstrtprtxreceive.c +++ b/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 */ - 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, " - "rtx pt %u, orig seqnum %u, orig pt %u", seqnum, ssrc, payload_type, - orign_seqnum, origin_payload_type); - - /* first we check if we already have associated this retransmission stream - * to a master stream */ - if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map, - GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) { - GST_TRACE_OBJECT (rtx, - "packet is from retransmission stream %X already associated to " - "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); + /* check if there enough data to read OSN from the paylaod, + we need at least two bytes + */ + if (gst_rtp_buffer_get_payload_len (&rtp) > 1) { + /* 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, " + "rtx pt %u, orig seqnum %u, orig pt %u", seqnum, ssrc, payload_type, + orign_seqnum, origin_payload_type); + + /* first we check if we already have associated this retransmission stream + * to a master stream */ + if (g_hash_table_lookup_extended (rtx->ssrc2_ssrc1_map, + GUINT_TO_POINTER (ssrc), NULL, &ssrc1)) { + GST_TRACE_OBJECT (rtx, + "packet is from retransmission stream %X already associated to " + "master stream %X", ssrc, GPOINTER_TO_UINT (ssrc1)); 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; + 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 */ + 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; } } diff --git a/subprojects/gst-plugins-good/tests/check/elements/rtprtx.c b/subprojects/gst-plugins-good/tests/check/elements/rtprtx.c index dc7b96f..2e4c651 100644 --- a/subprojects/gst-plugins-good/tests/check/elements/rtprtx.c +++ b/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); - gst_rtp_buffer_set_ssrc (&rtpbuf, ssrc); - gst_rtp_buffer_set_payload_type (&rtpbuf, payload_type); - gst_rtp_buffer_set_seq (&rtpbuf, seqnum); - gst_rtp_buffer_set_timestamp (&rtpbuf, (guint32) timestamp); - memset (gst_rtp_buffer_get_payload (&rtpbuf), 0x29, payload_size); - 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; } static GstBuffer * create_rtp_buffer_with_timestamp (guint32 ssrc, guint8 payload_type, guint16 seqnum, guint32 timestamp) { - GstRTPBuffer rtpbuf = GST_RTP_BUFFER_INIT; - GstBuffer *buf = create_rtp_buffer (ssrc, payload_type, seqnum); - gst_rtp_buffer_map (buf, GST_MAP_WRITE, &rtpbuf); - gst_rtp_buffer_set_timestamp (&rtpbuf, timestamp); - gst_rtp_buffer_unmap (&rtpbuf); - return buf; + guint payload_size = 29; + GstRTPBuffer *rtpbuf = create_rtp_buffer_ex (ssrc, payload_type, seqnum, + timestamp, payload_size); + GstBuffer *ret = rtpbuf->buffer; + + 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); -- 2.7.4