From e283d1ca64c61cb5fa7fce1bcb2cb5fdc067f103 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 27 Mar 2020 09:56:51 +0100 Subject: [PATCH] add tcptype to prflx tcp candidates Adds the missing tcptype to prflx tcp candidates as tcptype is mandatory per RFC 6544 and if missing the candidate will contain double whitespace like this ... tcptype generation ... and will get rejected by the internal parser BUG=webrtc:11423 Change-Id: Id61babd85cf43d56e9e6f9bf30d4cc9e00f00f60 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170442 Reviewed-by: Taylor Reviewed-by: Harald Alvestrand Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#30959} --- p2p/base/connection.cc | 12 ++--------- p2p/base/p2p_transport_channel.cc | 3 +++ p2p/base/p2p_transport_channel_unittest.cc | 23 ++++++++++++++++++++++ p2p/base/tcp_port.cc | 3 ++- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 282599f0ab..0e3a228e90 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -1265,24 +1265,16 @@ void Connection::MaybeUpdateLocalCandidate(ConnectionRequest* request, const uint32_t priority = priority_attr->value(); std::string id = rtc::CreateRandomString(8); - Candidate new_local_candidate; + // Create a peer-reflexive candidate based on the local candidate. + Candidate new_local_candidate(local_candidate()); new_local_candidate.set_id(id); - new_local_candidate.set_component(local_candidate().component()); new_local_candidate.set_type(PRFLX_PORT_TYPE); - new_local_candidate.set_protocol(local_candidate().protocol()); new_local_candidate.set_address(addr->GetAddress()); new_local_candidate.set_priority(priority); - new_local_candidate.set_username(local_candidate().username()); - new_local_candidate.set_password(local_candidate().password()); - new_local_candidate.set_network_name(local_candidate().network_name()); - new_local_candidate.set_network_type(local_candidate().network_type()); new_local_candidate.set_related_address(local_candidate().address()); - new_local_candidate.set_generation(local_candidate().generation()); new_local_candidate.set_foundation(Port::ComputeFoundation( PRFLX_PORT_TYPE, local_candidate().protocol(), local_candidate().relay_protocol(), local_candidate().address())); - new_local_candidate.set_network_id(local_candidate().network_id()); - new_local_candidate.set_network_cost(local_candidate().network_cost()); // Change the local candidate of this Connection to the new prflx candidate. RTC_LOG(LS_INFO) << ToString() << ": Updating local candidate type to prflx."; diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 6937b20304..6a132a2f06 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -1008,6 +1008,9 @@ void P2PTransportChannel::OnUnknownAddress(PortInterface* port, component(), ProtoToString(proto), address, remote_candidate_priority, remote_username, remote_password, PRFLX_PORT_TYPE, remote_generation, "", network_id, network_cost); + if (proto == PROTO_TCP) { + remote_candidate.set_tcptype(TCPTYPE_ACTIVE_STR); + } // From RFC 5245, section-7.2.1.3: // The foundation of the candidate is set to an arbitrary value, different diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index c66a9f7ce0..ce78335fd9 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -1802,6 +1802,29 @@ TEST_F(P2PTransportChannelTest, TestTcpConnectionsFromActiveToPassive) { DestroyChannels(); } +// Test that tcptype is set on all candidates for a connection running over TCP. +TEST_F(P2PTransportChannelTest, TestTcpConnectionTcptypeSet) { + rtc::ScopedFakeClock clock; + ConfigureEndpoints(BLOCK_UDP_AND_INCOMING_TCP, OPEN, + PORTALLOCATOR_ENABLE_SHARED_SOCKET, + PORTALLOCATOR_ENABLE_SHARED_SOCKET); + + SetAllowTcpListen(0, false); // active. + SetAllowTcpListen(1, true); // actpass. + CreateChannels(); + + EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()), + kMediumTimeout, clock); + SIMULATED_WAIT(false, kDefaultTimeout, clock); + + EXPECT_EQ(RemoteCandidate(ep1_ch1())->tcptype(), "passive"); + EXPECT_EQ(LocalCandidate(ep1_ch1())->tcptype(), "active"); + EXPECT_EQ(RemoteCandidate(ep2_ch1())->tcptype(), "active"); + EXPECT_EQ(LocalCandidate(ep2_ch1())->tcptype(), "passive"); + + DestroyChannels(); +} + TEST_F(P2PTransportChannelTest, TestIceRoleConflict) { AddAddress(0, kPublicAddrs[0]); AddAddress(1, kPublicAddrs[1]); diff --git a/p2p/base/tcp_port.cc b/p2p/base/tcp_port.cc index e07361acf7..efbf62e496 100644 --- a/p2p/base/tcp_port.cc +++ b/p2p/base/tcp_port.cc @@ -122,7 +122,8 @@ Connection* TCPPort::CreateConnection(const Candidate& address, return NULL; } - if (address.tcptype() == TCPTYPE_ACTIVE_STR || + if ((address.tcptype() == TCPTYPE_ACTIVE_STR && + address.type() != PRFLX_PORT_TYPE) || (address.tcptype().empty() && address.address().port() == 0)) { // It's active only candidate, we should not try to create connections // for these candidates.