From d3b7ec2e911a3bc93c57579a6094e39789b37bae Mon Sep 17 00:00:00 2001 From: Joachim Bauch Date: Wed, 1 Aug 2018 10:12:00 +0200 Subject: [PATCH] Allow all "token" chars from RFC 4566 when checking for legal mid names. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously only alphanumeric characters were allowed. Bug: webrtc:9537 Change-Id: I3fd793ad88520b25ecd884efe3a698f2f0af4639 Reviewed-on: https://webrtc-review.googlesource.com/89388 Reviewed-by: Åsa Persson Reviewed-by: Steve Anton Reviewed-by: Fredrik Solenberg Reviewed-by: Harald Alvestrand Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#24167} --- api/rtp_headers.h | 9 +++- call/rtcp_demuxer.cc | 2 +- call/rtp_demuxer.cc | 4 +- call/rtp_demuxer_unittest.cc | 4 +- modules/rtp_rtcp/include/rtp_rtcp_defines.cc | 14 +++++- modules/rtp_rtcp/test/testAPI/test_api.cc | 51 ++++++++++++++++++++ 6 files changed, 77 insertions(+), 7 deletions(-) diff --git a/api/rtp_headers.h b/api/rtp_headers.h index c0b22cc101..c762534ab3 100644 --- a/api/rtp_headers.h +++ b/api/rtp_headers.h @@ -39,7 +39,14 @@ class StringRtpHeaderExtension { // maximum length that can be encoded with one-byte header extensions. static constexpr size_t kMaxSize = 16; - static bool IsLegalName(rtc::ArrayView name); + static bool IsLegalMidName(rtc::ArrayView name); + static bool IsLegalRsidName(rtc::ArrayView name); + + // TODO(bugs.webrtc.org/9537): Deprecate and remove when third parties have + // migrated to "IsLegalRsidName". + static bool IsLegalName(rtc::ArrayView name) { + return IsLegalRsidName(name); + } StringRtpHeaderExtension() { value_[0] = 0; } explicit StringRtpHeaderExtension(rtc::ArrayView value) { diff --git a/call/rtcp_demuxer.cc b/call/rtcp_demuxer.cc index 40c9163203..d441c05b3d 100644 --- a/call/rtcp_demuxer.cc +++ b/call/rtcp_demuxer.cc @@ -35,7 +35,7 @@ void RtcpDemuxer::AddSink(uint32_t sender_ssrc, RtcpPacketSinkInterface* sink) { void RtcpDemuxer::AddSink(const std::string& rsid, RtcpPacketSinkInterface* sink) { - RTC_DCHECK(StreamId::IsLegalName(rsid)); + RTC_DCHECK(StreamId::IsLegalRsidName(rsid)); RTC_DCHECK(sink); RTC_DCHECK(!ContainerHasKey(broadcast_sinks_, sink)); RTC_DCHECK(!MultimapAssociationExists(rsid_sinks_, rsid, sink)); diff --git a/call/rtp_demuxer.cc b/call/rtp_demuxer.cc index a8944b5543..8f0e2e529d 100644 --- a/call/rtp_demuxer.cc +++ b/call/rtp_demuxer.cc @@ -38,8 +38,8 @@ bool RtpDemuxer::AddSink(const RtpDemuxerCriteria& criteria, RtpPacketSinkInterface* sink) { RTC_DCHECK(!criteria.payload_types.empty() || !criteria.ssrcs.empty() || !criteria.mid.empty() || !criteria.rsid.empty()); - RTC_DCHECK(criteria.mid.empty() || Mid::IsLegalName(criteria.mid)); - RTC_DCHECK(criteria.rsid.empty() || StreamId::IsLegalName(criteria.rsid)); + RTC_DCHECK(criteria.mid.empty() || Mid::IsLegalMidName(criteria.mid)); + RTC_DCHECK(criteria.rsid.empty() || StreamId::IsLegalRsidName(criteria.rsid)); RTC_DCHECK(sink); // We return false instead of DCHECKing for logical conflicts with the new diff --git a/call/rtp_demuxer_unittest.cc b/call/rtp_demuxer_unittest.cc index 76a13ee248..b5ede3cd03 100644 --- a/call/rtp_demuxer_unittest.cc +++ b/call/rtp_demuxer_unittest.cc @@ -1491,9 +1491,9 @@ TEST_F(RtpDemuxerTest, RsidMustBeAlphaNumeric) { EXPECT_DEATH(AddSinkOnlyRsid("a_3", &sink), ""); } -TEST_F(RtpDemuxerTest, MidMustBeAlphaNumeric) { +TEST_F(RtpDemuxerTest, MidMustBeToken) { MockRtpPacketSink sink; - EXPECT_DEATH(AddSinkOnlyMid("a_3", &sink), ""); + EXPECT_DEATH(AddSinkOnlyMid("a(3)", &sink), ""); } TEST_F(RtpDemuxerTest, RsidMustNotExceedMaximumLength) { diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc index ad9bd45c78..f86b238b7d 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc @@ -16,7 +16,19 @@ StreamDataCounters::StreamDataCounters() : first_packet_time_ms(-1) {} constexpr size_t StreamId::kMaxSize; -bool StreamId::IsLegalName(rtc::ArrayView name) { +// Check if passed character is a "token-char" from RFC 4566. +static bool IsTokenChar(char ch) { + return ch == 0x21 || (ch >= 0x23 && ch <= 0x27) || ch == 0x2a || ch == 0x2b || + ch == 0x2d || ch == 0x2e || (ch >= 0x30 && ch <= 0x39) || + (ch >= 0x41 && ch <= 0x5a) || (ch >= 0x5e && ch <= 0x7e); +} + +bool StreamId::IsLegalMidName(rtc::ArrayView name) { + return (name.size() <= kMaxSize && name.size() > 0 && + std::all_of(name.data(), name.data() + name.size(), IsTokenChar)); +} + +bool StreamId::IsLegalRsidName(rtc::ArrayView name) { return (name.size() <= kMaxSize && name.size() > 0 && std::all_of(name.data(), name.data() + name.size(), isalnum)); } diff --git a/modules/rtp_rtcp/test/testAPI/test_api.cc b/modules/rtp_rtcp/test/testAPI/test_api.cc index fd5067be99..b11a18b056 100644 --- a/modules/rtp_rtcp/test/testAPI/test_api.cc +++ b/modules/rtp_rtcp/test/testAPI/test_api.cc @@ -12,6 +12,7 @@ #include #include +#include #include #include "rtc_base/checks.h" @@ -160,4 +161,54 @@ TEST_F(RtpRtcpAPITest, RtxSender) { EXPECT_EQ(kRtxRetransmitted, module_->RtxSendStatus()); } +TEST_F(RtpRtcpAPITest, LegalMidName) { + static const std::string kLegalMidNames[] = { + // clang-format off + "audio", + "audio0", + "audio_0", + // clang-format on + }; + for (const auto& name : kLegalMidNames) { + EXPECT_TRUE(StreamId::IsLegalMidName(name)) + << "Mid should be legal: " << name; + } + + static const std::string kNonLegalMidNames[] = { + // clang-format off + "", + "(audio0)", + // clang-format on + }; + for (const auto& name : kNonLegalMidNames) { + EXPECT_FALSE(StreamId::IsLegalMidName(name)) + << "Mid should not be legal: " << name; + } +} + +TEST_F(RtpRtcpAPITest, LegalRsidName) { + static const std::string kLegalRsidNames[] = { + // clang-format off + "audio", + "audio0", + // clang-format on + }; + for (const auto& name : kLegalRsidNames) { + EXPECT_TRUE(StreamId::IsLegalRsidName(name)) + << "Rsid should be legal: " << name; + } + + static const std::string kNonLegalRsidNames[] = { + // clang-format off + "", + "audio_0", + "(audio0)", + // clang-format on + }; + for (const auto& name : kNonLegalRsidNames) { + EXPECT_FALSE(StreamId::IsLegalRsidName(name)) + << "Rsid should not be legal: " << name; + } +} + } // namespace webrtc