commit 4bf52387e78e0a88c806c7e2c3646756f80e4515
author Philipp Hancke <phancke@microsoft.com> Thu May 25 14:11:36 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue May 30 10:58:27 2023
tree 05c8f8c8107a9554ac2a0c537b197d1de661fed5
parent b18463409bfa413347826da395db4bfb7fbab34e

sdp: reject BUNDLE with RTP header extension id collisions

after measurements have shown this is quite rare. Rollout is guarded by
  WebRTC-PreventBundleHeaderExtensionIdCollision
which acts as a killswitch.

BUG=webrtc:14782,chromium:1447758

Change-Id: Ib314c2c8099c05ace761710fdf0e01a77fc89f76
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/306223
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#40177}

pc/peer_connection_media_unittest.cc

diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc
index 2213db6..7ed47db 100644
--- a/pc/peer_connection_media_unittest.cc
+++ b/pc/peer_connection_media_unittest.cc
@@ -1037,13 +1037,17 @@
   EXPECT_EQ("Failed to set local answer sdp: " + expected_error_, error);
 }
 
-void RemoveVideoContent(cricket::SessionDescription* desc) {
+void RemoveVideoContentAndUnbundle(cricket::SessionDescription* desc) {
+  // Removing BUNDLE is easier than removing the content in there.
+  desc->RemoveGroupByName("BUNDLE");
   auto content_name = cricket::GetFirstVideoContent(desc)->name;
   desc->RemoveContentByName(content_name);
   desc->RemoveTransportInfoByName(content_name);
 }
 
-void RenameVideoContent(cricket::SessionDescription* desc) {
+void RenameVideoContentAndUnbundle(cricket::SessionDescription* desc) {
+  // Removing BUNDLE is easier than renaming the content in there.
+  desc->RemoveGroupByName("BUNDLE");
   auto* video_content = cricket::GetFirstVideoContent(desc);
   auto* transport_info = desc->GetTransportInfoByName(video_content->name);
   video_content->name = "video_renamed";
@@ -1072,10 +1076,10 @@
     PeerConnectionMediaInvalidMediaTest,
     Combine(Values(SdpSemantics::kPlanB_DEPRECATED, SdpSemantics::kUnifiedPlan),
             Values(std::make_tuple("remove video",
-                                   RemoveVideoContent,
+                                   RemoveVideoContentAndUnbundle,
                                    kMLinesOutOfOrder),
                    std::make_tuple("rename video",
-                                   RenameVideoContent,
+                                   RenameVideoContentAndUnbundle,
                                    kMLinesOutOfOrder),
                    std::make_tuple("reverse media sections",
                                    ReverseMediaContent,

pc/sdp_offer_answer.cc

diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index c87b6ec..cf22cf3 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -501,7 +501,7 @@
     return RTCError(
         RTCErrorType::INVALID_PARAMETER,
         "A BUNDLE group contains a codec collision for "
-        "header extension id='" +
+        "header extension id=" +
             rtc::ToString(extension.id) +
             ". The id must be the same across all bundled media descriptions");
   }
@@ -3530,9 +3530,13 @@
 
   // Validate that there are no collisions of bundled header extensions ids.
   error = ValidateBundledRtpHeaderExtensions(*sdesc->description());
-  // TODO(bugs.webrtc.org/14782): actually reject.
   RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledExtensionIds",
                         error.ok());
+  // TODO(bugs.webrtc.org/14782): remove killswitch after rollout.
+  if (!error.ok() && !pc_->trials().IsDisabled(
+                         "WebRTC-PreventBundleHeaderExtensionIdCollision")) {
+    return error;
+  }
 
   if (!pc_->ValidateBundleSettings(sdesc->description(),
                                    bundle_groups_by_mid)) {

pc/sdp_offer_answer_unittest.cc

diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
index 4c4554f..79f9457 100644
--- a/pc/sdp_offer_answer_unittest.cc
+++ b/pc/sdp_offer_answer_unittest.cc
@@ -330,7 +330,8 @@
   ASSERT_NE(desc, nullptr);
   RTCError error;
   pc->SetRemoteDescription(std::move(desc), &error);
-  EXPECT_TRUE(error.ok());
+  EXPECT_FALSE(error.ok());
+  EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER);
   EXPECT_METRIC_EQ(
       1, webrtc::metrics::NumEvents(
              "WebRTC.PeerConnection.ValidBundledExtensionIds", false));