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 <orphis@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43318}

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",

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 "
+                          << existing_codec_it->second << " to "
-                          << " new codec " << codec.ToString();
+                          << " 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<cricket::Codec> 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_;
 }

call/payload_type_picker.h

@@ -17,8 +17,9 @@
 #include <vector>
 
 #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<std::pair<PayloadType, cricket::Codec>> GetMappings() const;
   RTCErrorOr<PayloadType> LookupPayloadType(cricket::Codec codec) const;
   RTCErrorOr<cricket::Codec> 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<PayloadType, cricket::Codec> payload_type_to_codec_;
   std::map<PayloadType, cricket::Codec> checkpoint_payload_type_to_codec_;
+  int disallow_redefinition_level_ = 0;
+  std::set<PayloadType> accepted_definitions_;
 };
 
 }  // namespace webrtc

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);

pc/jsep_transport.cc

@@ -377,19 +377,29 @@ 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()) {
+      break;
+    }
+  }
+  if (local) {
+    local_payload_types_.ReallowRedefinition();
+  } else {
+    remote_payload_types_.ReallowRedefinition();
+  }
   return result;
 }
-  }
-  return webrtc::RTCError::OK();
-}
 
 void JsepTransport::SetRemoteIceParameters(
     const IceParameters& ice_parameters,