From 8206bc0f002862ea04a7cf8cd58d19c2d95f5655 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 2 Apr 2020 12:36:38 -0700 Subject: [PATCH] Handle missing tcptype on TCP ICE candidates. Our implementation accepts TCP candidates with a missing tcptype field, treating this as a passive candidate. However, if you try to convert such a candidate to SDP and back, which chromium started to do at some point, this was resulting in an error. This CL fixes that. Bug: webrtc:11423 Change-Id: Iec48d340f421f63f2b7a16c9496ea92ccd165981 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/172020 Reviewed-by: Harald Alvestrand Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#31026} --- pc/webrtc_sdp.cc | 18 +++++++++++++++--- pc/webrtc_sdp_unittest.cc | 26 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 7846e5e389..f77327faf1 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -1111,11 +1111,14 @@ bool ParseCandidate(const std::string& message, if (!StringToProto(transport.c_str(), &protocol)) { return ParseFailed(first_line, "Unsupported transport type.", error); } + bool tcp_protocol = false; switch (protocol) { + // Supported protocols. case cricket::PROTO_UDP: + break; case cricket::PROTO_TCP: case cricket::PROTO_SSLTCP: - // Supported protocol. + tcp_protocol = true; break; default: return ParseFailed(first_line, "Unsupported transport type.", error); @@ -1172,9 +1175,14 @@ bool ParseCandidate(const std::string& message, return ParseFailed(first_line, "Invalid TCP candidate type.", error); } - if (protocol != cricket::PROTO_TCP) { + if (!tcp_protocol) { return ParseFailed(first_line, "Invalid non-TCP candidate", error); } + } else if (tcp_protocol) { + // We allow the tcptype to be missing, for backwards compatibility, + // treating it as a passive candidate. + // TODO(bugs.webrtc.org/11466): Treat a missing tcptype as an error? + tcptype = cricket::TCPTYPE_PASSIVE_STR; } // Extension @@ -2007,7 +2015,11 @@ void BuildCandidate(const std::vector& candidates, << candidate.related_address().PortAsString() << " "; } - if (candidate.protocol() == cricket::TCP_PROTOCOL_NAME) { + // Note that we allow the tcptype to be missing, for backwards + // compatibility; the implementation treats this as a passive candidate. + // TODO(bugs.webrtc.org/11466): Treat a missing tcptype as an error? + if (candidate.protocol() == cricket::TCP_PROTOCOL_NAME && + !candidate.tcptype().empty()) { os << kTcpCandidateType << " " << candidate.tcptype() << " "; } diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index b849f01864..a2ad4b8bdc 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2460,6 +2460,32 @@ TEST_F(WebRtcSdpTest, SerializeTcpCandidates) { EXPECT_EQ(std::string(kSdpTcpActiveCandidate), message); } +// Test serializing a TCP candidate that came in with a missing tcptype. This +// shouldn't happen according to the spec, but our implementation has been +// accepting this for quite some time, treating it as a passive candidate. +// +// So, we should be able to at least convert such candidates to and from SDP. +// See: bugs.webrtc.org/11423 +TEST_F(WebRtcSdpTest, ParseTcpCandidateWithoutTcptype) { + std::string missing_tcptype = + "candidate:a0+B/1 1 tcp 2130706432 192.168.1.5 9999 typ host"; + JsepIceCandidate jcandidate(kDummyMid, kDummyIndex); + EXPECT_TRUE(SdpDeserializeCandidate(missing_tcptype, &jcandidate)); + + EXPECT_EQ(std::string(cricket::TCPTYPE_PASSIVE_STR), + jcandidate.candidate().tcptype()); +} + +TEST_F(WebRtcSdpTest, ParseSslTcpCandidate) { + std::string ssltcp = + "candidate:a0+B/1 1 ssltcp 2130706432 192.168.1.5 9999 typ host tcptype " + "passive"; + JsepIceCandidate jcandidate(kDummyMid, kDummyIndex); + EXPECT_TRUE(SdpDeserializeCandidate(ssltcp, &jcandidate)); + + EXPECT_EQ(std::string("ssltcp"), jcandidate.candidate().protocol()); +} + TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithH264) { cricket::VideoCodec h264_codec("H264"); h264_codec.SetParam("profile-level-id", "42e01f");