From 978504e25cafc1ce59cf7fbcace3a331a9b98112 Mon Sep 17 00:00:00 2001 From: danilchap Date: Thu, 6 Apr 2017 01:03:53 -0700 Subject: [PATCH] Move rtp header extension length check from Packet::FindExtension to ExtensionT::Parse to allow to read variable-length extensions. BUG=webrtc:7433 Review-Url: https://codereview.webrtc.org/2801733002 Cr-Commit-Position: refs/heads/master@{#17554} --- .../rtp_rtcp/source/rtp_header_extensions.cc | 41 ++++++++++++++----- .../rtp_rtcp/source/rtp_header_extensions.h | 16 ++++---- webrtc/modules/rtp_rtcp/source/rtp_packet.cc | 18 ++------ webrtc/modules/rtp_rtcp/source/rtp_packet.h | 19 ++++----- .../rtp_rtcp/source/rtp_packet_unittest.cc | 2 +- 5 files changed, 51 insertions(+), 45 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc index 167f29ee95..1b311e6419 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -35,8 +35,11 @@ constexpr RTPExtensionType AbsoluteSendTime::kId; constexpr uint8_t AbsoluteSendTime::kValueSizeBytes; constexpr const char* AbsoluteSendTime::kUri; -bool AbsoluteSendTime::Parse(const uint8_t* data, uint32_t* time_24bits) { - *time_24bits = ByteReader::ReadBigEndian(data); +bool AbsoluteSendTime::Parse(rtc::ArrayView data, + uint32_t* time_24bits) { + if (data.size() != 3) + return false; + *time_24bits = ByteReader::ReadBigEndian(data.data()); return true; } @@ -61,9 +64,11 @@ constexpr RTPExtensionType AudioLevel::kId; constexpr uint8_t AudioLevel::kValueSizeBytes; constexpr const char* AudioLevel::kUri; -bool AudioLevel::Parse(const uint8_t* data, +bool AudioLevel::Parse(rtc::ArrayView data, bool* voice_activity, uint8_t* audio_level) { + if (data.size() != 1) + return false; *voice_activity = (data[0] & 0x80) != 0; *audio_level = data[0] & 0x7F; return true; @@ -97,8 +102,11 @@ constexpr RTPExtensionType TransmissionOffset::kId; constexpr uint8_t TransmissionOffset::kValueSizeBytes; constexpr const char* TransmissionOffset::kUri; -bool TransmissionOffset::Parse(const uint8_t* data, int32_t* rtp_time) { - *rtp_time = ByteReader::ReadBigEndian(data); +bool TransmissionOffset::Parse(rtc::ArrayView data, + int32_t* rtp_time) { + if (data.size() != 3) + return false; + *rtp_time = ByteReader::ReadBigEndian(data.data()); return true; } @@ -117,8 +125,11 @@ constexpr RTPExtensionType TransportSequenceNumber::kId; constexpr uint8_t TransportSequenceNumber::kValueSizeBytes; constexpr const char* TransportSequenceNumber::kUri; -bool TransportSequenceNumber::Parse(const uint8_t* data, uint16_t* value) { - *value = ByteReader::ReadBigEndian(data); +bool TransportSequenceNumber::Parse(rtc::ArrayView data, + uint16_t* value) { + if (data.size() != 2) + return false; + *value = ByteReader::ReadBigEndian(data.data()); return true; } @@ -142,7 +153,10 @@ constexpr RTPExtensionType VideoOrientation::kId; constexpr uint8_t VideoOrientation::kValueSizeBytes; constexpr const char* VideoOrientation::kUri; -bool VideoOrientation::Parse(const uint8_t* data, VideoRotation* rotation) { +bool VideoOrientation::Parse(rtc::ArrayView data, + VideoRotation* rotation) { + if (data.size() != 1) + return false; *rotation = ConvertCVOByteToVideoRotation(data[0]); return true; } @@ -152,7 +166,10 @@ bool VideoOrientation::Write(uint8_t* data, VideoRotation rotation) { return true; } -bool VideoOrientation::Parse(const uint8_t* data, uint8_t* value) { +bool VideoOrientation::Parse(rtc::ArrayView data, + uint8_t* value) { + if (data.size() != 1) + return false; *value = data[0]; return true; } @@ -171,10 +188,12 @@ constexpr RTPExtensionType PlayoutDelayLimits::kId; constexpr uint8_t PlayoutDelayLimits::kValueSizeBytes; constexpr const char* PlayoutDelayLimits::kUri; -bool PlayoutDelayLimits::Parse(const uint8_t* data, +bool PlayoutDelayLimits::Parse(rtc::ArrayView data, PlayoutDelay* playout_delay) { RTC_DCHECK(playout_delay); - uint32_t raw = ByteReader::ReadBigEndian(data); + if (data.size() != 3) + return false; + uint32_t raw = ByteReader::ReadBigEndian(data.data()); uint16_t min_raw = (raw >> 12); uint16_t max_raw = (raw & 0xfff); if (min_raw > max_raw) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h index ea6f9dbc9c..543688c75e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h @@ -13,6 +13,7 @@ #include #include "webrtc/api/video/video_rotation.h" +#include "webrtc/base/array_view.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -24,7 +25,7 @@ class AbsoluteSendTime { static constexpr const char* kUri = "http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time"; - static bool Parse(const uint8_t* data, uint32_t* time_24bits); + static bool Parse(rtc::ArrayView data, uint32_t* time_24bits); static bool Write(uint8_t* data, int64_t time_ms); static constexpr uint32_t MsTo24Bits(int64_t time_ms) { @@ -39,7 +40,7 @@ class AudioLevel { static constexpr const char* kUri = "urn:ietf:params:rtp-hdrext:ssrc-audio-level"; - static bool Parse(const uint8_t* data, + static bool Parse(rtc::ArrayView data, bool* voice_activity, uint8_t* audio_level); static bool Write(uint8_t* data, bool voice_activity, uint8_t audio_level); @@ -51,7 +52,7 @@ class TransmissionOffset { static constexpr uint8_t kValueSizeBytes = 3; static constexpr const char* kUri = "urn:ietf:params:rtp-hdrext:toffset"; - static bool Parse(const uint8_t* data, int32_t* rtp_time); + static bool Parse(rtc::ArrayView data, int32_t* rtp_time); static bool Write(uint8_t* data, int32_t rtp_time); }; @@ -62,7 +63,7 @@ class TransportSequenceNumber { static constexpr const char* kUri = "http://www.ietf.org/id/" "draft-holmer-rmcat-transport-wide-cc-extensions-01"; - static bool Parse(const uint8_t* data, uint16_t* value); + static bool Parse(rtc::ArrayView data, uint16_t* value); static bool Write(uint8_t* data, uint16_t value); }; @@ -72,9 +73,9 @@ class VideoOrientation { static constexpr uint8_t kValueSizeBytes = 1; static constexpr const char* kUri = "urn:3gpp:video-orientation"; - static bool Parse(const uint8_t* data, VideoRotation* value); + static bool Parse(rtc::ArrayView data, VideoRotation* value); static bool Write(uint8_t* data, VideoRotation value); - static bool Parse(const uint8_t* data, uint8_t* value); + static bool Parse(rtc::ArrayView data, uint8_t* value); static bool Write(uint8_t* data, uint8_t value); }; @@ -92,7 +93,8 @@ class PlayoutDelayLimits { // Maximum playout delay value in milliseconds. static constexpr int kMaxMs = 0xfff * kGranularityMs; // 40950. - static bool Parse(const uint8_t* data, PlayoutDelay* playout_delay); + static bool Parse(rtc::ArrayView data, + PlayoutDelay* playout_delay); static bool Write(uint8_t* data, const PlayoutDelay& playout_delay); }; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet.cc index b66f001bd8..0c28092d4c 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet.cc @@ -533,27 +533,17 @@ bool Packet::ParseBuffer(const uint8_t* buffer, size_t size) { return true; } -bool Packet::FindExtension(ExtensionType type, - uint8_t length, - uint16_t* offset) const { - RTC_DCHECK(offset); +rtc::ArrayView Packet::FindExtension(ExtensionType type) const { for (const ExtensionInfo& extension : extension_entries_) { if (extension.type == type) { if (extension.length == 0) { // Extension is registered but not set. - return false; + return nullptr; } - if (length != extension.length) { - LOG(LS_WARNING) << "Length mismatch for extension '" << type - << "': expected " << static_cast(length) - << ", received " << static_cast(extension.length); - return false; - } - *offset = extension.offset; - return true; + return rtc::MakeArrayView(data() + extension.offset, extension.length); } } - return false; + return nullptr; } rtc::ArrayView Packet::AllocateExtension(ExtensionType type, diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet.h b/webrtc/modules/rtp_rtcp/source/rtp_packet.h index 072456dd5b..ca189dbe0d 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet.h @@ -142,13 +142,9 @@ class Packet { // but does not touch packet own buffer, leaving packet in invalid state. bool ParseBuffer(const uint8_t* buffer, size_t size); - // Find an extension based on the type field of the parameter. - // If found, length field would be validated, the offset field will be set - // and true returned, - // otherwise the parameter will be unchanged and false is returned. - bool FindExtension(ExtensionType type, - uint8_t length, - uint16_t* offset) const; + // Find an extension |type|. + // Returns view of the raw extension or empty view on failure. + rtc::ArrayView FindExtension(ExtensionType type) const; // Find or allocate an extension |type|. Returns view of size |length| // to write raw extension to or an empty view on failure. @@ -174,16 +170,15 @@ class Packet { template bool Packet::HasExtension() const { - uint16_t offset = 0; - return FindExtension(Extension::kId, Extension::kValueSizeBytes, &offset); + return !FindExtension(Extension::kId).empty(); } template bool Packet::GetExtension(Values... values) const { - uint16_t offset = 0; - if (!FindExtension(Extension::kId, Extension::kValueSizeBytes, &offset)) + auto raw = FindExtension(Extension::kId); + if (raw.empty()) return false; - return Extension::Parse(data() + offset, values...); + return Extension::Parse(raw, values...); } template diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc index d8870654f8..bdf3aba7b0 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -358,7 +358,7 @@ TEST(RtpPacketTest, ParseWithoutExtensionManager) { int32_t time_offset = 0; auto raw_extension = packet.GetRawExtension(kTransmissionOffsetExtensionId); EXPECT_EQ(raw_extension.size(), TransmissionOffset::kValueSizeBytes); - EXPECT_TRUE(TransmissionOffset::Parse(raw_extension.data(), &time_offset)); + EXPECT_TRUE(TransmissionOffset::Parse(raw_extension, &time_offset)); EXPECT_EQ(time_offset, kTimeOffset); }