Skip to content

Commit

Permalink
Use the dummy address 0.0.0.0:9 in the c= and the m= lines if the
Browse files Browse the repository at this point in the history
default connection address is a hostname candidate.

Using a FQDN in the c= line has caused an inter-op issue with Firefox
when hostname candidates are the only candidates gathered when forming
the media sections. To address this issue, we use 0.0.0.0:9 when a
hostname candidate would be used to populate the c= and the m= lines.

The SDP grammar related to ICE candidates has been moved out of RFC8445,
and is currently defined in draft-ietf-mmusic-ice-sip-sdp. A FQDN
address must not be used in the connection address attribute per the
latest draft, if the ICE agent generates local candidates. Also, the
wildcard addresses  (0.0.0.0 or ::) with port 9 are given the exception
as the connection address that will not result in an ICE mismatch. We
thus adopt the aforementioned solution after combining these
considerations.

Bug: chromium:927309, chromium:982108
Change-Id: I3df2db0f154276da39f99650289cf81baa677e74
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145280
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28547}
  • Loading branch information
Qingsi Wang authored and Commit Bot committed Jul 12, 2019
1 parent 58388aa commit 3ae59d3
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 33 deletions.
18 changes: 17 additions & 1 deletion pc/jsep_session_description.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,23 @@ void UpdateConnectionAddress(
}
rtc::SocketAddress connection_addr(ip, port);
if (rtc::IPIsUnspec(connection_addr.ipaddr()) && !hostname.empty()) {
connection_addr = rtc::SocketAddress(hostname, port);
// When a hostname candidate becomes the (default) connection address,
// we use the dummy address 0.0.0.0 and port 9 in the c= and the m= lines.
//
// We have observed in deployment that with a FQDN in a c= line, SDP parsing
// could fail in other JSEP implementations. We note that the wildcard
// addresses (0.0.0.0 or ::) with port 9 are given the exception as the
// connection address that will not result in an ICE mismatch
// (draft-ietf-mmusic-ice-sip-sdp). Also, 0.0.0.0 or :: can be used as the
// connection address in the initial offer or answer with trickle ICE
// if the offerer or answerer does not want to include the host IP address
// (draft-ietf-mmusic-trickle-ice-sip), and in particular 0.0.0.0 has been
// widely deployed for this use without outstanding compatibility issues.
// Combining the above considerations, we use 0.0.0.0 with port 9 to
// populate the c= and the m= lines. See |BuildMediaDescription| in
// webrtc_sdp.cc for the SDP generation with
// |media_desc->connection_address()|.
connection_addr = rtc::SocketAddress(kDummyAddress, kDummyPort);
}
media_desc->set_connection_address(connection_addr);
}
Expand Down
42 changes: 39 additions & 3 deletions pc/jsep_session_description_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,14 @@ TEST_F(JsepSessionDescriptionTest, AddHostnameCandidate) {
c.set_protocol(cricket::UDP_PROTOCOL_NAME);
c.set_address(rtc::SocketAddress("example.local", 1234));
c.set_type(cricket::LOCAL_PORT_TYPE);
JsepIceCandidate hostname_candidate("audio", 0, c);
const size_t audio_index = 0;
JsepIceCandidate hostname_candidate("audio", audio_index, c);
EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate));

ASSERT_NE(nullptr, jsep_desc_->description());
const auto& content = jsep_desc_->description()->contents()[0];
EXPECT_EQ("example.local:1234",
ASSERT_EQ(2u, jsep_desc_->description()->contents().size());
const auto& content = jsep_desc_->description()->contents()[audio_index];
EXPECT_EQ("0.0.0.0:9",
content.media_description()->connection_address().ToString());
}

Expand All @@ -242,6 +245,39 @@ TEST_F(JsepSessionDescriptionTest, SerializeDeserialize) {
EXPECT_EQ(sdp, parsed_sdp);
}

// Test that we can serialize a JsepSessionDescription when a hostname candidate
// is the default destination and deserialize it again. The connection address
// in the deserialized description should be the dummy address 0.0.0.0:9.
TEST_F(JsepSessionDescriptionTest, SerializeDeserializeWithHostnameCandidate) {
cricket::Candidate c;
c.set_component(cricket::ICE_CANDIDATE_COMPONENT_RTP);
c.set_protocol(cricket::UDP_PROTOCOL_NAME);
c.set_address(rtc::SocketAddress("example.local", 1234));
c.set_type(cricket::LOCAL_PORT_TYPE);
const size_t audio_index = 0;
const size_t video_index = 1;
JsepIceCandidate hostname_candidate_audio("audio", audio_index, c);
JsepIceCandidate hostname_candidate_video("video", video_index, c);
EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate_audio));
EXPECT_TRUE(jsep_desc_->AddCandidate(&hostname_candidate_video));

std::string sdp = Serialize(jsep_desc_.get());

auto parsed_jsep_desc = DeSerialize(sdp);
EXPECT_EQ(2u, parsed_jsep_desc->number_of_mediasections());

ASSERT_NE(nullptr, parsed_jsep_desc->description());
ASSERT_EQ(2u, parsed_jsep_desc->description()->contents().size());
const auto& audio_content =
parsed_jsep_desc->description()->contents()[audio_index];
const auto& video_content =
parsed_jsep_desc->description()->contents()[video_index];
EXPECT_EQ("0.0.0.0:9",
audio_content.media_description()->connection_address().ToString());
EXPECT_EQ("0.0.0.0:9",
video_content.media_description()->connection_address().ToString());
}

// Tests that we can serialize and deserialize a JsepSesssionDescription
// with candidates.
TEST_F(JsepSessionDescriptionTest, SerializeDeserializeWithCandidates) {
Expand Down
4 changes: 0 additions & 4 deletions pc/webrtc_sdp.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1468,10 +1468,6 @@ void BuildMediaDescription(const ContentInfo* content_info,
} else if (media_desc->connection_address().family() == AF_INET6) {
os << " " << kConnectionIpv6Addrtype << " "
<< media_desc->connection_address().ipaddr().ToString();
} else if (!media_desc->connection_address().hostname().empty()) {
// For hostname candidates, we use c=IN IP4 <hostname>.
os << " " << kConnectionIpv4Addrtype << " "
<< media_desc->connection_address().hostname();
} else {
os << " " << kConnectionIpv4Addrtype << " " << kDummyAddress;
}
Expand Down
25 changes: 0 additions & 25 deletions pc/webrtc_sdp_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4259,31 +4259,6 @@ TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithConnectionAddress) {
video_desc->connection_address().ToString());
}

// Test that a media description that contains a hostname connection address can
// be correctly serialized.
TEST_F(WebRtcSdpTest, SerializeAndDeserializeWithHostnameConnectionAddress) {
JsepSessionDescription expected_jsep(kDummyType);
cricket::Candidate c;
const rtc::SocketAddress hostname_addr("example.local", 1234);
audio_desc_->set_connection_address(hostname_addr);
video_desc_->set_connection_address(hostname_addr);
ASSERT_TRUE(
expected_jsep.Initialize(desc_.Clone(), kSessionId, kSessionVersion));
// Serialization.
std::string message = webrtc::SdpSerialize(expected_jsep);
// Deserialization.
JsepSessionDescription jdesc(kDummyType);
ASSERT_TRUE(SdpDeserialize(message, &jdesc));
auto audio_desc = jdesc.description()
->GetContentByName(kAudioContentName)
->media_description();
auto video_desc = jdesc.description()
->GetContentByName(kVideoContentName)
->media_description();
EXPECT_EQ(hostname_addr, audio_desc->connection_address());
EXPECT_EQ(hostname_addr, video_desc->connection_address());
}

// RFC4566 says "If a session has no meaningful name, the value "s= " SHOULD be
// used (i.e., a single space as the session name)." So we should accept that.
TEST_F(WebRtcSdpTest, DeserializeEmptySessionName) {
Expand Down

0 comments on commit 3ae59d3

Please sign in to comment.