diff --git a/media/engine/webrtc_media_engine.cc b/media/engine/webrtc_media_engine.cc index 6ce52e4c8e..f083b9c9ca 100644 --- a/media/engine/webrtc_media_engine.cc +++ b/media/engine/webrtc_media_engine.cc @@ -10,6 +10,7 @@ #include "media/engine/webrtc_media_engine.h" +#include #include #include @@ -74,7 +75,8 @@ void DiscardRedundantExtensions( } // namespace bool ValidateRtpExtensions( - const std::vector& extensions) { + rtc::ArrayView extensions, + rtc::ArrayView old_extensions) { bool id_used[1 + webrtc::RtpExtension::kMaxId] = {false}; for (const auto& extension : extensions) { if (extension.id < webrtc::RtpExtension::kMinId || @@ -89,6 +91,45 @@ bool ValidateRtpExtensions( } id_used[extension.id] = true; } + // Validate the extension list against the already negotiated extensions. + // Re-registering is OK, re-mapping (either same URL at new ID or same + // ID used with new URL) is an illegal remap. + + // This is required in order to avoid a crash when registering an + // extension. A better structure would use the registered extensions + // in the RTPSender. This requires spinning through: + // + // WebRtcVoiceMediaChannel::::WebRtcAudioSendStream::stream_ (pointer) + // AudioSendStream::rtp_rtcp_module_ (pointer) + // ModuleRtpRtcpImpl2::rtp_sender_ (pointer) + // RtpSenderContext::packet_generator (struct member) + // RTPSender::rtp_header_extension_map_ (class member) + // + // Getting at this seems like a hard slog. + if (!old_extensions.empty()) { + absl::string_view urimap[1 + webrtc::RtpExtension::kMaxId]; + std::map idmap; + for (const auto& old_extension : old_extensions) { + urimap[old_extension.id] = old_extension.uri; + idmap[old_extension.uri] = old_extension.id; + } + for (const auto& extension : extensions) { + if (!urimap[extension.id].empty() && + urimap[extension.id] != extension.uri) { + RTC_LOG(LS_ERROR) << "Extension negotiation failure: " << extension.id + << " was mapped to " << urimap[extension.id] + << " but is proposed changed to " << extension.uri; + return false; + } + const auto& it = idmap.find(extension.uri); + if (it != idmap.end() && it->second != extension.id) { + RTC_LOG(LS_ERROR) << "Extension negotation failure: " << extension.uri + << " was identified by " << it->second + << " but is proposed changed to " << extension.id; + return false; + } + } + } return true; } @@ -97,7 +138,8 @@ std::vector FilterRtpExtensions( bool (*supported)(absl::string_view), bool filter_redundant_extensions, const webrtc::WebRtcKeyValueConfig& trials) { - RTC_DCHECK(ValidateRtpExtensions(extensions)); + // Don't check against old parameters; this should have been done earlier. + RTC_DCHECK(ValidateRtpExtensions(extensions, {})); RTC_DCHECK(supported); std::vector result; diff --git a/media/engine/webrtc_media_engine.h b/media/engine/webrtc_media_engine.h index 34ec4cdc9c..ff977609b2 100644 --- a/media/engine/webrtc_media_engine.h +++ b/media/engine/webrtc_media_engine.h @@ -63,8 +63,11 @@ RTC_EXPORT std::unique_ptr CreateMediaEngine( MediaEngineDependencies dependencies); // Verify that extension IDs are within 1-byte extension range and are not -// overlapping. -bool ValidateRtpExtensions(const std::vector& extensions); +// overlapping, and that they form a legal change from previously registerd +// extensions (if any). +bool ValidateRtpExtensions( + rtc::ArrayView extennsions, + rtc::ArrayView old_extensions); // Discard any extensions not validated by the 'supported' predicate. Duplicate // extensions are removed if 'filter_redundant_extensions' is set, and also any diff --git a/media/engine/webrtc_media_engine_unittest.cc b/media/engine/webrtc_media_engine_unittest.cc index 78d13fc297..81982fae2b 100644 --- a/media/engine/webrtc_media_engine_unittest.cc +++ b/media/engine/webrtc_media_engine_unittest.cc @@ -66,41 +66,68 @@ bool IsSorted(const std::vector& extensions) { } } // namespace -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_EmptyList) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsEmptyList) { std::vector extensions; - EXPECT_TRUE(ValidateRtpExtensions(extensions)); + EXPECT_TRUE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_AllGood) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsAllGood) { std::vector extensions = MakeUniqueExtensions(); - EXPECT_TRUE(ValidateRtpExtensions(extensions)); + EXPECT_TRUE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OutOfRangeId_Low) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOutOfRangeId_Low) { std::vector extensions = MakeUniqueExtensions(); extensions.push_back(RtpExtension("foo", 0)); - EXPECT_FALSE(ValidateRtpExtensions(extensions)); + EXPECT_FALSE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OutOfRangeId_High) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOutOfRangeIdHigh) { std::vector extensions = MakeUniqueExtensions(); extensions.push_back(RtpExtension("foo", 256)); - EXPECT_FALSE(ValidateRtpExtensions(extensions)); + EXPECT_FALSE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_StartOfSet) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOverlappingIdsStartOfSet) { std::vector extensions = MakeUniqueExtensions(); extensions.push_back(RtpExtension("foo", 1)); - EXPECT_FALSE(ValidateRtpExtensions(extensions)); + EXPECT_FALSE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_EndOfSet) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOverlappingIdsEndOfSet) { std::vector extensions = MakeUniqueExtensions(); extensions.push_back(RtpExtension("foo", 255)); - EXPECT_FALSE(ValidateRtpExtensions(extensions)); + EXPECT_FALSE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_EmptyList) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsEmptyToEmpty) { + std::vector extensions; + EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions)); +} + +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsNoChange) { + std::vector extensions = MakeUniqueExtensions(); + EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions)); +} + +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsChangeIdNotUrl) { + std::vector old_extensions = MakeUniqueExtensions(); + std::vector new_extensions = old_extensions; + std::swap(new_extensions[0].id, new_extensions[1].id); + + EXPECT_FALSE(ValidateRtpExtensions(new_extensions, old_extensions)); +} + +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsChangeIdForUrl) { + std::vector old_extensions = MakeUniqueExtensions(); + std::vector new_extensions = old_extensions; + // Change first extension to something not generated by MakeUniqueExtensions + new_extensions[0].id = 123; + + EXPECT_FALSE(ValidateRtpExtensions(new_extensions, old_extensions)); +} + +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsEmptyList) { std::vector extensions; webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -108,7 +135,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_EmptyList) { EXPECT_EQ(0u, filtered.size()); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_IncludeOnlySupported) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsIncludeOnlySupported) { std::vector extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -118,7 +145,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_IncludeOnlySupported) { EXPECT_EQ("i", filtered[1].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_1) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsSortedByName1) { std::vector extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -127,7 +154,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_1) { EXPECT_TRUE(IsSorted(filtered)); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsSortedByName2) { std::vector extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -136,7 +163,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) { EXPECT_TRUE(IsSorted(filtered)); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_DontRemoveRedundant) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsDontRemoveRedundant) { std::vector extensions = MakeRedundantExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -146,7 +173,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_DontRemoveRedundant) { EXPECT_EQ(filtered[0].uri, filtered[1].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundant) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundant) { std::vector extensions = MakeRedundantExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -156,7 +183,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundant) { EXPECT_NE(filtered[0].uri, filtered[1].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_1) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantEncrypted1) { std::vector extensions; extensions.push_back(webrtc::RtpExtension("b", 1)); extensions.push_back(webrtc::RtpExtension("b", 2, true)); @@ -173,7 +200,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_1) { EXPECT_NE(filtered[1].uri, filtered[2].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_2) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantEncrypted2) { std::vector extensions; extensions.push_back(webrtc::RtpExtension("b", 1, true)); extensions.push_back(webrtc::RtpExtension("b", 2)); @@ -190,7 +217,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_2) { EXPECT_NE(filtered[1].uri, filtered[2].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_1) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe1) { webrtc::test::ScopedFieldTrials override_field_trials_( "WebRTC-FilterAbsSendTimeExtension/Enabled/"); webrtc::FieldTrialBasedConfig trials; @@ -209,7 +236,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_1) { } TEST(WebRtcMediaEngineTest, - FilterRtpExtensions_RemoveRedundantBwe_1_KeepAbsSendTime) { + FilterRtpExtensionsRemoveRedundantBwe1KeepAbsSendTime) { std::vector extensions; extensions.push_back( RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3)); @@ -226,7 +253,7 @@ TEST(WebRtcMediaEngineTest, EXPECT_EQ(RtpExtension::kAbsSendTimeUri, filtered[1].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBweEncrypted_1) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBweEncrypted1) { webrtc::test::ScopedFieldTrials override_field_trials_( "WebRTC-FilterAbsSendTimeExtension/Enabled/"); webrtc::FieldTrialBasedConfig trials; @@ -251,7 +278,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBweEncrypted_1) { } TEST(WebRtcMediaEngineTest, - FilterRtpExtensions_RemoveRedundantBweEncrypted_1_KeepAbsSendTime) { + FilterRtpExtensionsRemoveRedundantBweEncrypted1KeepAbsSendTime) { std::vector extensions; extensions.push_back( RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3)); @@ -274,7 +301,7 @@ TEST(WebRtcMediaEngineTest, EXPECT_NE(filtered[0].encrypt, filtered[1].encrypt); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_2) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe2) { std::vector extensions; extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 1)); extensions.push_back(RtpExtension(RtpExtension::kAbsSendTimeUri, 14)); @@ -286,7 +313,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_2) { EXPECT_EQ(RtpExtension::kAbsSendTimeUri, filtered[0].uri); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_3) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe3) { std::vector extensions; extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 2)); extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 14)); diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 5de6752d64..bdce2ba7b6 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -827,7 +827,7 @@ bool WebRtcVideoChannel::GetChangedSendParameters( const VideoSendParameters& params, ChangedSendParameters* changed_params) const { if (!ValidateCodecFormats(params.codecs) || - !ValidateRtpExtensions(params.extensions)) { + !ValidateRtpExtensions(params.extensions, send_rtp_extensions_)) { return false; } @@ -862,7 +862,7 @@ bool WebRtcVideoChannel::GetChangedSendParameters( std::vector filtered_extensions = FilterRtpExtensions( params.extensions, webrtc::RtpExtension::IsSupportedForVideo, true, call_->trials()); - if (!send_rtp_extensions_ || (*send_rtp_extensions_ != filtered_extensions)) { + if (send_rtp_extensions_ != filtered_extensions) { changed_params->rtp_header_extensions = absl::optional>(filtered_extensions); } @@ -1009,7 +1009,7 @@ bool WebRtcVideoChannel::ApplyChangedParams( SetExtmapAllowMixed(*changed_params.extmap_allow_mixed); } if (changed_params.rtp_header_extensions) { - send_rtp_extensions_ = changed_params.rtp_header_extensions; + send_rtp_extensions_ = *changed_params.rtp_header_extensions; } if (changed_params.send_codec || changed_params.max_bandwidth_bps) { @@ -1186,7 +1186,7 @@ bool WebRtcVideoChannel::GetChangedRecvParameters( const VideoRecvParameters& params, ChangedRecvParameters* changed_params) const { if (!ValidateCodecFormats(params.codecs) || - !ValidateRtpExtensions(params.extensions)) { + !ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) { return false; } diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 8b3a7f42c6..90d824a55b 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -598,7 +598,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, std::vector negotiated_codecs_ RTC_GUARDED_BY(thread_checker_); - absl::optional> send_rtp_extensions_ + std::vector send_rtp_extensions_ RTC_GUARDED_BY(thread_checker_); webrtc::VideoEncoderFactory* const encoder_factory_ diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 448ad3522a..7ce5063cfe 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1384,7 +1384,7 @@ bool WebRtcVoiceMediaChannel::SetSendParameters( return false; } - if (!ValidateRtpExtensions(params.extensions)) { + if (!ValidateRtpExtensions(params.extensions, send_rtp_extensions_)) { return false; } @@ -1430,7 +1430,7 @@ bool WebRtcVoiceMediaChannel::SetRecvParameters( return false; } - if (!ValidateRtpExtensions(params.extensions)) { + if (!ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) { return false; } std::vector filtered_extensions = FilterRtpExtensions( diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map.cc b/modules/rtp_rtcp/source/rtp_header_extension_map.cc index 8fcef1de49..3c9659343e 100644 --- a/modules/rtp_rtcp/source/rtp_header_extension_map.cc +++ b/modules/rtp_rtcp/source/rtp_header_extension_map.cc @@ -154,7 +154,12 @@ bool RtpHeaderExtensionMap::Register(int id, << static_cast(registered_type); return false; } - RTC_DCHECK(!IsRegistered(type)); + if (IsRegistered(type)) { + RTC_LOG(LS_WARNING) << "Illegal reregistration for uri: " << uri + << " is previously registered with id " << GetId(type) + << " and cannot be reregistered with id " << id; + return false; + } // There is a run-time check above id fits into uint8_t. ids_[type] = static_cast(id); diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc b/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc index 19a17a5ecc..42842cc876 100644 --- a/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc @@ -106,4 +106,10 @@ TEST(RtpHeaderExtensionTest, GetId) { EXPECT_EQ(3, map.GetId(TransmissionOffset::kId)); } +TEST(RtpHeaderExtensionTest, RemapFails) { + RtpHeaderExtensionMap map; + EXPECT_TRUE(map.Register(3)); + EXPECT_FALSE(map.Register(4)); +} + } // namespace webrtc