From 90a1351072a42f0b835f1c759f969ea014ce6b7f Mon Sep 17 00:00:00 2001 From: danilchap Date: Mon, 11 Apr 2016 10:05:02 -0700 Subject: [PATCH] Fixed rtcp rpsi parsing of invalid packets. Added packet type RpsiItem to destinguish parsed rpsi header and Rpsi body preventing handling two half-valid (header-only) rpsi packets as one valid, making test parser calculate rpsi packet once instead of twice. Added check padding bits doesn't exceed native bit string length. Marking rpsi received moved after it is validated. BUG=600977 Review URL: https://codereview.webrtc.org/1880443002 Cr-Commit-Position: refs/heads/master@{#12318} --- .../modules/rtp_rtcp/source/rtcp_receiver.cc | 5 ++- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 45 +++++++++++++++++++ .../rtp_rtcp/source/rtcp_sender_unittest.cc | 1 + .../modules/rtp_rtcp/source/rtcp_utility.cc | 10 ++++- webrtc/modules/rtp_rtcp/source/rtcp_utility.h | 1 + webrtc/test/rtcp_packet_parser.cc | 2 +- 6 files changed, 60 insertions(+), 4 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index 1bfa7cdebc..0faf2a4257 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -1101,8 +1101,7 @@ RTCPReceiver::HandleRPSI(RTCPUtility::RTCPParserV2& rtcpParser, { const RTCPUtility::RTCPPacket& rtcpPacket = rtcpParser.Packet(); RTCPUtility::RTCPPacketTypes pktType = rtcpParser.Iterate(); - if (pktType == RTCPPacketTypes::kPsfbRpsi) { - rtcpPacketInformation.rtcpPacketTypeFlags |= kRtcpRpsi; // received signal that we have a confirmed reference picture + if (pktType == RTCPPacketTypes::kPsfbRpsiItem) { if(rtcpPacket.RPSI.NumberOfValidBits%8 != 0) { // to us unknown @@ -1110,6 +1109,8 @@ RTCPReceiver::HandleRPSI(RTCPUtility::RTCPParserV2& rtcpParser, rtcpParser.Iterate(); return; } + // Received signal that we have a confirmed reference picture. + rtcpPacketInformation.rtcpPacketTypeFlags |= kRtcpRpsi; rtcpPacketInformation.rpsiPictureId = 0; // convert NativeBitString to rpsiPictureId diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 507f835bc3..08f109bc29 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -29,6 +29,7 @@ #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/pli.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h" +#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rpsi.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/sli.h" @@ -165,6 +166,50 @@ TEST_F(RtcpReceiverTest, InvalidFeedbackPacketIsIgnored) { EXPECT_EQ(0U, rtcp_packet_info_.rtcpPacketTypeFlags); } +TEST_F(RtcpReceiverTest, RpsiWithFractionalPaddingIsIgnored) { + // Padding size represent fractional number of bytes. + const uint8_t kPaddingSizeBits = 0x0b; + const uint8_t bad_packet[] = {0x83, RTCPUtility::PT_PSFB, 0, 3, + 0x12, 0x34, 0x56, 0x78, + 0x98, 0x76, 0x54, 0x32, + kPaddingSizeBits, 0x00, 0x00, 0x00}; + EXPECT_EQ(0, InjectRtcpPacket(bad_packet, sizeof(bad_packet))); + EXPECT_EQ(0U, rtcp_packet_info_.rtcpPacketTypeFlags); +} + +TEST_F(RtcpReceiverTest, RpsiWithTooLargePaddingIsIgnored) { + // Padding size exceeds packet size. + const uint8_t kPaddingSizeBits = 0xa8; + const uint8_t bad_packet[] = {0x83, RTCPUtility::PT_PSFB, 0, 3, + 0x12, 0x34, 0x56, 0x78, + 0x98, 0x76, 0x54, 0x32, + kPaddingSizeBits, 0x00, 0x00, 0x00}; + EXPECT_EQ(0, InjectRtcpPacket(bad_packet, sizeof(bad_packet))); + EXPECT_EQ(0U, rtcp_packet_info_.rtcpPacketTypeFlags); +} + +// With parsing using rtcp classes this test will make no sense. +// With current stateful parser this test was failing. +TEST_F(RtcpReceiverTest, TwoHalfValidRpsiAreIgnored) { + const uint8_t bad_packet[] = {0x83, RTCPUtility::PT_PSFB, 0, 2, + 0x12, 0x34, 0x56, 0x78, + 0x98, 0x76, 0x54, 0x32, + 0x83, RTCPUtility::PT_PSFB, 0, 2, + 0x12, 0x34, 0x56, 0x78, + 0x98, 0x76, 0x54, 0x32}; + EXPECT_EQ(0, InjectRtcpPacket(bad_packet, sizeof(bad_packet))); + EXPECT_EQ(0U, rtcp_packet_info_.rtcpPacketTypeFlags); +} + +TEST_F(RtcpReceiverTest, InjectRpsiPacket) { + const uint64_t kPictureId = 0x123456789; + rtcp::Rpsi rpsi; + rpsi.WithPictureId(kPictureId); + rtc::Buffer packet = rpsi.Build(); + EXPECT_EQ(0, InjectRtcpPacket(packet.data(), packet.size())); + EXPECT_EQ(kRtcpRpsi, rtcp_packet_info_.rtcpPacketTypeFlags); +} + TEST_F(RtcpReceiverTest, InjectSrPacket) { const uint32_t kSenderSsrc = 0x10203; rtcp::SenderReport sr; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index f440da489d..dafc2e0be6 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -461,6 +461,7 @@ TEST_F(RtcpSenderTest, SendRpsi) { feedback_state.send_payload_type = kPayloadType; EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state, kRtcpRpsi, 0, nullptr, false, kPictureId)); + EXPECT_EQ(1, parser()->rpsi()->num_packets()); EXPECT_EQ(kPayloadType, parser()->rpsi()->PayloadType()); EXPECT_EQ(kPictureId, parser()->rpsi()->PictureId()); } diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc index 63693fa9c2..9b5a83515f 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc @@ -1334,11 +1334,19 @@ bool RTCPUtility::RTCPParserV2::ParseRPSIItem() { return false; } - _packetType = RTCPPacketTypes::kPsfbRpsi; uint8_t padding_bits = *_ptrRTCPData++; _packet.RPSI.PayloadType = *_ptrRTCPData++; + if (padding_bits > static_cast(length - 2) * 8) { + _state = ParseState::State_TopLevel; + + EndCurrentBlock(); + return false; + } + + _packetType = RTCPPacketTypes::kPsfbRpsiItem; + memcpy(_packet.RPSI.NativeBitString, _ptrRTCPData, length - 2); _ptrRTCPData += length - 2; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_utility.h b/webrtc/modules/rtp_rtcp/source/rtcp_utility.h index 0b03ceb56e..4067a40886 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.h @@ -272,6 +272,7 @@ enum class RTCPPacketTypes { kPsfbPli, kPsfbRpsi, + kPsfbRpsiItem, kPsfbSli, kPsfbSliItem, kPsfbApp, diff --git a/webrtc/test/rtcp_packet_parser.cc b/webrtc/test/rtcp_packet_parser.cc index 8ce249e0b6..65f450d2e3 100644 --- a/webrtc/test/rtcp_packet_parser.cc +++ b/webrtc/test/rtcp_packet_parser.cc @@ -68,7 +68,7 @@ void RtcpPacketParser::Parse(const void *data, size_t len) { case RTCPPacketTypes::kPsfbSliItem: sli_item_.Set(parser.Packet().SLIItem); break; - case RTCPPacketTypes::kPsfbRpsi: + case RTCPPacketTypes::kPsfbRpsiItem: rpsi_.Set(parser.Packet().RPSI); break; case RTCPPacketTypes::kPsfbFir: