From b2d355ed1f978e0f25c3998a61d79877f622d601 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Thu, 26 Oct 2017 17:26:37 -0700 Subject: [PATCH] Reland: Reject the description with fewer m= sections. If the subsequent offer contains fewer m= sections than the existing description, it would be rejected. The helper method MediaSectionsInSameOrder is modified and it will compare the number of m= sections before matching the media type. The original CL: https://webrtc-review.googlesource.com/c/src/+/9621 Reland it after the web-platform-tests are updated: https://chromium-review.googlesource.com/c/chromium/src/+/736318 TBR=deadbeef@webrtc.org Bug: chromium:773620 Change-Id: I60e972eb856efc3cef4a18777791053c9f8e0491 Reviewed-on: https://webrtc-review.googlesource.com/16382 Reviewed-by: Zhi Huang Commit-Queue: Zhi Huang Cr-Commit-Position: refs/heads/master@{#20455} --- pc/peerconnectioninterface_unittest.cc | 31 +++++++++++++++++ pc/webrtcsession.cc | 48 ++++++++++++++++---------- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index d8fa486e18..b0423a365e 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -3672,6 +3672,37 @@ TEST_F(PeerConnectionInterfaceTest, MediaStreamAddTrackRemoveTrackRenegotiate) { observer_.renegotiation_needed_ = false; } +// Tests that an error is returned if a description is applied that has fewer +// media sections than the existing description. +TEST_F(PeerConnectionInterfaceTest, + MediaSectionCountEnforcedForSubsequentOffer) { + CreatePeerConnection(); + FakeConstraints constraints; + constraints.SetMandatoryReceiveAudio(true); + constraints.SetMandatoryReceiveVideo(true); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + EXPECT_TRUE(DoSetRemoteDescription(std::move(offer))); + + // A remote offer with fewer media sections should be rejected. + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + offer->description()->contents().pop_back(); + offer->description()->contents().pop_back(); + ASSERT_TRUE(offer->description()->contents().empty()); + EXPECT_FALSE(DoSetRemoteDescription(std::move(offer))); + + std::unique_ptr answer; + ASSERT_TRUE(DoCreateAnswer(&answer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(std::move(answer))); + + // A subsequent local offer with fewer media sections should be rejected. + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + offer->description()->contents().pop_back(); + offer->description()->contents().pop_back(); + ASSERT_TRUE(offer->description()->contents().empty()); + EXPECT_FALSE(DoSetLocalDescription(std::move(offer))); +} + class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/pc/webrtcsession.cc b/pc/webrtcsession.cc index 66b515c5ea..ebdcf4ebfe 100644 --- a/pc/webrtcsession.cc +++ b/pc/webrtcsession.cc @@ -137,25 +137,30 @@ IceCandidatePairType GetIceCandidatePairCounter( return kIceCandidatePairMax; } -// Verify that the order of media sections in |desc1| matches |desc2|. The -// number of m= sections could be different. -static bool MediaSectionsInSameOrder(const SessionDescription* desc1, - const SessionDescription* desc2) { - if (!desc1 || !desc2) { +// Verify that the order of media sections in |new_desc| matches +// |existing_desc|. The number of m= sections in |new_desc| should be no less +// than |existing_desc|. +static bool MediaSectionsInSameOrder(const SessionDescription* existing_desc, + const SessionDescription* new_desc) { + if (!existing_desc || !new_desc) { return false; } - for (size_t i = 0; - i < desc1->contents().size() && i < desc2->contents().size(); ++i) { - if ((desc2->contents()[i].name) != desc1->contents()[i].name) { + + if (existing_desc->contents().size() > new_desc->contents().size()) { + return false; + } + + for (size_t i = 0; i < existing_desc->contents().size(); ++i) { + if (new_desc->contents()[i].name != existing_desc->contents()[i].name) { return false; } - const MediaContentDescription* desc2_mdesc = + const MediaContentDescription* new_desc_mdesc = static_cast( - desc2->contents()[i].description); - const MediaContentDescription* desc1_mdesc = + new_desc->contents()[i].description); + const MediaContentDescription* existing_desc_mdesc = static_cast( - desc1->contents()[i].description); - if (desc2_mdesc->type() != desc1_mdesc->type()) { + existing_desc->contents()[i].description); + if (new_desc_mdesc->type() != existing_desc_mdesc->type()) { return false; } } @@ -2123,16 +2128,21 @@ bool WebRtcSession::ValidateSessionDescription( const cricket::SessionDescription* offer_desc = (source == cricket::CS_LOCAL) ? remote_description()->description() : local_description()->description(); - if (!MediaSectionsHaveSameCount(sdesc->description(), offer_desc) || - !MediaSectionsInSameOrder(sdesc->description(), offer_desc)) { + if (!MediaSectionsHaveSameCount(offer_desc, sdesc->description()) || + !MediaSectionsInSameOrder(offer_desc, sdesc->description())) { return BadAnswerSdp(source, kMlineMismatchInAnswer, err_desc); } } else { - // The re-offers should respect the order of m= sections in current local + const cricket::SessionDescription* current_desc = nullptr; + if (source == cricket::CS_LOCAL && local_description()) { + current_desc = local_description()->description(); + } else if (source == cricket::CS_REMOTE && remote_description()) { + current_desc = remote_description()->description(); + } + // The re-offers should respect the order of m= sections in current // description. See RFC3264 Section 8 paragraph 4 for more details. - if (local_description() && - !MediaSectionsInSameOrder(sdesc->description(), - local_description()->description())) { + if (current_desc && + !MediaSectionsInSameOrder(current_desc, sdesc->description())) { return BadOfferSdp(source, kMlineMismatchInSubsequentOffer, err_desc); } }