From 2a5e4268f821ef7e3a0fb59bc4d40b8af04ec4f9 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Thu, 14 Sep 2017 01:15:03 -0700 Subject: [PATCH] Reject the descriptions that attempt to change the order of m= sections in current local description. When setting the descriptions, the order of m= sections would be compared against existing m= sections and an error would be returned if the order doesn't match. Previously reviewed on: https://codereview.webrtc.org/3012313002/ BUG=chromium:763842 TBR=deadbeef@webrtc.org Change-Id: I577e3424830b0a4c5ecd5524923873e30ad23d43 Reviewed-on: https://webrtc-review.googlesource.com/1200 Commit-Queue: Zhi Huang Reviewed-by: Zhi Huang Cr-Commit-Position: refs/heads/master@{#19842} --- webrtc/pc/peerconnectioninterface_unittest.cc | 34 +++++++++++ webrtc/pc/webrtcsession.cc | 59 +++++++++++++------ webrtc/pc/webrtcsession.h | 3 +- webrtc/pc/webrtcsession_unittest.cc | 13 ++-- 4 files changed, 84 insertions(+), 25 deletions(-) diff --git a/webrtc/pc/peerconnectioninterface_unittest.cc b/webrtc/pc/peerconnectioninterface_unittest.cc index 565704e935..ccf64d2415 100644 --- a/webrtc/pc/peerconnectioninterface_unittest.cc +++ b/webrtc/pc/peerconnectioninterface_unittest.cc @@ -8,6 +8,7 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include #include #include #include @@ -3831,6 +3832,39 @@ TEST_F(PeerConnectionInterfaceTest, CreateAnswerWithoutRemoteDescription) { EXPECT_TRUE(DoCreateAnswer(&answer, nullptr)); } +// Test that an error is returned if a description is applied that doesn't +// respect the order of existing media sections. +TEST_F(PeerConnectionInterfaceTest, + MediaSectionOrderEnforcedForSubsequentOffers) { + CreatePeerConnection(); + FakeConstraints constraints; + constraints.SetMandatoryReceiveAudio(true); + constraints.SetMandatoryReceiveVideo(true); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + EXPECT_TRUE(DoSetRemoteDescription(std::move(offer))); + + std::unique_ptr answer; + ASSERT_TRUE(DoCreateAnswer(&answer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(std::move(answer))); + + // A remote offer with different m=line order should be rejected. + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + std::reverse(offer->description()->contents().begin(), + offer->description()->contents().end()); + std::reverse(offer->description()->transport_infos().begin(), + offer->description()->transport_infos().end()); + EXPECT_FALSE(DoSetRemoteDescription(std::move(offer))); + + // A subsequent local offer with different m=line order should be rejected. + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + std::reverse(offer->description()->contents().begin(), + offer->description()->contents().end()); + std::reverse(offer->description()->transport_infos().begin(), + offer->description()->transport_infos().end()); + EXPECT_FALSE(DoSetLocalDescription(std::move(offer))); +} + class PeerConnectionMediaConfigTest : public testing::Test { protected: void SetUp() override { diff --git a/webrtc/pc/webrtcsession.cc b/webrtc/pc/webrtcsession.cc index c78dbc4da7..075308142f 100644 --- a/webrtc/pc/webrtcsession.cc +++ b/webrtc/pc/webrtcsession.cc @@ -61,8 +61,12 @@ const char kBundleWithoutRtcpMux[] = "RTCP-MUX must be enabled when BUNDLE " const char kCreateChannelFailed[] = "Failed to create channels."; const char kInvalidCandidates[] = "Description contains invalid candidates."; const char kInvalidSdp[] = "Invalid session description."; -const char kMlineMismatch[] = - "Offer and answer descriptions m-lines are not matching. Rejecting answer."; +const char kMlineMismatchInAnswer[] = + "The order of m-lines in answer doesn't match order in offer. Rejecting " + "answer."; +const char kMlineMismatchInSubsequentOffer[] = + "The order of m-lines in subsequent offer doesn't match order from " + "previous offer/answer."; const char kPushDownTDFailed[] = "Failed to push down transport description:"; const char kSdpWithoutDtlsFingerprint[] = @@ -136,31 +140,39 @@ IceCandidatePairType GetIceCandidatePairCounter( return kIceCandidatePairMax; } -// Compares |answer| against |offer|. Comparision is done -// for number of m-lines in answer against offer. If matches true will be -// returned otherwise false. -static bool VerifyMediaDescriptions( - const SessionDescription* answer, const SessionDescription* offer) { - if (offer->contents().size() != answer->contents().size()) +// 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) { return false; - - for (size_t i = 0; i < offer->contents().size(); ++i) { - if ((offer->contents()[i].name) != answer->contents()[i].name) { + } + for (size_t i = 0; + i < desc1->contents().size() && i < desc2->contents().size(); ++i) { + if ((desc2->contents()[i].name) != desc1->contents()[i].name) { return false; } - const MediaContentDescription* offer_mdesc = + const MediaContentDescription* desc2_mdesc = static_cast( - offer->contents()[i].description); - const MediaContentDescription* answer_mdesc = + desc2->contents()[i].description); + const MediaContentDescription* desc1_mdesc = static_cast( - answer->contents()[i].description); - if (offer_mdesc->type() != answer_mdesc->type()) { + desc1->contents()[i].description); + if (desc2_mdesc->type() != desc1_mdesc->type()) { return false; } } return true; } +static bool MediaSectionsHaveSameCount(const SessionDescription* desc1, + const SessionDescription* desc2) { + if (!desc1 || !desc2) { + return false; + } + return desc1->contents().size() == desc2->contents().size(); +} + // Checks that each non-rejected content has SDES crypto keys or a DTLS // fingerprint, unless it's in a BUNDLE group, in which case only the // BUNDLE-tag section (first media section/description in the BUNDLE group) @@ -2153,12 +2165,21 @@ bool WebRtcSession::ValidateSessionDescription( // m-lines that do not rtcp-mux enabled. // Verify m-lines in Answer when compared against Offer. - if (action == kAnswer) { + if (action == kAnswer || action == kPrAnswer) { const cricket::SessionDescription* offer_desc = (source == cricket::CS_LOCAL) ? remote_description()->description() : local_description()->description(); - if (!VerifyMediaDescriptions(sdesc->description(), offer_desc)) { - return BadAnswerSdp(source, kMlineMismatch, err_desc); + if (!MediaSectionsHaveSameCount(sdesc->description(), offer_desc) || + !MediaSectionsInSameOrder(sdesc->description(), offer_desc)) { + return BadAnswerSdp(source, kMlineMismatchInAnswer, err_desc); + } + } else { + // The re-offers should respect the order of m= sections in current local + // description. See RFC3264 Section 8 paragraph 4 for more details. + if (local_description() && + !MediaSectionsInSameOrder(sdesc->description(), + local_description()->description())) { + return BadOfferSdp(source, kMlineMismatchInSubsequentOffer, err_desc); } } diff --git a/webrtc/pc/webrtcsession.h b/webrtc/pc/webrtcsession.h index 686ac1efe2..8be828fc6c 100644 --- a/webrtc/pc/webrtcsession.h +++ b/webrtc/pc/webrtcsession.h @@ -61,7 +61,8 @@ extern const char kBundleWithoutRtcpMux[]; extern const char kCreateChannelFailed[]; extern const char kInvalidCandidates[]; extern const char kInvalidSdp[]; -extern const char kMlineMismatch[]; +extern const char kMlineMismatchInAnswer[]; +extern const char kMlineMismatchInSubsequentOffer[]; extern const char kPushDownTDFailed[]; extern const char kSdpWithoutDtlsFingerprint[]; extern const char kSdpWithoutSdesCrypto[]; diff --git a/webrtc/pc/webrtcsession_unittest.cc b/webrtc/pc/webrtcsession_unittest.cc index 862196557b..497daffc32 100644 --- a/webrtc/pc/webrtcsession_unittest.cc +++ b/webrtc/pc/webrtcsession_unittest.cc @@ -70,7 +70,7 @@ using webrtc::WebRtcSession; using webrtc::kBundleWithoutRtcpMux; using webrtc::kCreateChannelFailed; using webrtc::kInvalidSdp; -using webrtc::kMlineMismatch; +using webrtc::kMlineMismatchInAnswer; using webrtc::kPushDownTDFailed; using webrtc::kSdpWithoutIceUfragPwd; using webrtc::kSdpWithoutDtlsFingerprint; @@ -3747,7 +3747,8 @@ TEST_F(WebRtcSessionTest, TestIncorrectMLinesInRemoteAnswer) { EXPECT_TRUE(modified_answer->Initialize(answer_copy, answer->session_id(), answer->session_version())); - SetRemoteDescriptionAnswerExpectError(kMlineMismatch, modified_answer); + SetRemoteDescriptionAnswerExpectError(kMlineMismatchInAnswer, + modified_answer); // Different content names. std::string sdp; @@ -3760,7 +3761,8 @@ TEST_F(WebRtcSessionTest, TestIncorrectMLinesInRemoteAnswer) { &sdp); SessionDescriptionInterface* modified_answer1 = CreateSessionDescription(JsepSessionDescription::kAnswer, sdp, NULL); - SetRemoteDescriptionAnswerExpectError(kMlineMismatch, modified_answer1); + SetRemoteDescriptionAnswerExpectError(kMlineMismatchInAnswer, + modified_answer1); // Different media types. EXPECT_TRUE(answer->ToString(&sdp)); @@ -3772,7 +3774,8 @@ TEST_F(WebRtcSessionTest, TestIncorrectMLinesInRemoteAnswer) { &sdp); SessionDescriptionInterface* modified_answer2 = CreateSessionDescription(JsepSessionDescription::kAnswer, sdp, NULL); - SetRemoteDescriptionAnswerExpectError(kMlineMismatch, modified_answer2); + SetRemoteDescriptionAnswerExpectError(kMlineMismatchInAnswer, + modified_answer2); SetRemoteDescriptionWithoutError(answer.release()); } @@ -3794,7 +3797,7 @@ TEST_F(WebRtcSessionTest, TestIncorrectMLinesInLocalAnswer) { EXPECT_TRUE(modified_answer->Initialize(answer_copy, answer->session_id(), answer->session_version())); - SetLocalDescriptionAnswerExpectError(kMlineMismatch, modified_answer); + SetLocalDescriptionAnswerExpectError(kMlineMismatchInAnswer, modified_answer); SetLocalDescriptionWithoutError(answer); }