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}
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 95696e1..83bcf84 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -478,17 +478,38 @@
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 @@
} 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<uint32>(rtc::ComputeCrc32(new_remote_candidate.id())));
+ remote_candidate.set_foundation(
+ rtc::ToString<uint32>(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 @@
// 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 @@
}
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 @@
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 @@
// 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 f6beee0..13f19ae 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1700,19 +1700,20 @@
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 @@
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<cricket::Port*>(ch->ports()[0]);
+ return static_cast<cricket::Port*>(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 @@
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 @@
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 @@
// 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);
+}
