diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 3ac291fa1f..a39ad93d5a 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -605,14 +605,7 @@ void P2PTransportChannel::OnUnknownAddress( << (remote_candidate_is_new ? "peer reflexive" : "resurrected") << " candidate: " << remote_candidate.ToString(); AddConnection(connection); - connection->ReceivedPing(); - - bool received_use_candidate = - stun_msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr; - if (received_use_candidate && ice_role_ == ICEROLE_CONTROLLED) { - connection->set_nominated(true); - OnNominated(connection); - } + connection->HandleBindingRequest(stun_msg); // Update the list of connections since we just added another. We do this // after sending the response since it could (in principle) delete the diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index c5c21acc72..8ec6474a2d 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1932,7 +1932,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // The controlled side will select a connection as the "best connection" based // on requests from an unknown address before the controlling side nominates // a connection, and will nominate a connection from an unknown address if the -// request contains the use_candidate attribute. +// request contains the use_candidate attribute. Plus, it will also sends back +// a ping response. TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); @@ -1948,14 +1949,16 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { uint32_t prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24; request.AddAttribute(new cricket::StunUInt32Attribute( cricket::STUN_ATTR_PRIORITY, prflx_priority)); - cricket::Port* port = GetPort(&ch); + cricket::TestUDPPort* port = static_cast(GetPort(&ch)); port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), cricket::PROTO_UDP, &request, kIceUfrag[1], false); cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); ASSERT_TRUE(conn1 != nullptr); + EXPECT_TRUE(port->sent_binding_response()); EXPECT_EQ(conn1, ch.best_connection()); conn1->ReceivedPingResponse(); EXPECT_EQ(conn1, ch.best_connection()); + port->set_sent_binding_response(false); // Another connection is nominated via use_candidate. ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 1)); @@ -1977,8 +1980,10 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { cricket::PROTO_UDP, &request, kIceUfrag[1], false); cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); ASSERT_TRUE(conn3 != nullptr); + EXPECT_TRUE(port->sent_binding_response()); conn3->ReceivedPingResponse(); // Become writable. EXPECT_EQ(conn2, ch.best_connection()); + port->set_sent_binding_response(false); // However if the request contains use_candidate attribute, it will be // selected as the best connection. @@ -1988,6 +1993,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { cricket::PROTO_UDP, &request, kIceUfrag[1], false); cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4); ASSERT_TRUE(conn4 != nullptr); + EXPECT_TRUE(port->sent_binding_response()); // conn4 is not the best connection yet because it is not writable. EXPECT_EQ(conn2, ch.best_connection()); conn4->ReceivedPingResponse(); // Become writable. diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 69dff9483b..f04e619426 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -567,10 +567,6 @@ void Port::SendBindingResponse(StunMessage* request, response.AddMessageIntegrity(password_); response.AddFingerprint(); - // The fact that we received a successful request means that this connection - // (if one exists) should now be receiving. - Connection* conn = GetConnection(addr); - // Send the response message. rtc::ByteBuffer buf; response.Write(&buf); @@ -585,6 +581,7 @@ void Port::SendBindingResponse(StunMessage* request, } else { // Log at LS_INFO if we send a stun ping response on an unwritable // connection. + Connection* conn = GetConnection(addr); rtc::LoggingSeverity sev = (conn && !conn->writable()) ? rtc::LS_INFO : rtc::LS_VERBOSE; LOG_JV(sev, this) @@ -592,10 +589,6 @@ void Port::SendBindingResponse(StunMessage* request, << ", to=" << addr.ToSensitiveString() << ", id=" << rtc::hex_encode(response.transaction_id()); } - - ASSERT(conn != NULL); - if (conn) - conn->ReceivedPing(); } void Port::SendBindingErrorResponse(StunMessage* request, @@ -924,29 +917,7 @@ void Connection::OnReadPacket( << ", id=" << rtc::hex_encode(msg->transaction_id()); if (remote_ufrag == remote_candidate_.username()) { - // Check for role conflicts. - if (!port_->MaybeIceRoleConflict(addr, msg.get(), remote_ufrag)) { - // Received conflicting role from the peer. - LOG(LS_INFO) << "Received conflicting role from the peer."; - return; - } - - // Incoming, validated stun request from remote peer. - // This call will also set the connection receiving. - port_->SendBindingResponse(msg.get(), addr); - - // If timed out sending writability checks, start up again - if (!pruned_ && (write_state_ == STATE_WRITE_TIMEOUT)) - set_write_state(STATE_WRITE_INIT); - - if (port_->GetIceRole() == ICEROLE_CONTROLLED) { - const StunByteStringAttribute* use_candidate_attr = - msg->GetByteString(STUN_ATTR_USE_CANDIDATE); - if (use_candidate_attr) { - set_nominated(true); - SignalNominated(this); - } - } + HandleBindingRequest(msg.get()); } else { // The packet had the right local username, but the remote username // was not the right one for the remote address. @@ -986,6 +957,37 @@ void Connection::OnReadPacket( } } +void Connection::HandleBindingRequest(IceMessage* msg) { + // This connection should now be receiving. + ReceivedPing(); + + const rtc::SocketAddress& remote_addr = remote_candidate_.address(); + const std::string& remote_ufrag = remote_candidate_.username(); + // Check for role conflicts. + if (!port_->MaybeIceRoleConflict(remote_addr, msg, remote_ufrag)) { + // Received conflicting role from the peer. + LOG(LS_INFO) << "Received conflicting role from the peer."; + return; + } + + // This is a validated stun request from remote peer. + port_->SendBindingResponse(msg, remote_addr); + + // If it timed out on writing check, start up again + if (!pruned_ && write_state_ == STATE_WRITE_TIMEOUT) { + set_write_state(STATE_WRITE_INIT); + } + + if (port_->GetIceRole() == ICEROLE_CONTROLLED) { + const StunByteStringAttribute* use_candidate_attr = + msg->GetByteString(STUN_ATTR_USE_CANDIDATE); + if (use_candidate_attr) { + set_nominated(true); + SignalNominated(this); + } + } +} + void Connection::OnReadyToSend() { if (write_state_ == STATE_WRITABLE) { SignalReadyToSend(this); diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 01c45f26d8..2f69f49459 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -523,6 +523,8 @@ class Connection : public rtc::MessageHandler, // public because the connection intercepts the first ping for us. uint32_t last_ping_received() const { return last_ping_received_; } void ReceivedPing(); + // Handles the binding request; sends a response if this is a valid request. + void HandleBindingRequest(IceMessage* msg); // Debugging description of this connection std::string ToDebugId() const; diff --git a/webrtc/p2p/client/fakeportallocator.h b/webrtc/p2p/client/fakeportallocator.h index d59333a97d..d9af4b3f47 100644 --- a/webrtc/p2p/client/fakeportallocator.h +++ b/webrtc/p2p/client/fakeportallocator.h @@ -24,6 +24,62 @@ class Thread; namespace cricket { +class TestUDPPort : public UDPPort { + public: + static TestUDPPort* Create(rtc::Thread* thread, + rtc::PacketSocketFactory* factory, + rtc::Network* network, + const rtc::IPAddress& ip, + uint16_t min_port, + uint16_t max_port, + const std::string& username, + const std::string& password, + const std::string& origin, + bool emit_localhost_for_anyaddress) { + TestUDPPort* port = new TestUDPPort(thread, factory, network, ip, min_port, + max_port, username, password, origin, + emit_localhost_for_anyaddress); + if (!port->Init()) { + delete port; + port = nullptr; + } + return port; + } + void SendBindingResponse(StunMessage* request, + const rtc::SocketAddress& addr) override { + UDPPort::SendBindingResponse(request, addr); + sent_binding_response_ = true; + } + bool sent_binding_response() { return sent_binding_response_; } + void set_sent_binding_response(bool response) { + sent_binding_response_ = response; + } + + protected: + TestUDPPort(rtc::Thread* thread, + rtc::PacketSocketFactory* factory, + rtc::Network* network, + const rtc::IPAddress& ip, + uint16_t min_port, + uint16_t max_port, + const std::string& username, + const std::string& password, + const std::string& origin, + bool emit_localhost_for_anyaddress) + : UDPPort(thread, + factory, + network, + ip, + min_port, + max_port, + username, + password, + origin, + emit_localhost_for_anyaddress) {} + + bool sent_binding_response_ = false; +}; + class FakePortAllocatorSession : public PortAllocatorSession { public: FakePortAllocatorSession(rtc::Thread* worker_thread, @@ -45,16 +101,9 @@ class FakePortAllocatorSession : public PortAllocatorSession { virtual void StartGettingPorts() { if (!port_) { - port_.reset(cricket::UDPPort::Create(worker_thread_, - factory_, - &network_, - network_.GetBestIP(), - 0, - 0, - username(), - password(), - std::string(), - false)); + port_.reset(TestUDPPort::Create(worker_thread_, factory_, &network_, + network_.GetBestIP(), 0, 0, username(), + password(), std::string(), false)); AddPort(port_.get()); } ++port_config_count_;