From d2248f82d331d11e3661e221ec73ca94bbc7b816 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Tue, 10 Apr 2018 14:41:03 -0700 Subject: [PATCH] Handle the corner cases for BUNDLE. Reject the local/remote description trying to change the pre-negotiated BUNDLE tag. Reject an answer containing a BUNDLE group that's not a subset of the offered group. Reject an offer/answer with a BUNDLE group containing a MID that no m= section has. Reject an answer removes an m= section from an established BUNDLE group without rejecting it. Bug: chromium:827917 Change-Id: If334eefb00b1c1c1e24f9afba0cb00b5867f5590 Reviewed-on: https://webrtc-review.googlesource.com/67190 Commit-Queue: Zhi Huang Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22813} --- media/sctp/sctptransport.h | 2 +- pc/jseptransportcontroller.cc | 195 +++++++++++++++++-------- pc/jseptransportcontroller.h | 20 ++- pc/jseptransportcontroller_unittest.cc | 86 ++++++++++- pc/peerconnection_bundle_unittest.cc | 140 ++++++++++++++++++ pc/peerconnection_media_unittest.cc | 8 + 6 files changed, 388 insertions(+), 63 deletions(-) diff --git a/media/sctp/sctptransport.h b/media/sctp/sctptransport.h index 25f2f567e7..45132e2414 100644 --- a/media/sctp/sctptransport.h +++ b/media/sctp/sctptransport.h @@ -140,7 +140,7 @@ class SctpTransport : public SctpTransportInternal, // Helps pass inbound/outbound packets asynchronously to the network thread. rtc::AsyncInvoker invoker_; // Underlying DTLS channel. - rtc::PacketTransportInternal* transport_; + rtc::PacketTransportInternal* transport_ = nullptr; bool was_ever_writable_ = false; int local_port_ = kSctpDefaultPort; int remote_port_ = kSctpDefaultPort; diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 1a1fce8366..31d27cae4d 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -543,17 +543,8 @@ RTCError JsepTransportController::ApplyDescription_n( remote_desc_ = description; } - if (ShouldUpdateBundleGroup(type, description)) { - if (!description->HasGroup(cricket::GROUP_TYPE_BUNDLE)) { - return RTCError(RTCErrorType::INVALID_PARAMETER, - "max-bundle is used but no bundle group found."); - } else { - bundle_group_ = *description->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); - } - } - RTCError error; - error = ValidateBundleGroup(description); + error = ValidateAndMaybeUpdateBundleGroup(local, type, description); if (!error.ok()) { return error; } @@ -570,7 +561,7 @@ RTCError JsepTransportController::ApplyDescription_n( (IsBundled(content_info.name) && content_info.name != *bundled_mid())) { continue; } - error = MaybeCreateJsepTransport(content_info.name, content_info); + error = MaybeCreateJsepTransport(content_info); if (!error.ok()) { return error; } @@ -583,7 +574,7 @@ RTCError JsepTransportController::ApplyDescription_n( const cricket::TransportInfo& transport_info = description->transport_infos()[i]; if (content_info.rejected) { - HandleRejectedContent(content_info); + HandleRejectedContent(content_info, description); continue; } @@ -633,9 +624,75 @@ RTCError JsepTransportController::ApplyDescription_n( return RTCError::OK(); } -RTCError JsepTransportController::ValidateBundleGroup( +RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( + bool local, + SdpType type, const cricket::SessionDescription* description) { RTC_DCHECK(description); + const cricket::ContentGroup* new_bundle_group = + description->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + + // The BUNDLE group containing a MID that no m= section has is invalid. + if (new_bundle_group) { + for (auto content_name : new_bundle_group->content_names()) { + if (!description->GetContentByName(content_name)) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The BUNDLE group contains MID:" + content_name + + " matching no m= section."); + } + } + } + + if (type == SdpType::kAnswer) { + const cricket::ContentGroup* offered_bundle_group = + local ? remote_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE) + : local_desc_->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + + if (new_bundle_group) { + // The BUNDLE group in answer should be a subset of offered group. + for (auto content_name : new_bundle_group->content_names()) { + if (!offered_bundle_group || + !offered_bundle_group->HasContentName(content_name)) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The BUNDLE group in answer contains a MID that was " + "not in the offered group."); + } + } + } + + if (bundle_group_) { + for (auto content_name : bundle_group_->content_names()) { + // An answer that removes m= sections from pre-negotiated BUNDLE group + // without rejecting it, is invalid. + if (!new_bundle_group || + !new_bundle_group->HasContentName(content_name)) { + auto* content_info = description->GetContentByName(content_name); + if (!content_info || !content_info->rejected) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Answer cannot remove m= section " + content_name + + " from already-established BUNDLE group."); + } + } + } + } + } + + if (config_.bundle_policy == + PeerConnectionInterface::kBundlePolicyMaxBundle && + !description->HasGroup(cricket::GROUP_TYPE_BUNDLE)) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "max-bundle is used but no bundle group found."); + } + + if (ShouldUpdateBundleGroup(type, description)) { + std::string new_bundled_mid = *(new_bundle_group->FirstContentName()); + if (bundled_mid() && *bundled_mid() != new_bundled_mid) { + return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, + "Changing the negotiated BUNDLE-tag is not supported."); + } + + bundle_group_ = *new_bundle_group; + } if (!bundled_mid()) { return RTCError::OK(); @@ -679,24 +736,20 @@ RTCError JsepTransportController::ValidateContent( } void JsepTransportController::HandleRejectedContent( - const cricket::ContentInfo& content_info) { + const cricket::ContentInfo& content_info, + const cricket::SessionDescription* description) { // If the content is rejected, let the // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first, // then destroy the cricket::JsepTransport2. - if (content_info.type == cricket::MediaProtocolType::kRtp) { - SignalRtpTransportChanged(content_info.name, nullptr); - } else { - SignalDtlsTransportChanged(content_info.name, nullptr); - } + RemoveTransportForMid(content_info.name, content_info.type); // If the answerer rejects the first content, which other contents are bundled // on, all the other contents in the bundle group will be rejected. if (content_info.name == bundled_mid()) { for (auto content_name : bundle_group_->content_names()) { - if (content_info.type == cricket::MediaProtocolType::kRtp) { - SignalRtpTransportChanged(content_name, nullptr); - } else { - SignalDtlsTransportChanged(content_name, nullptr); - } + const cricket::ContentInfo* content_in_group = + description->GetContentByName(content_name); + RTC_DCHECK(content_in_group); + RemoveTransportForMid(content_name, content_in_group->type); } bundle_group_.reset(); } else if (IsBundled(content_info.name)) { @@ -712,21 +765,42 @@ void JsepTransportController::HandleRejectedContent( void JsepTransportController::HandleBundledContent( const cricket::ContentInfo& content_info) { + auto jsep_transport = GetJsepTransportByName(*bundled_mid()); + RTC_DCHECK(jsep_transport); // If the content is bundled, let the // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first, // then destroy the cricket::JsepTransport2. - if (content_info.type == cricket::MediaProtocolType::kRtp) { - auto rtp_transport = - jsep_transports_by_name_[*bundled_mid()]->rtp_transport(); - SignalRtpTransportChanged(content_info.name, rtp_transport); - } else { - auto dtls_transport = - jsep_transports_by_name_[*bundled_mid()]->rtp_dtls_transport(); - SignalDtlsTransportChanged(content_info.name, dtls_transport); - } + SetTransportForMid(content_info.name, jsep_transport, content_info.type); MaybeDestroyJsepTransport(content_info.name); } +void JsepTransportController::SetTransportForMid( + const std::string& mid, + cricket::JsepTransport2* jsep_transport, + cricket::MediaProtocolType protocol_type) { + if (mid_to_transport_[mid] == jsep_transport) { + return; + } + + mid_to_transport_[mid] = jsep_transport; + if (protocol_type == cricket::MediaProtocolType::kRtp) { + SignalRtpTransportChanged(mid, jsep_transport->rtp_transport()); + } else { + SignalDtlsTransportChanged(mid, jsep_transport->rtp_dtls_transport()); + } +} + +void JsepTransportController::RemoveTransportForMid( + const std::string& mid, + cricket::MediaProtocolType protocol_type) { + if (protocol_type == cricket::MediaProtocolType::kRtp) { + SignalRtpTransportChanged(mid, nullptr); + } else { + SignalDtlsTransportChanged(mid, nullptr); + } + mid_to_transport_.erase(mid); +} + cricket::JsepTransportDescription JsepTransportController::CreateJsepTransportDescription( cricket::ContentInfo content_info, @@ -832,22 +906,14 @@ int JsepTransportController::GetRtpAbsSendTimeHeaderExtensionId( const cricket::JsepTransport2* JsepTransportController::GetJsepTransportForMid( const std::string& mid) const { - auto target_mid = mid; - if (IsBundled(mid)) { - target_mid = *bundled_mid(); - } - auto it = jsep_transports_by_name_.find(target_mid); - return (it == jsep_transports_by_name_.end()) ? nullptr : it->second.get(); + auto it = mid_to_transport_.find(mid); + return it == mid_to_transport_.end() ? nullptr : it->second; } cricket::JsepTransport2* JsepTransportController::GetJsepTransportForMid( const std::string& mid) { - auto target_mid = mid; - if (IsBundled(mid)) { - target_mid = *bundled_mid(); - } - auto it = jsep_transports_by_name_.find(target_mid); - return (it == jsep_transports_by_name_.end()) ? nullptr : it->second.get(); + auto it = mid_to_transport_.find(mid); + return it == mid_to_transport_.end() ? nullptr : it->second; } const cricket::JsepTransport2* JsepTransportController::GetJsepTransportByName( @@ -863,10 +929,10 @@ cricket::JsepTransport2* JsepTransportController::GetJsepTransportByName( } RTCError JsepTransportController::MaybeCreateJsepTransport( - const std::string& mid, const cricket::ContentInfo& content_info) { RTC_DCHECK(network_thread_->IsCurrent()); - cricket::JsepTransport2* transport = GetJsepTransportForMid(mid); + cricket::JsepTransport2* transport = + GetJsepTransportByName(content_info.name); if (transport) { return RTCError::OK(); } @@ -880,12 +946,13 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( } std::unique_ptr rtp_dtls_transport = - CreateDtlsTransport(mid, /*rtcp =*/false); + CreateDtlsTransport(content_info.name, /*rtcp =*/false); std::unique_ptr rtcp_dtls_transport; if (config_.rtcp_mux_policy != PeerConnectionInterface::kRtcpMuxPolicyRequire && content_info.type == cricket::MediaProtocolType::kRtp) { - rtcp_dtls_transport = CreateDtlsTransport(mid, /*rtcp =*/true); + rtcp_dtls_transport = + CreateDtlsTransport(content_info.name, /*rtcp =*/true); } std::unique_ptr unencrypted_rtp_transport; @@ -893,30 +960,44 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( std::unique_ptr dtls_srtp_transport; if (config_.disable_encryption) { unencrypted_rtp_transport = CreateUnencryptedRtpTransport( - mid, rtp_dtls_transport.get(), rtcp_dtls_transport.get()); + content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get()); } else if (!content_desc->cryptos().empty()) { - sdes_transport = CreateSdesTransport(mid, rtp_dtls_transport.get(), - rtcp_dtls_transport.get()); + sdes_transport = CreateSdesTransport( + content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get()); } else { - dtls_srtp_transport = CreateDtlsSrtpTransport(mid, rtp_dtls_transport.get(), - rtcp_dtls_transport.get()); + dtls_srtp_transport = CreateDtlsSrtpTransport( + content_info.name, rtp_dtls_transport.get(), rtcp_dtls_transport.get()); } std::unique_ptr jsep_transport = rtc::MakeUnique( - mid, certificate_, std::move(unencrypted_rtp_transport), + content_info.name, certificate_, std::move(unencrypted_rtp_transport), std::move(sdes_transport), std::move(dtls_srtp_transport), std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport)); jsep_transport->SignalRtcpMuxActive.connect( this, &JsepTransportController::UpdateAggregateStates_n); - jsep_transports_by_name_[mid] = std::move(jsep_transport); - UpdateAggregateStates_n(); + SetTransportForMid(content_info.name, jsep_transport.get(), + content_info.type); + jsep_transports_by_name_[content_info.name] = std::move(jsep_transport); + UpdateAggregateStates_n(); return RTCError::OK(); } void JsepTransportController::MaybeDestroyJsepTransport( const std::string& mid) { + auto jsep_transport = GetJsepTransportByName(mid); + if (!jsep_transport) { + return; + } + + // Don't destroy the JsepTransport if there are still media sections referring + // to it. + for (const auto& kv : mid_to_transport_) { + if (kv.second == jsep_transport) { + return; + } + } jsep_transports_by_name_.erase(mid); UpdateAggregateStates_n(); } diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index f533cf8179..794175a6e2 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -177,12 +177,22 @@ class JsepTransportController : public sigslot::has_slots<>, RTCError ApplyDescription_n(bool local, SdpType type, const cricket::SessionDescription* description); - RTCError ValidateBundleGroup(const cricket::SessionDescription* description); + RTCError ValidateAndMaybeUpdateBundleGroup( + bool local, + SdpType type, + const cricket::SessionDescription* description); RTCError ValidateContent(const cricket::ContentInfo& content_info); - void HandleRejectedContent(const cricket::ContentInfo& content_info); + void HandleRejectedContent(const cricket::ContentInfo& content_info, + const cricket::SessionDescription* description); void HandleBundledContent(const cricket::ContentInfo& content_info); + void SetTransportForMid(const std::string& mid, + cricket::JsepTransport2* jsep_transport, + cricket::MediaProtocolType protocol_type); + void RemoveTransportForMid(const std::string& mid, + cricket::MediaProtocolType protocol_type); + cricket::JsepTransportDescription CreateJsepTransportDescription( cricket::ContentInfo content_info, cricket::TransportInfo transport_info, @@ -227,8 +237,7 @@ class JsepTransportController : public sigslot::has_slots<>, cricket::JsepTransport2* GetJsepTransportByName( const std::string& transport_name); - RTCError MaybeCreateJsepTransport(const std::string& mid, - const cricket::ContentInfo& content_info); + RTCError MaybeCreateJsepTransport(const cricket::ContentInfo& content_info); void MaybeDestroyJsepTransport(const std::string& mid); void DestroyAllJsepTransports_n(); @@ -284,6 +293,9 @@ class JsepTransportController : public sigslot::has_slots<>, std::map> jsep_transports_by_name_; + // This keeps track of the mapping between media section + // (BaseChannel/SctpTransport) and the JsepTransport2 underneath. + std::map mid_to_transport_; // Aggregate state for Transports. cricket::IceConnectionState ice_connection_state_ = diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc index 73af35d18c..16a829f2dc 100644 --- a/pc/jseptransportcontroller_unittest.cc +++ b/pc/jseptransportcontroller_unittest.cc @@ -1053,7 +1053,7 @@ TEST_F(JsepTransportControllerTest, BundleSubsetOfMediaSections) { ASSERT_TRUE(it != changed_rtp_transport_by_mid_.end()); EXPECT_EQ(transport1, it->second); it = changed_rtp_transport_by_mid_.find(kAudioMid2); - EXPECT_TRUE(it == changed_rtp_transport_by_mid_.end()); + EXPECT_TRUE(transport2 == it->second); } // Tests that the initial offer/answer only have data section and audio/video @@ -1325,4 +1325,88 @@ TEST_F(JsepTransportControllerTest, ApplyNonRtcpMuxAnswerWhenMuxingRequired) { .ok()); } +// This tests that the BUNDLE group in answer should be a subset of the offered +// group. +TEST_F(JsepTransportControllerTest, + AddContentToBundleGroupInAnswerNotSupported) { + CreateJsepTransportController(JsepTransportController::Config()); + auto local_offer = CreateSessionDescriptionWithoutBundle(); + auto remote_answer = CreateSessionDescriptionWithoutBundle(); + + cricket::ContentGroup offer_bundle_group(cricket::GROUP_TYPE_BUNDLE); + offer_bundle_group.AddContentName(kAudioMid1); + local_offer->AddGroup(offer_bundle_group); + + cricket::ContentGroup answer_bundle_group(cricket::GROUP_TYPE_BUNDLE); + answer_bundle_group.AddContentName(kAudioMid1); + answer_bundle_group.AddContentName(kVideoMid1); + remote_answer->AddGroup(answer_bundle_group); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_FALSE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); +} + +// This tests that the BUNDLE group with non-existing MID should be rejectd. +TEST_F(JsepTransportControllerTest, RejectBundleGroupWithNonExistingMid) { + CreateJsepTransportController(JsepTransportController::Config()); + auto local_offer = CreateSessionDescriptionWithoutBundle(); + auto remote_answer = CreateSessionDescriptionWithoutBundle(); + + cricket::ContentGroup invalid_bundle_group(cricket::GROUP_TYPE_BUNDLE); + // The BUNDLE group is invalid because there is no data section in the + // description. + invalid_bundle_group.AddContentName(kDataMid1); + local_offer->AddGroup(invalid_bundle_group); + remote_answer->AddGroup(invalid_bundle_group); + + EXPECT_FALSE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_FALSE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); +} + +// This tests that an answer shouldn't be able to remove an m= section from an +// established group without rejecting it. +TEST_F(JsepTransportControllerTest, RemoveContentFromBundleGroup) { + CreateJsepTransportController(JsepTransportController::Config()); + + auto local_offer = CreateSessionDescriptionWithBundleGroup(); + auto remote_answer = CreateSessionDescriptionWithBundleGroup(); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + // Do an re-offer/answer. + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + auto new_answer = CreateSessionDescriptionWithoutBundle(); + cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE); + // The answer removes video from the BUNDLE group without rejecting it is + // invalid. + new_bundle_group.AddContentName(kAudioMid1); + new_answer->AddGroup(new_bundle_group); + + // Applying invalid answer is expected to fail. + EXPECT_FALSE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, new_answer.get()) + .ok()); + + // Rejected the video content. + auto video_content = new_answer->GetContentByName(kVideoMid1); + ASSERT_TRUE(video_content); + video_content->rejected = true; + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, new_answer.get()) + .ok()); +} + } // namespace webrtc diff --git a/pc/peerconnection_bundle_unittest.cc b/pc/peerconnection_bundle_unittest.cc index 98191153e9..c71662bda0 100644 --- a/pc/peerconnection_bundle_unittest.cc +++ b/pc/peerconnection_bundle_unittest.cc @@ -659,6 +659,146 @@ TEST_P(PeerConnectionBundleTest, BundleOnFirstMidInAnswer) { EXPECT_EQ(caller->voice_rtp_transport(), caller->video_rtp_transport()); } +// This tests that changing the pre-negotiated BUNDLE tag is not supported. +TEST_P(PeerConnectionBundleTest, RejectDescriptionChangingBundleTag) { + RTCConfiguration config; + config.bundle_policy = BundlePolicy::kBundlePolicyMaxBundle; + auto caller = CreatePeerConnectionWithAudioVideo(config); + auto callee = CreatePeerConnectionWithAudioVideo(config); + + RTCOfferAnswerOptions options; + options.use_rtp_mux = true; + auto offer = caller->CreateOfferAndSetAsLocal(options); + + // Create a new bundle-group with different bundled_mid. + auto* old_bundle_group = + offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + std::string first_mid = old_bundle_group->content_names()[0]; + std::string second_mid = old_bundle_group->content_names()[1]; + cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group.AddContentName(second_mid); + + auto re_offer = CloneSessionDescription(offer.get()); + callee->SetRemoteDescription(std::move(offer)); + auto answer = callee->CreateAnswer(options); + // Reject the first MID. + answer->description()->contents()[0].rejected = true; + // Remove the first MID from the bundle group. + answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + answer->description()->AddGroup(new_bundle_group); + // The answer is expected to be rejected. + EXPECT_FALSE(caller->SetRemoteDescription(std::move(answer))); + + // Do the same thing for re-offer. + re_offer->description()->contents()[0].rejected = true; + re_offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + re_offer->description()->AddGroup(new_bundle_group); + // The re-offer is expected to be rejected. + EXPECT_FALSE(caller->SetLocalDescription(std::move(re_offer))); +} + +// This tests that removing contents from BUNDLE group and reject the whole +// BUNDLE group could work. This is a regression test for +// (https://bugs.chromium.org/p/chromium/issues/detail?id=827917) +TEST_P(PeerConnectionBundleTest, RemovingContentAndRejectBundleGroup) { + RTCConfiguration config; +#ifndef HAVE_SCTP + config.enable_rtp_data_channel = true; +#endif + config.bundle_policy = BundlePolicy::kBundlePolicyMaxBundle; + auto caller = CreatePeerConnectionWithAudioVideo(config); + caller->CreateDataChannel("dc"); + + auto offer = caller->CreateOfferAndSetAsLocal(); + auto re_offer = CloneSessionDescription(offer.get()); + + // Removing the second MID from the BUNDLE group. + auto* old_bundle_group = + offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + std::string first_mid = old_bundle_group->content_names()[0]; + std::string third_mid = old_bundle_group->content_names()[2]; + cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group.AddContentName(first_mid); + new_bundle_group.AddContentName(third_mid); + + // Reject the entire new bundle group. + re_offer->description()->contents()[0].rejected = true; + re_offer->description()->contents()[2].rejected = true; + re_offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + re_offer->description()->AddGroup(new_bundle_group); + + EXPECT_TRUE(caller->SetLocalDescription(std::move(re_offer))); +} + +// This tests that the BUNDLE group in answer should be a subset of the offered +// group. +TEST_P(PeerConnectionBundleTest, AddContentToBundleGroupInAnswerNotSupported) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + auto offer = caller->CreateOffer(); + std::string first_mid = offer->description()->contents()[0].name; + std::string second_mid = offer->description()->contents()[1].name; + + cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE); + bundle_group.AddContentName(first_mid); + offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + offer->description()->AddGroup(bundle_group); + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); + + auto answer = callee->CreateAnswer(); + bundle_group.AddContentName(second_mid); + answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + answer->description()->AddGroup(bundle_group); + + // The answer is expected to be rejected because second mid is not in the + // offered BUNDLE group. + EXPECT_FALSE(callee->SetLocalDescription(std::move(answer))); +} + +// This tests that the BUNDLE group with non-existing MID should be rejectd. +TEST_P(PeerConnectionBundleTest, RejectBundleGroupWithNonExistingMid) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + auto offer = caller->CreateOffer(); + auto invalid_bundle_group = + *offer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + invalid_bundle_group.AddContentName("non-existing-MID"); + offer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + offer->description()->AddGroup(invalid_bundle_group); + + EXPECT_FALSE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_FALSE(callee->SetRemoteDescription(std::move(offer))); +} + +// This tests that an answer shouldn't be able to remove an m= section from an +// established group without rejecting it. +TEST_P(PeerConnectionBundleTest, RemoveContentFromBundleGroup) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + EXPECT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + auto answer = callee->CreateAnswer(); + std::string second_mid = answer->description()->contents()[1].name; + + auto invalid_bundle_group = + *answer->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + invalid_bundle_group.RemoveContentName(second_mid); + answer->description()->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + answer->description()->AddGroup(invalid_bundle_group); + + EXPECT_FALSE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); +} + INSTANTIATE_TEST_CASE_P(PeerConnectionBundleTest, PeerConnectionBundleTest, Values(SdpSemantics::kPlanB, diff --git a/pc/peerconnection_media_unittest.cc b/pc/peerconnection_media_unittest.cc index 8d1dd7694f..228243c5a3 100644 --- a/pc/peerconnection_media_unittest.cc +++ b/pc/peerconnection_media_unittest.cc @@ -955,6 +955,14 @@ void RenameContent(cricket::SessionDescription* desc, auto* transport = desc->GetTransportInfoByName(old_name); RTC_DCHECK(transport); transport->content_name = new_name; + + // Rename the content name in the BUNDLE group. + cricket::ContentGroup new_bundle_group = + *desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group.RemoveContentName(old_name); + new_bundle_group.AddContentName(new_name); + desc->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + desc->AddGroup(new_bundle_group); } // Tests that an answer responds with the same MIDs as the offer.