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-Original-Commit-Position: refs/heads/master@{#12318}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 90a1351072a42f0b835f1c759f969ea014ce6b7f
diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc
index 1bfa7cd..0faf2a4 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver.cc
@@ -1101,8 +1101,7 @@
{
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 @@
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/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
index 507f835..08f109b 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc
+++ b/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 @@
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/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index f440da4..dafc2e0 100644
--- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -461,6 +461,7 @@
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/modules/rtp_rtcp/source/rtcp_utility.cc b/modules/rtp_rtcp/source/rtcp_utility.cc
index 63693fa..9b5a835 100644
--- a/modules/rtp_rtcp/source/rtcp_utility.cc
+++ b/modules/rtp_rtcp/source/rtcp_utility.cc
@@ -1334,11 +1334,19 @@
return false;
}
- _packetType = RTCPPacketTypes::kPsfbRpsi;
uint8_t padding_bits = *_ptrRTCPData++;
_packet.RPSI.PayloadType = *_ptrRTCPData++;
+ if (padding_bits > static_cast<uint16_t>(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/modules/rtp_rtcp/source/rtcp_utility.h b/modules/rtp_rtcp/source/rtcp_utility.h
index 0b03ceb..4067a40 100644
--- a/modules/rtp_rtcp/source/rtcp_utility.h
+++ b/modules/rtp_rtcp/source/rtcp_utility.h
@@ -272,6 +272,7 @@
kPsfbPli,
kPsfbRpsi,
+ kPsfbRpsiItem,
kPsfbSli,
kPsfbSliItem,
kPsfbApp,
diff --git a/test/rtcp_packet_parser.cc b/test/rtcp_packet_parser.cc
index 8ce249e..65f450d 100644
--- a/test/rtcp_packet_parser.cc
+++ b/test/rtcp_packet_parser.cc
@@ -68,7 +68,7 @@
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:
