From 372f2fcc59829f9eaa2b10519bebd0f71e915f93 Mon Sep 17 00:00:00 2001 From: Guo-wei Shieh Date: Fri, 12 Jun 2015 10:12:46 -0700 Subject: [PATCH] Connection resurrected with incorrect candidate type. Connection can be resurrected with current code when there is no any existing connection for the same address. However, it's always resurrected with prflx candidate priority hence the new connection could bump down other better connection. Migrated from https://webrtc-codereview.appspot.com/51959004/ This is based on test cases added for triggered checks. BUG=webrtc:4724 R=pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1172483002 Cr-Commit-Position: refs/heads/master@{#9429} --- webrtc/p2p/base/p2ptransportchannel.cc | 73 +++++++++-------- .../p2p/base/p2ptransportchannel_unittest.cc | 82 +++++++++++++++---- 2 files changed, 106 insertions(+), 49 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 95696e1e2c..83bcf840ee 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -478,17 +478,38 @@ void P2PTransportChannel::OnUnknownAddress( remote_password = remote_ice_pwd_; } - Candidate new_remote_candidate; - if (candidate != NULL) { - new_remote_candidate = *candidate; + Candidate remote_candidate; + bool remote_candidate_is_new = (candidate == nullptr); + if (!remote_candidate_is_new) { + remote_candidate = *candidate; if (ufrag_per_port) { - new_remote_candidate.set_address(address); + remote_candidate.set_address(address); } } else { // Create a new candidate with this address. std::string type; + int remote_candidate_priority; if (port->IceProtocol() == ICEPROTO_RFC5245) { + // RFC 5245 + // If the source transport address of the request does not match any + // existing remote candidates, it represents a new peer reflexive remote + // candidate. type = PRFLX_PORT_TYPE; + + // The priority of the candidate is set to the PRIORITY attribute + // from the request. + const StunUInt32Attribute* priority_attr = + stun_msg->GetUInt32(STUN_ATTR_PRIORITY); + if (!priority_attr) { + LOG(LS_WARNING) << "P2PTransportChannel::OnUnknownAddress - " + << "No STUN_ATTR_PRIORITY found in the " + << "stun request message"; + port->SendBindingErrorResponse(stun_msg, address, + STUN_ERROR_BAD_REQUEST, + STUN_ERROR_REASON_BAD_REQUEST); + return; + } + remote_candidate_priority = priority_attr->value(); } else { // G-ICE doesn't support prflx candidate. // We set candidate type to STUN_PORT_TYPE if the binding request comes @@ -499,43 +520,24 @@ void P2PTransportChannel::OnUnknownAddress( } else { type = port->Type(); } + remote_candidate_priority = remote_candidate.GetPriority( + ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0); } - new_remote_candidate = + remote_candidate = Candidate(component(), ProtoToString(proto), address, 0, remote_username, remote_password, type, 0U, ""); // From RFC 5245, section-7.2.1.3: // The foundation of the candidate is set to an arbitrary value, different // from the foundation for all other remote candidates. - new_remote_candidate.set_foundation( - rtc::ToString(rtc::ComputeCrc32(new_remote_candidate.id()))); + remote_candidate.set_foundation( + rtc::ToString(rtc::ComputeCrc32(remote_candidate.id()))); - new_remote_candidate.set_priority(new_remote_candidate.GetPriority( - ICE_TYPE_PREFERENCE_PRFLX, port->Network()->preference(), 0)); + remote_candidate.set_priority(remote_candidate_priority); } if (port->IceProtocol() == ICEPROTO_RFC5245) { - // RFC 5245 - // If the source transport address of the request does not match any - // existing remote candidates, it represents a new peer reflexive remote - // candidate. - - // The priority of the candidate is set to the PRIORITY attribute - // from the request. - const StunUInt32Attribute* priority_attr = - stun_msg->GetUInt32(STUN_ATTR_PRIORITY); - if (!priority_attr) { - LOG(LS_WARNING) << "P2PTransportChannel::OnUnknownAddress - " - << "No STUN_ATTR_PRIORITY found in the " - << "stun request message"; - port->SendBindingErrorResponse(stun_msg, address, - STUN_ERROR_BAD_REQUEST, - STUN_ERROR_REASON_BAD_REQUEST); - return; - } - new_remote_candidate.set_priority(priority_attr->value()); - // RFC5245, the agent constructs a pair whose local candidate is equal to // the transport address on which the STUN request was received, and a // remote candidate equal to the source transport address where the @@ -545,10 +547,10 @@ void P2PTransportChannel::OnUnknownAddress( // When ports are muxed, this channel might get multiple unknown address // signals. In that case if the connection is already exists, we should // simply ignore the signal othewise send server error. - if (port->GetConnection(new_remote_candidate.address())) { + if (port->GetConnection(remote_candidate.address())) { if (port_muxed) { LOG(LS_INFO) << "Connection already exists for peer reflexive " - << "candidate: " << new_remote_candidate.ToString(); + << "candidate: " << remote_candidate.ToString(); return; } else { ASSERT(false); @@ -560,7 +562,7 @@ void P2PTransportChannel::OnUnknownAddress( } Connection* connection = port->CreateConnection( - new_remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT); + remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT); if (!connection) { ASSERT(false); port->SendBindingErrorResponse(stun_msg, address, @@ -569,8 +571,9 @@ void P2PTransportChannel::OnUnknownAddress( return; } - LOG(LS_INFO) << "Adding connection from peer reflexive candidate: " - << new_remote_candidate.ToString(); + LOG(LS_INFO) << "Adding connection from " + << (remote_candidate_is_new ? "peer reflexive" : "resurrected") + << " candidate: " << remote_candidate.ToString(); AddConnection(connection); connection->ReceivedPing(); @@ -585,7 +588,7 @@ void P2PTransportChannel::OnUnknownAddress( // Check for connectivity to this address. Create connections // to this address across all local ports. First, add this as a new remote // address - if (!CreateConnections(new_remote_candidate, port, true)) { + if (!CreateConnections(remote_candidate, port, true)) { // Hopefully this won't occur, because changing a destination address // shouldn't cause a new connection to fail ASSERT(false); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index f6beee07bf..13f19ae0a5 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1700,19 +1700,20 @@ TEST_F(P2PTransportChannelMultihomedTest, TestDrain) { DestroyChannels(); } -class P2PTransportChannelPingOrderTest : public testing::Test, - public sigslot::has_slots<> { +// A collection of tests which tests a single P2PTransportChannel by sending +// pings. +class P2PTransportChannelPingTest : public testing::Test, + public sigslot::has_slots<> { public: - P2PTransportChannelPingOrderTest() : - pss_(new rtc::PhysicalSocketServer), - vss_(new rtc::VirtualSocketServer(pss_.get())), - ss_scope_(vss_.get()) { - } + P2PTransportChannelPingTest() + : pss_(new rtc::PhysicalSocketServer), + vss_(new rtc::VirtualSocketServer(pss_.get())), + ss_scope_(vss_.get()) {} protected: void PrepareChannel(cricket::P2PTransportChannel* ch) { ch->SignalRequestSignaling.connect( - this, &P2PTransportChannelPingOrderTest::OnChannelRequestSignaling); + this, &P2PTransportChannelPingTest::OnChannelRequestSignaling); ch->SetIceProtocolType(cricket::ICEPROTO_RFC5245); ch->SetIceRole(cricket::ICEROLE_CONTROLLING); ch->SetIceCredentials(kIceUfrag[0], kIcePwd[0]); @@ -1741,13 +1742,17 @@ class P2PTransportChannelPingOrderTest : public testing::Test, return GetConnectionTo(ch, ip, port_num); } - cricket::Connection* GetConnectionTo(cricket::P2PTransportChannel* ch, - const std::string& ip, - int port_num) { + cricket::Port* GetPort(cricket::P2PTransportChannel* ch) { if (ch->ports().empty()) { return nullptr; } - cricket::Port* port = static_cast(ch->ports()[0]); + return static_cast(ch->ports()[0]); + } + + cricket::Connection* GetConnectionTo(cricket::P2PTransportChannel* ch, + const std::string& ip, + int port_num) { + cricket::Port* port = GetPort(ch); if (!port) { return nullptr; } @@ -1760,7 +1765,7 @@ class P2PTransportChannelPingOrderTest : public testing::Test, rtc::SocketServerScope ss_scope_; }; -TEST_F(P2PTransportChannelPingOrderTest, TestTriggeredChecks) { +TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); @@ -1784,7 +1789,7 @@ TEST_F(P2PTransportChannelPingOrderTest, TestTriggeredChecks) { EXPECT_EQ(conn1, ch.FindNextPingableConnection()); } -TEST_F(P2PTransportChannelPingOrderTest, TestNoTriggeredChecksWhenWritable) { +TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); @@ -1807,3 +1812,52 @@ TEST_F(P2PTransportChannelPingOrderTest, TestNoTriggeredChecksWhenWritable) { // a higher priority. EXPECT_EQ(conn2, ch.FindNextPingableConnection()); } + +TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("connection resurrection", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.Connect(); + + // Create conn1 and keep track of original candidate priority. + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1)); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + uint32 remote_priority = conn1->remote_candidate().priority(); + + // Create a higher priority candidate and make the connection + // readable/writable. This will prune conn1. + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 2)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn2 != nullptr); + conn2->ReceivedPing(); + conn2->ReceivedPingResponse(); + + // Wait for conn1 being destroyed. + EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 3000); + cricket::Port* port = GetPort(&ch); + + // Create a minimal STUN message with prflx priority. + cricket::IceMessage request; + request.SetType(cricket::STUN_BINDING_REQUEST); + request.AddAttribute(new cricket::StunByteStringAttribute( + cricket::STUN_ATTR_USERNAME, kIceUfrag[1])); + uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24; + request.AddAttribute(new cricket::StunUInt32Attribute( + cricket::STUN_ATTR_PRIORITY, prflx_priority)); + EXPECT_NE(prflx_priority, remote_priority); + + // conn1 should be resurrected with original priority. + port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn1->remote_candidate().priority(), remote_priority); + + // conn3, a real prflx connection, should have prflx priority. + port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 1), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 1); + ASSERT_TRUE(conn3 != nullptr); + EXPECT_EQ(conn3->remote_candidate().priority(), prflx_priority); +}