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 <zhihuang@webrtc.org>
Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
Cr-Original-Commit-Position: refs/heads/master@{#19842}
Cr-Mirrored-From: https://webrtc.googlesource.com/src
Cr-Mirrored-Commit: 2a5e4268f821ef7e3a0fb59bc4d40b8af04ec4f9diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc
index 565704e..ccf64d2 100644
--- a/pc/peerconnectioninterface_unittest.cc
+++ b/pc/peerconnectioninterface_unittest.cc
@@ -8,6 +8,7 @@
* be found in the AUTHORS file in the root of the source tree.
*/
+#include <algorithm>
#include <memory>
#include <sstream>
#include <string>
@@ -3831,6 +3832,39 @@
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<SessionDescriptionInterface> offer;
+ ASSERT_TRUE(DoCreateOffer(&offer, &constraints));
+ EXPECT_TRUE(DoSetRemoteDescription(std::move(offer)));
+
+ std::unique_ptr<SessionDescriptionInterface> 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/pc/webrtcsession.cc b/pc/webrtcsession.cc
index c78dbc4..0753081 100644
--- a/pc/webrtcsession.cc
+++ b/pc/webrtcsession.cc
@@ -61,8 +61,12 @@
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 @@
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<const MediaContentDescription*>(
- offer->contents()[i].description);
- const MediaContentDescription* answer_mdesc =
+ desc2->contents()[i].description);
+ const MediaContentDescription* desc1_mdesc =
static_cast<const MediaContentDescription*>(
- 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 @@
// 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/pc/webrtcsession.h b/pc/webrtcsession.h
index 686ac1e..8be828f 100644
--- a/pc/webrtcsession.h
+++ b/pc/webrtcsession.h
@@ -61,7 +61,8 @@
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/pc/webrtcsession_unittest.cc b/pc/webrtcsession_unittest.cc
index 8621965..497daff 100644
--- a/pc/webrtcsession_unittest.cc
+++ b/pc/webrtcsession_unittest.cc
@@ -70,7 +70,7 @@
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 @@
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 @@
&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 @@
&sdp);
SessionDescriptionInterface* modified_answer2 =
CreateSessionDescription(JsepSessionDescription::kAnswer, sdp, NULL);
- SetRemoteDescriptionAnswerExpectError(kMlineMismatch, modified_answer2);
+ SetRemoteDescriptionAnswerExpectError(kMlineMismatchInAnswer,
+ modified_answer2);
SetRemoteDescriptionWithoutError(answer.release());
}
@@ -3794,7 +3797,7 @@
EXPECT_TRUE(modified_answer->Initialize(answer_copy,
answer->session_id(),
answer->session_version()));
- SetLocalDescriptionAnswerExpectError(kMlineMismatch, modified_answer);
+ SetLocalDescriptionAnswerExpectError(kMlineMismatchInAnswer, modified_answer);
SetLocalDescriptionWithoutError(answer);
}
