From 75a106672e0b8892097f6e243733d51d228d952c Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Fri, 25 Oct 2024 14:17:02 +0000 Subject: [PATCH] Guard against redefining a PT within a single codec list. Bug: webrtc:360058654 Change-Id: I433031a11f40a70cedc3862edb3eee4e94ddbdc9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366563 Reviewed-by: Florent Castelli Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#43318} --- call/BUILD.gn | 1 + call/payload_type_picker.cc | 30 ++++++++++++++++++++++++++-- call/payload_type_picker.h | 17 +++++++++++++++- call/payload_type_picker_unittest.cc | 18 +++++++++++++++++ pc/jsep_transport.cc | 16 ++++++++++++--- 5 files changed, 76 insertions(+), 6 deletions(-) diff --git a/call/BUILD.gn b/call/BUILD.gn index ed33d2cc72..ab93575312 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -394,6 +394,7 @@ rtc_library("payload_type_picker") { "../api/audio_codecs:audio_codecs_api", "../media:codec", "../media:media_constants", + "../rtc_base:checks", "../rtc_base:logging", "../rtc_base:strong_alias", "//third_party/abseil-cpp/absl/strings", diff --git a/call/payload_type_picker.cc b/call/payload_type_picker.cc index 12854cfec3..3e75db4c37 100644 --- a/call/payload_type_picker.cc +++ b/call/payload_type_picker.cc @@ -22,6 +22,7 @@ #include "media/base/codec.h" #include "media/base/codec_comparators.h" #include "media/base/media_constants.h" +#include "rtc_base/checks.h" #include "rtc_base/logging.h" namespace webrtc { @@ -178,6 +179,17 @@ RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type, auto existing_codec_it = payload_type_to_codec_.find(payload_type); if (existing_codec_it != payload_type_to_codec_.end() && !MatchesWithCodecRules(codec, existing_codec_it->second)) { + // Redefinition attempted. + if (disallow_redefinition_level_ > 0) { + if (accepted_definitions_.count(payload_type) > 0) { + // We have already defined this PT in this scope. + RTC_LOG(LS_WARNING) + << "Rejected attempt to redefine mapping for PT " << payload_type + << " from " << existing_codec_it->second << " to " << codec; + return RTCError(RTCErrorType::INVALID_MODIFICATION, + "Attempt to redefine a codec mapping"); + } + } if (absl::EqualsIgnoreCase(codec.name, existing_codec_it->second.name)) { // The difference is in clock rate, channels or FMTP parameters. RTC_LOG(LS_INFO) << "Warning: Attempt to change a codec's parameters"; @@ -185,15 +197,17 @@ RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type, // This is done in production today, so we can't return an error. } else { RTC_LOG(LS_WARNING) << "Warning: You attempted to redefine a codec from " - << existing_codec_it->second.ToString() << " to " - << " new codec " << codec.ToString(); + << existing_codec_it->second << " to " + << " new codec " << codec; // This is a spec violation. // TODO: https://issues.webrtc.org/41480892 - return an error. } // Accept redefinition. + accepted_definitions_.emplace(payload_type); payload_type_to_codec_.insert_or_assign(payload_type, codec); return RTCError::OK(); } + accepted_definitions_.emplace(payload_type); payload_type_to_codec_.emplace(payload_type, codec); suggester_.AddMapping(payload_type, codec); return RTCError::OK(); @@ -229,6 +243,18 @@ RTCErrorOr PayloadTypeRecorder::LookupCodec( return result->second; } +void PayloadTypeRecorder::DisallowRedefinition() { + if (disallow_redefinition_level_ == 0) { + accepted_definitions_.clear(); + } + ++disallow_redefinition_level_; +} + +void PayloadTypeRecorder::ReallowRedefinition() { + RTC_CHECK(disallow_redefinition_level_ > 0); + --disallow_redefinition_level_; +} + void PayloadTypeRecorder::Commit() { checkpoint_payload_type_to_codec_ = payload_type_to_codec_; } diff --git a/call/payload_type_picker.h b/call/payload_type_picker.h index 0b2141c7e3..5a41c8db60 100644 --- a/call/payload_type_picker.h +++ b/call/payload_type_picker.h @@ -17,8 +17,9 @@ #include #include "api/rtc_error.h" -#include "media/base/codec.h" #include "call/payload_type.h" +#include "media/base/codec.h" +#include "rtc_base/checks.h" namespace webrtc { @@ -57,11 +58,23 @@ class PayloadTypeRecorder { public: explicit PayloadTypeRecorder(PayloadTypePicker& suggester) : suggester_(suggester) {} + ~PayloadTypeRecorder() { + // Ensure consistent use of paired Disallow/ReallowRedefintion calls. + RTC_DCHECK(disallow_redefinition_level_ == 0); + } RTCError AddMapping(PayloadType payload_type, cricket::Codec codec); std::vector> GetMappings() const; RTCErrorOr LookupPayloadType(cricket::Codec codec) const; RTCErrorOr LookupCodec(PayloadType payload_type) const; + // Redefinition guard. + // In some scenarios, redefinition must be allowed between one offer/answer + // set and the next offer/answer set, but within the processing of one + // SDP, it should never be allowed. + // Implemented as a stack push/pop for convenience; if Disallow has + // been called more times than Reallow, redefinition is prohibited. + void DisallowRedefinition(); + void ReallowRedefinition(); // Transaction support. // Commit() commits previous changes. void Commit(); @@ -72,6 +85,8 @@ class PayloadTypeRecorder { PayloadTypePicker& suggester_; std::map payload_type_to_codec_; std::map checkpoint_payload_type_to_codec_; + int disallow_redefinition_level_ = 0; + std::set accepted_definitions_; }; } // namespace webrtc diff --git a/call/payload_type_picker_unittest.cc b/call/payload_type_picker_unittest.cc index 0ecb80e24a..bc5913f450 100644 --- a/call/payload_type_picker_unittest.cc +++ b/call/payload_type_picker_unittest.cc @@ -67,6 +67,24 @@ TEST(PayloadTypePicker, ModifyingPtIsIgnored) { EXPECT_EQ(result.value(), b_codec); } +TEST(PayloadTypePicker, ModifyingPtIsAnErrorIfDisallowed) { + PayloadTypePicker picker; + PayloadTypeRecorder recorder(picker); + const PayloadType a_payload_type(123); + cricket::Codec a_codec = + cricket::CreateVideoCodec(cricket::Codec::kIdNotSet, "vp8"); + cricket::Codec b_codec = + cricket::CreateVideoCodec(cricket::Codec::kIdNotSet, "vp9"); + recorder.DisallowRedefinition(); + recorder.AddMapping(a_payload_type, a_codec); + auto error = recorder.AddMapping(a_payload_type, b_codec); + EXPECT_FALSE(error.ok()); + auto result = recorder.LookupCodec(a_payload_type); + // Attempted redefinition should be ignored. + EXPECT_EQ(result.value(), a_codec); + recorder.ReallowRedefinition(); +} + TEST(PayloadTypePicker, RollbackAndCommit) { PayloadTypePicker picker; PayloadTypeRecorder recorder(picker); diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 31cd1b5d17..a60ec29d51 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -377,18 +377,28 @@ webrtc::RTCError JsepTransport::RecordPayloadTypes(bool local, webrtc::SdpType type, const ContentInfo& content) { RTC_DCHECK_RUN_ON(network_thread_); + if (local) { + local_payload_types_.DisallowRedefinition(); + } else { + remote_payload_types_.DisallowRedefinition(); + } + webrtc::RTCError result = webrtc::RTCError::OK(); for (auto codec : content.media_description()->codecs()) { - webrtc::RTCError result; if (local) { result = local_payload_types_.AddMapping(codec.id, codec); } else { result = remote_payload_types_.AddMapping(codec.id, codec); } if (!result.ok()) { - return result; + break; } } - return webrtc::RTCError::OK(); + if (local) { + local_payload_types_.ReallowRedefinition(); + } else { + remote_payload_types_.ReallowRedefinition(); + } + return result; } void JsepTransport::SetRemoteIceParameters(