From 36f50e8e4e3e947ad8db1179e358b11431a8f079 Mon Sep 17 00:00:00 2001 From: honghaiz Date: Wed, 1 Jun 2016 15:57:03 -0700 Subject: [PATCH] Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 Review-Url: https://codereview.webrtc.org/2018693002 Cr-Commit-Position: refs/heads/master@{#13000} --- webrtc/p2p/base/p2ptransportchannel.cc | 51 ++++++++-------- .../p2p/base/p2ptransportchannel_unittest.cc | 58 +++++++++++++++++++ webrtc/p2p/base/port.cc | 16 ++++- webrtc/p2p/base/port.h | 9 ++- webrtc/p2p/base/port_unittest.cc | 30 +++++++++- webrtc/p2p/base/relayport.cc | 2 +- webrtc/p2p/base/stunport.cc | 2 +- webrtc/p2p/base/tcpport.cc | 2 +- webrtc/p2p/base/turnport.cc | 5 +- webrtc/p2p/base/turnport.h | 2 +- 10 files changed, 140 insertions(+), 37 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index b1de3e91d3..a2fb774456 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -855,40 +855,41 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port, return false; } // Look for an existing connection with this remote address. If one is not - // found, then we can create a new connection for this address. + // found or it is found but the existing remote candidate has an older + // generation, then we can create a new connection for this address. Connection* connection = port->GetConnection(remote_candidate.address()); - if (connection != NULL) { - connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate); - - // It is not legal to try to change any of the parameters of an existing - // connection; however, the other side can send a duplicate candidate. - if (!remote_candidate.IsEquivalent(connection->remote_candidate())) { - LOG(INFO) << "Attempt to change a remote candidate." - << " Existing remote candidate: " - << connection->remote_candidate().ToString() - << "New remote candidate: " - << remote_candidate.ToString(); + if (connection == nullptr || + connection->remote_candidate().generation() < + remote_candidate.generation()) { + // Don't create a connection if this is a candidate we received in a + // message and we are not allowed to make outgoing connections. + PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port); + if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) { return false; } - } else { - PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port); - - // Don't create connection if this is a candidate we received in a - // message and we are not allowed to make outgoing connections. - if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) + Connection* connection = port->CreateConnection(remote_candidate, origin); + if (!connection) { return false; - - connection = port->CreateConnection(remote_candidate, origin); - if (!connection) - return false; - + } AddConnection(connection); - LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", (" << connections_.size() << " total)"; + return true; } - return true; + // No new connection was created. + // Check if this is a peer reflexive candidate. + connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate); + + // It is not legal to try to change any of the parameters of an existing + // connection; however, the other side can send a duplicate candidate. + if (!remote_candidate.IsEquivalent(connection->remote_candidate())) { + LOG(INFO) << "Attempt to change a remote candidate." + << " Existing remote candidate: " + << connection->remote_candidate().ToString() + << "New remote candidate: " << remote_candidate.ToString(); + } + return false; } bool P2PTransportChannel::FindConnection(Connection* connection) const { diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 47a64e1d0f..68543a5f39 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1984,6 +1984,25 @@ class P2PTransportChannelPingTest : public testing::Test, last_sent_packet_id_ = last_sent_packet_id; } + void ReceivePingOnConnection(cricket::Connection* conn, + const std::string& remote_ufrag, + int priority) { + cricket::IceMessage msg; + msg.SetType(cricket::STUN_BINDING_REQUEST); + msg.AddAttribute(new cricket::StunByteStringAttribute( + cricket::STUN_ATTR_USERNAME, + conn->local_candidate().username() + ":" + remote_ufrag)); + msg.AddAttribute(new cricket::StunUInt32Attribute( + cricket::STUN_ATTR_PRIORITY, priority)); + msg.SetTransactionID( + rtc::CreateRandomString(cricket::kStunTransactionIdLength)); + msg.AddMessageIntegrity(conn->local_candidate().password()); + msg.AddFingerprint(); + rtc::ByteBufferWriter buf; + msg.Write(&buf); + conn->OnReadPacket(buf.Data(), buf.Length(), rtc::CreatePacketTime(0)); + } + void OnReadyToSend(cricket::TransportChannel* channel) { channel_ready_to_send_ = true; } @@ -2453,6 +2472,45 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { EXPECT_EQ(conn3, ch.best_connection()); } +// Test that if a new remote candidate has the same address and port with +// an old one, it will be used to create a new connection. +TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("candidate reuse", 1, &pa); + PrepareChannel(&ch); + ch.Connect(); + ch.MaybeStartGathering(); + const std::string host_address = "1.1.1.1"; + const int port_num = 1; + + // kIceUfrag[1] is the current generation ufrag. + cricket::Candidate candidate = + CreateHostCandidate(host_address, port_num, 1, kIceUfrag[1]); + ch.AddRemoteCandidate(candidate); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, host_address, port_num); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(0u, conn1->remote_candidate().generation()); + + // Simply adding the same candidate again won't create a new connection. + ch.AddRemoteCandidate(candidate); + cricket::Connection* conn2 = GetConnectionTo(&ch, host_address, port_num); + EXPECT_EQ(conn1, conn2); + + // Update the ufrag of the candidate and add it again. + candidate.set_username(kIceUfrag[2]); + ch.AddRemoteCandidate(candidate); + conn2 = GetConnectionTo(&ch, host_address, port_num); + EXPECT_NE(conn1, conn2); + EXPECT_EQ(kIceUfrag[2], conn2->remote_candidate().username()); + EXPECT_EQ(1u, conn2->remote_candidate().generation()); + + // Verify that a ping with the new ufrag can be received on the new + // connection. + EXPECT_EQ(0, conn2->last_ping_received()); + ReceivePingOnConnection(conn2, kIceUfrag[2], 1 /* priority */); + EXPECT_TRUE(conn2->last_ping_received() > 0); +} + // When the current best connection is strong, lower-priority connections will // be pruned. Otherwise, lower-priority connections are kept. TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index b08cc4c910..0ce946582a 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -270,8 +270,19 @@ void Port::AddAddress(const rtc::SocketAddress& address, } } -void Port::AddConnection(Connection* conn) { - connections_[conn->remote_candidate().address()] = conn; +void Port::AddOrReplaceConnection(Connection* conn) { + auto ret = connections_.insert( + std::make_pair(conn->remote_candidate().address(), conn)); + // If there is a different connection on the same remote address, replace + // it with the new one and destroy the old one. + if (ret.second == false && ret.first->second != conn) { + LOG_J(LS_WARNING, this) + << "A new connection was created on an existing remote address. " + << "New remote candidate: " << conn->remote_candidate().ToString(); + ret.first->second->SignalDestroyed.disconnect(this); + ret.first->second->Destroy(); + ret.first->second = conn; + } conn->SignalDestroyed.connect(this, &Port::OnConnectionDestroyed); SignalConnectionCreated(this, conn); } @@ -688,6 +699,7 @@ void Port::OnConnectionDestroyed(Connection* conn) { connections_.find(conn->remote_candidate().address()); ASSERT(iter != connections_.end()); connections_.erase(iter); + HandleConnectionDestroyed(conn); // On the controlled side, ports time out after all connections fail. // Note: If a new connection is added after this message is posted, but it diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 704fbb7a65..21203572b1 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -316,8 +316,10 @@ class Port : public PortInterface, public rtc::MessageHandler, uint32_t relay_preference, bool final); - // Adds the given connection to the list. (Deleting removes them.) - void AddConnection(Connection* conn); + // Adds the given connection to the map keyed by the remote candidate address. + // If an existing connection has the same address, the existing one will be + // replaced and destroyed. + void AddOrReplaceConnection(Connection* conn); // Called when a packet is received from an unknown address that is not // currently a connection. If this is an authenticated STUN binding request, @@ -346,6 +348,9 @@ class Port : public PortInterface, public rtc::MessageHandler, return rtc::DSCP_NO_CHANGE; } + // Extra work to be done in subclasses when a connection is destroyed. + virtual void HandleConnectionDestroyed(Connection* conn) {} + private: void Construct(); // Called when one of our connections deletes itself. diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index 43ef084b02..099567f431 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -169,7 +169,7 @@ class TestPort : public Port { virtual Connection* CreateConnection(const Candidate& remote_candidate, CandidateOrigin origin) { Connection* conn = new ProxyConnection(this, 0, remote_candidate); - AddConnection(conn); + AddOrReplaceConnection(conn); // Set use-candidate attribute flag as this will add USE-CANDIDATE attribute // in STUN binding requests. conn->set_use_candidate_attr(true); @@ -2665,3 +2665,31 @@ TEST_F(PortTest, TestSetIceParameters) { EXPECT_EQ("ufrag2", candidate.username()); EXPECT_EQ("password2", candidate.password()); } + +TEST_F(PortTest, TestAddConnectionWithSameAddress) { + std::unique_ptr port( + CreateTestPort(kLocalAddr1, "ufrag1", "password1")); + port->PrepareAddress(); + EXPECT_EQ(1u, port->Candidates().size()); + rtc::SocketAddress address("1.1.1.1", 5000); + cricket::Candidate candidate(1, "udp", address, 0, "", "", "relay", 0, ""); + cricket::Connection* conn1 = + port->CreateConnection(candidate, Port::ORIGIN_MESSAGE); + cricket::Connection* conn_in_use = port->GetConnection(address); + EXPECT_EQ(conn1, conn_in_use); + EXPECT_EQ(0u, conn_in_use->remote_candidate().generation()); + + // Creating with a candidate with the same address again will get us a + // different connection with the new candidate. + candidate.set_generation(2); + cricket::Connection* conn2 = + port->CreateConnection(candidate, Port::ORIGIN_MESSAGE); + EXPECT_NE(conn1, conn2); + conn_in_use = port->GetConnection(address); + EXPECT_EQ(conn2, conn_in_use); + EXPECT_EQ(2u, conn_in_use->remote_candidate().generation()); + + // Make sure the new connection was not deleted. + rtc::Thread::Current()->ProcessMessages(300); + EXPECT_TRUE(port->GetConnection(address) != nullptr); +} diff --git a/webrtc/p2p/base/relayport.cc b/webrtc/p2p/base/relayport.cc index 7f05e1b1ef..df0be3919f 100644 --- a/webrtc/p2p/base/relayport.cc +++ b/webrtc/p2p/base/relayport.cc @@ -302,7 +302,7 @@ Connection* RelayPort::CreateConnection(const Candidate& address, } Connection * conn = new ProxyConnection(this, index, address); - AddConnection(conn); + AddOrReplaceConnection(conn); return conn; } diff --git a/webrtc/p2p/base/stunport.cc b/webrtc/p2p/base/stunport.cc index 9fc499ddc7..e56d454b80 100644 --- a/webrtc/p2p/base/stunport.cc +++ b/webrtc/p2p/base/stunport.cc @@ -268,7 +268,7 @@ Connection* UDPPort::CreateConnection(const Candidate& address, } Connection* conn = new ProxyConnection(this, 0, address); - AddConnection(conn); + AddOrReplaceConnection(conn); return conn; } diff --git a/webrtc/p2p/base/tcpport.cc b/webrtc/p2p/base/tcpport.cc index 5ccb8108a0..54a44249d3 100644 --- a/webrtc/p2p/base/tcpport.cc +++ b/webrtc/p2p/base/tcpport.cc @@ -162,7 +162,7 @@ Connection* TCPPort::CreateConnection(const Candidate& address, } else { conn = new TCPConnection(this, address); } - AddConnection(conn); + AddOrReplaceConnection(conn); return conn; } diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc index 31c15fb3df..2a0ff35c61 100644 --- a/webrtc/p2p/base/turnport.cc +++ b/webrtc/p2p/base/turnport.cc @@ -462,8 +462,7 @@ Connection* TurnPort::CreateConnection(const Candidate& address, for (size_t index = 0; index < Candidates().size(); ++index) { if (Candidates()[index].type() == RELAY_PORT_TYPE) { ProxyConnection* conn = new ProxyConnection(this, index, address); - conn->SignalDestroyed.connect(this, &TurnPort::OnConnectionDestroyed); - AddConnection(conn); + AddOrReplaceConnection(conn); return conn; } } @@ -1013,7 +1012,7 @@ void TurnPort::DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp) { } } -void TurnPort::OnConnectionDestroyed(Connection* conn) { +void TurnPort::HandleConnectionDestroyed(Connection* conn) { // Schedule an event to destroy TurnEntry for the connection, which is // already destroyed. const rtc::SocketAddress& remote_address = conn->remote_candidate().address(); diff --git a/webrtc/p2p/base/turnport.h b/webrtc/p2p/base/turnport.h index 461fc1304d..17d2d45181 100644 --- a/webrtc/p2p/base/turnport.h +++ b/webrtc/p2p/base/turnport.h @@ -190,6 +190,7 @@ class TurnPort : public Port { typedef std::set AttemptedServerSet; virtual void OnMessage(rtc::Message* pmsg); + virtual void HandleConnectionDestroyed(Connection* conn); bool CreateTurnClientSocket(); @@ -243,7 +244,6 @@ class TurnPort : public Port { void DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp); void ScheduleEntryDestruction(TurnEntry* entry); void CancelEntryDestruction(TurnEntry* entry); - void OnConnectionDestroyed(Connection* conn); // Destroys the connection with remote address |address|. Returns true if // a connection is found and destroyed.