diff --git a/webrtc/p2p/base/fakeportallocator.h b/webrtc/p2p/base/fakeportallocator.h index 7bf7f969bb..d7c30f0a6e 100644 --- a/webrtc/p2p/base/fakeportallocator.h +++ b/webrtc/p2p/base/fakeportallocator.h @@ -32,16 +32,15 @@ class TestUDPPort : public UDPPort { 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); + TestUDPPort* port = + new TestUDPPort(thread, factory, network, min_port, max_port, username, + password, origin, emit_localhost_for_anyaddress); if (!port->Init()) { delete port; port = nullptr; @@ -62,7 +61,6 @@ class TestUDPPort : public UDPPort { 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, @@ -72,7 +70,6 @@ class TestUDPPort : public UDPPort { : UDPPort(thread, factory, network, - ip, min_port, max_port, username, @@ -128,9 +125,9 @@ class FakePortAllocatorSession : public PortAllocatorSession { (rtc::HasIPv6Enabled() && (flags() & PORTALLOCATOR_ENABLE_IPV6)) ? ipv6_network_ : ipv4_network_; - port_.reset(TestUDPPort::Create(network_thread_, factory_, &network, - network.GetBestIP(), 0, 0, username(), - password(), std::string(), false)); + port_.reset(TestUDPPort::Create(network_thread_, factory_, &network, 0, 0, + username(), password(), std::string(), + false)); port_->SignalDestroyed.connect( this, &FakePortAllocatorSession::OnPortDestroyed); AddPort(port_.get()); diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index a1016332de..40b2502f65 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -145,7 +145,6 @@ Port::Port(rtc::Thread* thread, const std::string& type, rtc::PacketSocketFactory* factory, rtc::Network* network, - const rtc::IPAddress& ip, const std::string& username_fragment, const std::string& password) : thread_(thread), @@ -153,7 +152,6 @@ Port::Port(rtc::Thread* thread, type_(type), send_retransmit_count_attribute_(false), network_(network), - ip_(ip), min_port_(0), max_port_(0), component_(ICE_CANDIDATE_COMPONENT_DEFAULT), @@ -172,7 +170,6 @@ Port::Port(rtc::Thread* thread, const std::string& type, rtc::PacketSocketFactory* factory, rtc::Network* network, - const rtc::IPAddress& ip, uint16_t min_port, uint16_t max_port, const std::string& username_fragment, @@ -182,7 +179,6 @@ Port::Port(rtc::Thread* thread, type_(type), send_retransmit_count_attribute_(false), network_(network), - ip_(ip), min_port_(min_port), max_port_(max_port), component_(ICE_CANDIDATE_COMPONENT_DEFAULT), @@ -471,14 +467,15 @@ bool Port::GetStunMessage(const char* data, } bool Port::IsCompatibleAddress(const rtc::SocketAddress& addr) { - int family = ip().family(); + // Get a representative IP for the Network this port is configured to use. + rtc::IPAddress ip = network_->GetBestIP(); // We use single-stack sockets, so families must match. - if (addr.family() != family) { + if (addr.family() != ip.family()) { return false; } // Link-local IPv6 ports can only connect to other link-local IPv6 ports. - if (family == AF_INET6 && - (IPIsLinkLocal(ip()) != IPIsLinkLocal(addr.ipaddr()))) { + if (ip.family() == AF_INET6 && + (IPIsLinkLocal(ip) != IPIsLinkLocal(addr.ipaddr()))) { return false; } return true; diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 5764738508..19391b52ba 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -142,14 +142,21 @@ class Port : public PortInterface, public rtc::MessageHandler, const std::string& type, rtc::PacketSocketFactory* factory, rtc::Network* network, - const rtc::IPAddress& ip, const std::string& username_fragment, const std::string& password); + // TODO(deadbeef): Delete this constructor once clients are moved off of it. Port(rtc::Thread* thread, const std::string& type, rtc::PacketSocketFactory* factory, rtc::Network* network, const rtc::IPAddress& ip, + const std::string& username_fragment, + const std::string& password) + : Port(thread, type, factory, network, username_fragment, password) {} + Port(rtc::Thread* thread, + const std::string& type, + rtc::PacketSocketFactory* factory, + rtc::Network* network, uint16_t min_port, uint16_t max_port, const std::string& username_fragment, @@ -283,7 +290,6 @@ class Port : public PortInterface, public rtc::MessageHandler, // Debugging description of this port virtual std::string ToString() const; - const rtc::IPAddress& ip() const { return ip_; } uint16_t min_port() { return min_port_; } uint16_t max_port() { return max_port_; } @@ -397,7 +403,6 @@ class Port : public PortInterface, public rtc::MessageHandler, std::string type_; bool send_retransmit_count_attribute_; rtc::Network* network_; - rtc::IPAddress ip_; uint16_t min_port_; uint16_t max_port_; std::string content_name_; diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index eb547605c9..757c1b31bf 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -8,6 +8,7 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include #include #include "webrtc/p2p/base/basicpacketsocketfactory.h" @@ -109,7 +110,6 @@ class TestPort : public Port { const std::string& type, rtc::PacketSocketFactory* factory, rtc::Network* network, - const rtc::IPAddress& ip, uint16_t min_port, uint16_t max_port, const std::string& username_fragment, @@ -118,7 +118,6 @@ class TestPort : public Port { type, factory, network, - ip, min_port, max_port, username_fragment, @@ -144,7 +143,9 @@ class TestPort : public Port { } virtual void PrepareAddress() { - rtc::SocketAddress addr(ip(), min_port()); + // Act as if the socket was bound to the best IP on the network, to the + // first port in the allowed range. + rtc::SocketAddress addr(Network()->GetBestIP(), min_port()); AddAddress(addr, addr, rtc::SocketAddress(), "udp", "", "", Type(), ICE_TYPE_PREFERENCE_HOST, 0, "", true); } @@ -382,7 +383,6 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { PortTest() : ss_(new rtc::VirtualSocketServer()), main_(ss_.get()), - network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), nat_factory1_(ss_.get(), kNatAddr1, SocketAddress()), nat_factory2_(ss_.get(), kNatAddr2, SocketAddress()), @@ -401,7 +401,6 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { password_(rtc::CreateRandomString(ICE_PWD_LENGTH)), role_conflict_(false), ports_destroyed_(0) { - network_.AddIP(rtc::IPAddress(INADDR_ANY)); } protected: @@ -483,32 +482,36 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { TestConnectivity("ssltcp", port1, RelayName(rtype, proto), port2, rtype == RELAY_GTURN, false, true, true); } + + rtc::Network* MakeNetwork(const SocketAddress& addr) { + networks_.emplace_back("unittest", "unittest", addr.ipaddr(), 32); + networks_.back().AddIP(addr.ipaddr()); + return &networks_.back(); + } + // helpers for above functions UDPPort* CreateUdpPort(const SocketAddress& addr) { return CreateUdpPort(addr, &socket_factory_); } UDPPort* CreateUdpPort(const SocketAddress& addr, PacketSocketFactory* socket_factory) { - return UDPPort::Create(&main_, socket_factory, &network_, addr.ipaddr(), 0, - 0, username_, password_, std::string(), true); + return UDPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0, + username_, password_, std::string(), true); } TCPPort* CreateTcpPort(const SocketAddress& addr) { return CreateTcpPort(addr, &socket_factory_); } TCPPort* CreateTcpPort(const SocketAddress& addr, PacketSocketFactory* socket_factory) { - return TCPPort::Create(&main_, socket_factory, &network_, - addr.ipaddr(), 0, 0, username_, password_, - true); + return TCPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0, + username_, password_, true); } StunPort* CreateStunPort(const SocketAddress& addr, rtc::PacketSocketFactory* factory) { ServerAddresses stun_servers; stun_servers.insert(kStunAddr); - return StunPort::Create(&main_, factory, &network_, - addr.ipaddr(), 0, 0, - username_, password_, stun_servers, - std::string()); + return StunPort::Create(&main_, factory, MakeNetwork(addr), 0, 0, username_, + password_, stun_servers, std::string()); } Port* CreateRelayPort(const SocketAddress& addr, RelayType rtype, ProtocolType int_proto, ProtocolType ext_proto) { @@ -530,8 +533,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { PacketSocketFactory* socket_factory, ProtocolType int_proto, ProtocolType ext_proto, const rtc::SocketAddress& server_addr) { - return TurnPort::Create(&main_, socket_factory, &network_, addr.ipaddr(), 0, - 0, username_, password_, + return TurnPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0, + username_, password_, ProtocolAddress(server_addr, int_proto), kRelayCredentials, 0, std::string()); } @@ -547,8 +550,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { // TODO(pthatcher): Remove GTURN. // Generate a username with length of 16 for Gturn only. std::string username = rtc::CreateRandomString(kGturnUserNameLength); - return RelayPort::Create(&main_, &socket_factory_, &network_, addr.ipaddr(), - 0, 0, username, password_); + return RelayPort::Create(&main_, &socket_factory_, MakeNetwork(addr), 0, 0, + username, password_); // TODO: Add an external address for ext_proto, so that the // other side can connect to this port using a non-UDP protocol. } @@ -600,10 +603,6 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { } } - void SetNetworkType(rtc::AdapterType adapter_type) { - network_.set_type(adapter_type); - } - void TestCrossFamilyPorts(int type); void ExpectPortsCanConnect(bool can_connect, Port* p1, Port* p2); @@ -764,8 +763,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { TestPort* CreateTestPort(const rtc::SocketAddress& addr, const std::string& username, const std::string& password) { - TestPort* port = new TestPort(&main_, "test", &socket_factory_, &network_, - addr.ipaddr(), 0, 0, username, password); + TestPort* port = new TestPort(&main_, "test", &socket_factory_, + MakeNetwork(addr), 0, 0, username, password); port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict); return port; } @@ -779,6 +778,15 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { port->SetIceTiebreaker(tiebreaker); return port; } + // Overload to create a test port given an rtc::Network directly. + TestPort* CreateTestPort(rtc::Network* network, + const std::string& username, + const std::string& password) { + TestPort* port = new TestPort(&main_, "test", &socket_factory_, network, 0, + 0, username, password); + port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict); + return port; + } void OnRoleConflict(PortInterface* port) { role_conflict_ = true; @@ -799,9 +807,12 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { rtc::VirtualSocketServer* vss() { return ss_.get(); } private: + // When a "create port" helper method is called with an IP, we create a + // Network with that IP and add it to this list. Using a list instead of a + // vector so that when it grows, pointers aren't invalidated. + std::list networks_; std::unique_ptr ss_; rtc::AutoSocketServerThread main_; - rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::unique_ptr nat_server1_; std::unique_ptr nat_server2_; @@ -1931,10 +1942,11 @@ TEST_F(PortTest, TestUseCandidateAttribute) { // change, the network cost of the local candidates will change. Also tests that // the remote network costs are updated with the stun binding requests. TEST_F(PortTest, TestNetworkCostChange) { + rtc::Network* test_network = MakeNetwork(kLocalAddr1); std::unique_ptr lport( - CreateTestPort(kLocalAddr1, "lfrag", "lpass")); + CreateTestPort(test_network, "lfrag", "lpass")); std::unique_ptr rport( - CreateTestPort(kLocalAddr2, "rfrag", "rpass")); + CreateTestPort(test_network, "rfrag", "rpass")); lport->SetIceRole(cricket::ICEROLE_CONTROLLING); lport->SetIceTiebreaker(kTiebreaker1); rport->SetIceRole(cricket::ICEROLE_CONTROLLED); @@ -1950,7 +1962,7 @@ TEST_F(PortTest, TestNetworkCostChange) { } // Change the network type to wifi. - SetNetworkType(rtc::ADAPTER_TYPE_WIFI); + test_network->set_type(rtc::ADAPTER_TYPE_WIFI); EXPECT_EQ(rtc::kNetworkCostLow, lport->network_cost()); for (const cricket::Candidate& candidate : lport->Candidates()) { EXPECT_EQ(rtc::kNetworkCostLow, candidate.network_cost()); @@ -1960,16 +1972,16 @@ TEST_F(PortTest, TestNetworkCostChange) { Connection* lconn = lport->CreateConnection(rport->Candidates()[0], Port::ORIGIN_MESSAGE); // Change the network type to cellular. - SetNetworkType(rtc::ADAPTER_TYPE_CELLULAR); + test_network->set_type(rtc::ADAPTER_TYPE_CELLULAR); EXPECT_EQ(rtc::kNetworkCostHigh, lport->network_cost()); for (const cricket::Candidate& candidate : lport->Candidates()) { EXPECT_EQ(rtc::kNetworkCostHigh, candidate.network_cost()); } - SetNetworkType(rtc::ADAPTER_TYPE_WIFI); + test_network->set_type(rtc::ADAPTER_TYPE_WIFI); Connection* rconn = rport->CreateConnection(lport->Candidates()[0], Port::ORIGIN_MESSAGE); - SetNetworkType(rtc::ADAPTER_TYPE_CELLULAR); + test_network->set_type(rtc::ADAPTER_TYPE_CELLULAR); lconn->Ping(0); // The rconn's remote candidate cost is rtc::kNetworkCostLow, but the ping // contains an attribute of network cost of rtc::kNetworkCostHigh. Once the @@ -1988,10 +2000,11 @@ TEST_F(PortTest, TestNetworkCostChange) { } TEST_F(PortTest, TestNetworkInfoAttribute) { + rtc::Network* test_network = MakeNetwork(kLocalAddr1); std::unique_ptr lport( - CreateTestPort(kLocalAddr1, "lfrag", "lpass")); + CreateTestPort(test_network, "lfrag", "lpass")); std::unique_ptr rport( - CreateTestPort(kLocalAddr2, "rfrag", "rpass")); + CreateTestPort(test_network, "rfrag", "rpass")); lport->SetIceRole(cricket::ICEROLE_CONTROLLING); lport->SetIceTiebreaker(kTiebreaker1); rport->SetIceRole(cricket::ICEROLE_CONTROLLED); @@ -2017,7 +2030,7 @@ TEST_F(PortTest, TestNetworkInfoAttribute) { // Set the network type to be cellular so its cost will be kNetworkCostHigh. // Send a fake ping from rport to lport. - SetNetworkType(rtc::ADAPTER_TYPE_CELLULAR); + test_network->set_type(rtc::ADAPTER_TYPE_CELLULAR); uint16_t rnetwork_id = 8; rport->Network()->set_id(rnetwork_id); Connection* rconn = diff --git a/webrtc/p2p/base/relayport.cc b/webrtc/p2p/base/relayport.cc index efb967098b..387ec4b9fc 100644 --- a/webrtc/p2p/base/relayport.cc +++ b/webrtc/p2p/base/relayport.cc @@ -182,7 +182,6 @@ class AllocateRequest : public StunRequest { RelayPort::RelayPort(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, @@ -191,7 +190,6 @@ RelayPort::RelayPort(rtc::Thread* thread, RELAY_PORT_TYPE, factory, network, - ip, min_port, max_port, username, @@ -489,14 +487,14 @@ void RelayEntry::Connect() { if (ra->proto == PROTO_UDP) { // UDP sockets are simple. socket = port_->socket_factory()->CreateUdpSocket( - rtc::SocketAddress(port_->ip(), 0), - port_->min_port(), port_->max_port()); + rtc::SocketAddress(port_->Network()->GetBestIP(), 0), port_->min_port(), + port_->max_port()); } else if (ra->proto == PROTO_TCP || ra->proto == PROTO_SSLTCP) { int opts = (ra->proto == PROTO_SSLTCP) ? rtc::PacketSocketFactory::OPT_TLS_FAKE : 0; socket = port_->socket_factory()->CreateClientTcpSocket( - rtc::SocketAddress(port_->ip(), 0), ra->address, + rtc::SocketAddress(port_->Network()->GetBestIP(), 0), ra->address, port_->proxy(), port_->user_agent(), opts); } else { LOG(LS_WARNING) << "Unknown protocol (" << ra->proto << ")"; diff --git a/webrtc/p2p/base/relayport.h b/webrtc/p2p/base/relayport.h index 8fa2235043..e2e6800710 100644 --- a/webrtc/p2p/base/relayport.h +++ b/webrtc/p2p/base/relayport.h @@ -38,13 +38,12 @@ class RelayPort : public Port { static RelayPort* 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) { - return new RelayPort(thread, factory, network, ip, min_port, max_port, - username, password); + return new RelayPort(thread, factory, network, min_port, max_port, username, + password); } ~RelayPort() override; @@ -82,7 +81,6 @@ class RelayPort : public Port { RelayPort(rtc::Thread* thread, rtc::PacketSocketFactory* factory, rtc::Network*, - const rtc::IPAddress& ip, uint16_t min_port, uint16_t max_port, const std::string& username, diff --git a/webrtc/p2p/base/relayport_unittest.cc b/webrtc/p2p/base/relayport_unittest.cc index e1bed76f6f..a5d919a678 100644 --- a/webrtc/p2p/base/relayport_unittest.cc +++ b/webrtc/p2p/base/relayport_unittest.cc @@ -46,19 +46,20 @@ class RelayPortTest : public testing::Test, RelayPortTest() : virtual_socket_server_(new rtc::VirtualSocketServer()), main_(virtual_socket_server_.get()), - network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), + network_("unittest", "unittest", kLocalAddress.ipaddr(), 32), socket_factory_(rtc::Thread::Current()), username_(rtc::CreateRandomString(16)), password_(rtc::CreateRandomString(16)), relay_port_(cricket::RelayPort::Create(&main_, &socket_factory_, &network_, - kLocalAddress.ipaddr(), 0, 0, username_, password_)), - relay_server_(new cricket::RelayServer(&main_)) {} + relay_server_(new cricket::RelayServer(&main_)) { + network_.AddIP(kLocalAddress.ipaddr()); + } void OnReadPacket(rtc::AsyncPacketSocket* socket, const char* data, size_t size, diff --git a/webrtc/p2p/base/stunport.cc b/webrtc/p2p/base/stunport.cc index fa7a63c46b..5f6889e6dc 100644 --- a/webrtc/p2p/base/stunport.cc +++ b/webrtc/p2p/base/stunport.cc @@ -170,7 +170,6 @@ UDPPort::UDPPort(rtc::Thread* thread, LOCAL_PORT_TYPE, factory, network, - socket->GetLocalAddress().ipaddr(), username, password), requests_(thread), @@ -185,7 +184,6 @@ UDPPort::UDPPort(rtc::Thread* thread, UDPPort::UDPPort(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, @@ -196,7 +194,6 @@ UDPPort::UDPPort(rtc::Thread* thread, LOCAL_PORT_TYPE, factory, network, - ip, min_port, max_port, username, @@ -215,7 +212,7 @@ bool UDPPort::Init() { if (!SharedSocket()) { RTC_DCHECK(socket_ == NULL); socket_ = socket_factory()->CreateUdpSocket( - rtc::SocketAddress(ip(), 0), min_port(), max_port()); + rtc::SocketAddress(Network()->GetBestIP(), 0), min_port(), max_port()); if (!socket_) { LOG_J(LS_WARNING, this) << "UDP socket creation failed"; return false; @@ -379,8 +376,8 @@ void UDPPort::OnResolveResult(const rtc::SocketAddress& input, RTC_DCHECK(resolver_.get() != NULL); rtc::SocketAddress resolved; - if (error != 0 || - !resolver_->GetResolvedAddress(input, ip().family(), &resolved)) { + if (error != 0 || !resolver_->GetResolvedAddress( + input, Network()->GetBestIP().family(), &resolved)) { LOG_J(LS_WARNING, this) << "StunPort: stun host lookup received error " << error; OnStunBindingOrResolveRequestFailed(input); diff --git a/webrtc/p2p/base/stunport.h b/webrtc/p2p/base/stunport.h index de5127c671..8fbc0d52fa 100644 --- a/webrtc/p2p/base/stunport.h +++ b/webrtc/p2p/base/stunport.h @@ -54,7 +54,6 @@ class UDPPort : public Port { static UDPPort* 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, @@ -62,7 +61,7 @@ class UDPPort : public Port { const std::string& origin, bool emit_local_for_anyaddress) { UDPPort* port = - new UDPPort(thread, factory, network, ip, min_port, max_port, username, + new UDPPort(thread, factory, network, min_port, max_port, username, password, origin, emit_local_for_anyaddress); if (!port->Init()) { delete port; @@ -127,7 +126,6 @@ class UDPPort : public Port { UDPPort(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, @@ -259,17 +257,14 @@ class StunPort : public UDPPort { static StunPort* 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 ServerAddresses& servers, const std::string& origin) { - StunPort* port = new StunPort(thread, factory, network, - ip, min_port, max_port, - username, password, servers, - origin); + StunPort* port = new StunPort(thread, factory, network, min_port, max_port, + username, password, servers, origin); if (!port->Init()) { delete port; port = NULL; @@ -287,7 +282,6 @@ class StunPort : public UDPPort { StunPort(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, @@ -297,7 +291,6 @@ class StunPort : public UDPPort { : UDPPort(thread, factory, network, - ip, min_port, max_port, username, diff --git a/webrtc/p2p/base/stunport_unittest.cc b/webrtc/p2p/base/stunport_unittest.cc index 25f5a74fcf..e994f67832 100644 --- a/webrtc/p2p/base/stunport_unittest.cc +++ b/webrtc/p2p/base/stunport_unittest.cc @@ -43,7 +43,7 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { StunPortTestBase() : ss_(new rtc::VirtualSocketServer()), thread_(ss_.get()), - network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), + network_("unittest", "unittest", kLocalAddr.ipaddr(), 32), socket_factory_(rtc::Thread::Current()), stun_server_1_(cricket::TestStunServer::Create(rtc::Thread::Current(), kStunAddr1)), @@ -52,7 +52,9 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { done_(false), error_(false), stun_keepalive_delay_(1), - stun_keepalive_lifetime_(-1) {} + stun_keepalive_lifetime_(-1) { + network_.AddIP(kLocalAddr.ipaddr()); + } cricket::UDPPort* port() const { return stun_port_.get(); } bool done() const { return done_; } @@ -70,9 +72,9 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { void CreateStunPort(const ServerAddresses& stun_servers) { stun_port_.reset(cricket::StunPort::Create( - rtc::Thread::Current(), &socket_factory_, &network_, - kLocalAddr.ipaddr(), 0, 0, rtc::CreateRandomString(16), - rtc::CreateRandomString(22), stun_servers, std::string())); + rtc::Thread::Current(), &socket_factory_, &network_, 0, 0, + rtc::CreateRandomString(16), rtc::CreateRandomString(22), stun_servers, + std::string())); stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_); // If |stun_keepalive_lifetime_| is negative, let the stun port // choose its lifetime from the network type. diff --git a/webrtc/p2p/base/tcpport.cc b/webrtc/p2p/base/tcpport.cc index 86670cd366..cbb25b9192 100644 --- a/webrtc/p2p/base/tcpport.cc +++ b/webrtc/p2p/base/tcpport.cc @@ -75,7 +75,6 @@ namespace cricket { TCPPort::TCPPort(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, @@ -85,7 +84,6 @@ TCPPort::TCPPort(rtc::Thread* thread, LOCAL_PORT_TYPE, factory, network, - ip, min_port, max_port, username, @@ -179,11 +177,15 @@ void TCPPort::PrepareAddress() { // Note: We still add the address, since otherwise the remote side won't // recognize our incoming TCP connections. According to // https://tools.ietf.org/html/rfc6544#section-4.5, for active candidate, - // the port must be set to the discard port, i.e. 9. - AddAddress(rtc::SocketAddress(ip(), DISCARD_PORT), - rtc::SocketAddress(ip(), 0), rtc::SocketAddress(), - TCP_PROTOCOL_NAME, "", TCPTYPE_ACTIVE_STR, LOCAL_PORT_TYPE, - ICE_TYPE_PREFERENCE_HOST_TCP, 0, "", true); + // the port must be set to the discard port, i.e. 9. We can't be 100% sure + // which IP address will actually be used, so GetBestIP is as good as we + // can do. + // TODO(deadbeef): We could do something like create a dummy socket just to + // see what IP we get. But that may be overkill. + AddAddress(rtc::SocketAddress(Network()->GetBestIP(), DISCARD_PORT), + rtc::SocketAddress(Network()->GetBestIP(), 0), + rtc::SocketAddress(), TCP_PROTOCOL_NAME, "", TCPTYPE_ACTIVE_STR, + LOCAL_PORT_TYPE, ICE_TYPE_PREFERENCE_HOST_TCP, 0, "", true); } } @@ -262,7 +264,8 @@ void TCPPort::OnNewConnection(rtc::AsyncPacketSocket* socket, void TCPPort::TryCreateServerSocket() { socket_ = socket_factory()->CreateServerTcpSocket( - rtc::SocketAddress(ip(), 0), min_port(), max_port(), false /* ssl */); + rtc::SocketAddress(Network()->GetBestIP(), 0), min_port(), max_port(), + false /* ssl */); if (!socket_) { LOG_J(LS_WARNING, this) << "TCP server socket creation failed; continuing anyway."; @@ -323,11 +326,16 @@ TCPConnection::TCPConnection(TCPPort* port, if (outgoing_) { CreateOutgoingTcpSocket(); } else { - // Incoming connections should match the network address. + // Incoming connections should match one of the network addresses. Same as + // what's being checked in OnConnect, but just DCHECKing here. LOG_J(LS_VERBOSE, this) << "socket ipaddr: " << socket_->GetLocalAddress().ToString() - << ",port() ip:" << port->ip().ToString(); - RTC_DCHECK(socket_->GetLocalAddress().ipaddr() == port->ip()); + << ", port() Network:" << port->Network()->ToString(); + const std::vector& desired_addresses = + port_->Network()->GetIPs(); + RTC_DCHECK(std::find(desired_addresses.begin(), desired_addresses.end(), + socket_->GetLocalAddress().ipaddr()) != + desired_addresses.end()); ConnectSocketSignals(socket); } } @@ -390,34 +398,49 @@ void TCPConnection::OnConnectionRequestResponse(ConnectionRequest* req, void TCPConnection::OnConnect(rtc::AsyncPacketSocket* socket) { RTC_DCHECK(socket == socket_.get()); - // Do not use this connection if the socket bound to a different address than - // the one we asked for. This is seen in Chrome, where TCP sockets cannot be - // given a binding address, and the platform is expected to pick the - // correct local address. - const rtc::SocketAddress& socket_addr = socket->GetLocalAddress(); - if (socket_addr.ipaddr() == port()->ip()) { + // Do not use this port if the socket bound to an address not associated with + // the desired network interface. This is seen in Chrome, where TCP sockets + // cannot be given a binding address, and the platform is expected to pick + // the correct local address. + // + // However, there are two situations in which we allow the bound address to + // not be one of the addresses of the requested interface: + // 1. The bound address is the loopback address. This happens when a proxy + // forces TCP to bind to only the localhost address (see issue 3927). + // 2. The bound address is the "any address". This happens when + // multiple_routes is disabled (see issue 4780). + // + // Note that, aside from minor differences in log statements, this logic is + // identical to that in TurnPort. + const rtc::SocketAddress& socket_address = socket->GetLocalAddress(); + const std::vector& desired_addresses = + port_->Network()->GetIPs(); + if (std::find(desired_addresses.begin(), desired_addresses.end(), + socket_address.ipaddr()) != desired_addresses.end()) { LOG_J(LS_VERBOSE, this) << "Connection established to " << socket->GetRemoteAddress().ToSensitiveString(); - } else if (IPIsAny(port()->ip())) { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket_addr.ipaddr().ToString() - << ", rather then the local port:" - << port()->ip().ToString() - << ". Still allowing it since it's any address" - << ", possibly caused by multi-routes being disabled."; - } else if (socket_addr.IsLoopbackIP()) { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket_addr.ipaddr().ToString() - << ", rather then the local port:" - << port()->ip().ToString() - << ". Still allowing it since it's localhost."; } else { - LOG_J(LS_WARNING, this) << "Dropping connection as TCP socket bound to IP " - << socket_addr.ipaddr().ToSensitiveString() - << ", different from the local candidate IP " - << port()->ip().ToSensitiveString(); - OnClose(socket, 0); - return; + if (socket->GetLocalAddress().IsLoopbackIP()) { + LOG(LS_WARNING) << "Socket is bound to the address:" + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << port_->Network()->ToString() + << ". Still allowing it since it's localhost."; + } else if (IPIsAny(port_->Network()->GetBestIP())) { + LOG(LS_WARNING) << "Socket is bound to the address:" + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << port_->Network()->ToString() + << ". Still allowing it since it's the 'any' address" + << ", possibly caused by multiple_routes being disabled."; + } else { + LOG(LS_WARNING) << "Dropping connection as TCP socket bound to IP " + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << port_->Network()->ToString(); + OnClose(socket, 0); + return; + } } // Connection is established successfully. @@ -501,8 +524,9 @@ void TCPConnection::CreateOutgoingTcpSocket() { ? rtc::PacketSocketFactory::OPT_TLS_FAKE : 0; socket_.reset(port()->socket_factory()->CreateClientTcpSocket( - rtc::SocketAddress(port()->ip(), 0), remote_candidate().address(), - port()->proxy(), port()->user_agent(), opts)); + rtc::SocketAddress(port()->Network()->GetBestIP(), 0), + remote_candidate().address(), port()->proxy(), port()->user_agent(), + opts)); if (socket_) { LOG_J(LS_VERBOSE, this) << "Connecting from " << socket_->GetLocalAddress().ToSensitiveString() diff --git a/webrtc/p2p/base/tcpport.h b/webrtc/p2p/base/tcpport.h index 0974fa78cd..a0a36df0e3 100644 --- a/webrtc/p2p/base/tcpport.h +++ b/webrtc/p2p/base/tcpport.h @@ -33,14 +33,13 @@ class TCPPort : public Port { static TCPPort* 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, bool allow_listen) { - return new TCPPort(thread, factory, network, ip, min_port, max_port, - username, password, allow_listen); + return new TCPPort(thread, factory, network, min_port, max_port, username, + password, allow_listen); } ~TCPPort() override; @@ -62,7 +61,6 @@ class TCPPort : public Port { TCPPort(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, diff --git a/webrtc/p2p/base/tcpport_unittest.cc b/webrtc/p2p/base/tcpport_unittest.cc index 0f7bd1f79d..884488efc3 100644 --- a/webrtc/p2p/base/tcpport_unittest.cc +++ b/webrtc/p2p/base/tcpport_unittest.cc @@ -8,6 +8,7 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include #include #include "webrtc/p2p/base/basicpacketsocketfactory.h" @@ -24,61 +25,134 @@ using cricket::ICE_UFRAG_LENGTH; using cricket::ICE_PWD_LENGTH; static int kTimeout = 1000; -static const SocketAddress kLocalAddr("11.11.11.11", 1); -static const SocketAddress kRemoteAddr("22.22.22.22", 2); +static const SocketAddress kLocalAddr("11.11.11.11", 0); +static const SocketAddress kAlternateLocalAddr("1.2.3.4", 0); +static const SocketAddress kRemoteAddr("22.22.22.22", 0); + +class ConnectionObserver : public sigslot::has_slots<> { + public: + ConnectionObserver(Connection* conn) { + conn->SignalDestroyed.connect(this, &ConnectionObserver::OnDestroyed); + } + + bool connection_destroyed() { return connection_destroyed_; } + + private: + void OnDestroyed(Connection*) { connection_destroyed_ = true; } + + bool connection_destroyed_ = false; +}; class TCPPortTest : public testing::Test, public sigslot::has_slots<> { public: TCPPortTest() : ss_(new rtc::VirtualSocketServer()), main_(ss_.get()), - network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), username_(rtc::CreateRandomString(ICE_UFRAG_LENGTH)), password_(rtc::CreateRandomString(ICE_PWD_LENGTH)) { - network_.AddIP(rtc::IPAddress(INADDR_ANY)); } - void ConnectSignalSocketCreated() { - ss_->SignalSocketCreated.connect(this, &TCPPortTest::OnSocketCreated); + rtc::Network* MakeNetwork(const SocketAddress& addr) { + networks_.emplace_back("unittest", "unittest", addr.ipaddr(), 32); + networks_.back().AddIP(addr.ipaddr()); + return &networks_.back(); } - void OnSocketCreated(rtc::VirtualSocket* socket) { - LOG(LS_INFO) << "socket created "; - socket->SignalAddressReady.connect( - this, &TCPPortTest::SetLocalhostAsAlternativeLocalAddress); + std::unique_ptr CreateTCPPort(const SocketAddress& addr) { + return std::unique_ptr( + TCPPort::Create(&main_, &socket_factory_, MakeNetwork(addr), 0, 0, + username_, password_, true)); } - void SetLocalhostAsAlternativeLocalAddress(rtc::VirtualSocket* socket, - const SocketAddress& address) { - SocketAddress local_address("127.0.0.1", 2000); - socket->SetAlternativeLocalAddress(local_address); - } - - TCPPort* CreateTCPPort(const SocketAddress& addr) { - return TCPPort::Create(&main_, &socket_factory_, &network_, addr.ipaddr(), - 0, 0, username_, password_, true); + std::unique_ptr CreateTCPPort(rtc::Network* network) { + return std::unique_ptr(TCPPort::Create( + &main_, &socket_factory_, network, 0, 0, username_, password_, true)); } protected: + // When a "create port" helper method is called with an IP, we create a + // Network with that IP and add it to this list. Using a list instead of a + // vector so that when it grows, pointers aren't invalidated. + std::list networks_; std::unique_ptr ss_; rtc::AutoSocketServerThread main_; - rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::string username_; std::string password_; }; TEST_F(TCPPortTest, TestTCPPortWithLocalhostAddress) { - std::unique_ptr lport(CreateTCPPort(kLocalAddr)); - std::unique_ptr rport(CreateTCPPort(kRemoteAddr)); - lport->PrepareAddress(); - rport->PrepareAddress(); - // Start to listen to new socket creation event. - ConnectSignalSocketCreated(); - Connection* conn = - lport->CreateConnection(rport->Candidates()[0], Port::ORIGIN_MESSAGE); + SocketAddress local_address("127.0.0.1", 0); + // After calling this, when TCPPort attempts to get a socket bound to + // kLocalAddr, it will end up using localhost instead. + ss_->SetAlternativeLocalAddress(kLocalAddr.ipaddr(), local_address.ipaddr()); + auto local_port = CreateTCPPort(kLocalAddr); + auto remote_port = CreateTCPPort(kRemoteAddr); + local_port->PrepareAddress(); + remote_port->PrepareAddress(); + Connection* conn = local_port->CreateConnection(remote_port->Candidates()[0], + Port::ORIGIN_MESSAGE); EXPECT_TRUE_WAIT(conn->connected(), kTimeout); + // Verify that the socket actually used localhost, otherwise this test isn't + // doing what it meant to. + ASSERT_EQ(local_address.ipaddr(), + local_port->Candidates()[0].address().ipaddr()); +} + +// If the address the socket ends up bound to does not match any address of the +// TCPPort's Network, then the socket should be discarded and no candidates +// should be signaled. In the context of ICE, where one TCPPort is created for +// each Network, when this happens it's likely that the unexpected address is +// associated with some other Network, which another TCPPort is already +// covering. +TEST_F(TCPPortTest, TCPPortDiscardedIfBoundAddressDoesNotMatchNetwork) { + // Sockets bound to kLocalAddr will actually end up with kAlternateLocalAddr. + ss_->SetAlternativeLocalAddress(kLocalAddr.ipaddr(), + kAlternateLocalAddr.ipaddr()); + + // Create ports (local_port is the one whose IP will end up reassigned). + auto local_port = CreateTCPPort(kLocalAddr); + auto remote_port = CreateTCPPort(kRemoteAddr); + local_port->PrepareAddress(); + remote_port->PrepareAddress(); + + // Tell port to create a connection; it should be destroyed when it's + // realized that it's using an unexpected address. + Connection* conn = local_port->CreateConnection(remote_port->Candidates()[0], + Port::ORIGIN_MESSAGE); + ConnectionObserver observer(conn); + EXPECT_TRUE_WAIT(observer.connection_destroyed(), kTimeout); +} + +// A caveat for the above logic: if the socket ends up bound to one of the IPs +// associated with the Network, just not the "best" one, this is ok. +TEST_F(TCPPortTest, TCPPortNotDiscardedIfNotBoundToBestIP) { + // Sockets bound to kLocalAddr will actually end up with kAlternateLocalAddr. + ss_->SetAlternativeLocalAddress(kLocalAddr.ipaddr(), + kAlternateLocalAddr.ipaddr()); + + // Set up a network with kLocalAddr1 as the "best" IP, and kAlternateLocalAddr + // as an alternate. + rtc::Network* network = MakeNetwork(kLocalAddr); + network->AddIP(kAlternateLocalAddr.ipaddr()); + ASSERT_EQ(kLocalAddr.ipaddr(), network->GetBestIP()); + + // Create ports (using our special 2-IP Network for local_port). + auto local_port = CreateTCPPort(network); + auto remote_port = CreateTCPPort(kRemoteAddr); + local_port->PrepareAddress(); + remote_port->PrepareAddress(); + + // Expect connection to succeed. + Connection* conn = local_port->CreateConnection(remote_port->Candidates()[0], + Port::ORIGIN_MESSAGE); + EXPECT_TRUE_WAIT(conn->connected(), kTimeout); + + // Verify that the socket actually used the alternate address, otherwise this + // test isn't doing what it meant to. + ASSERT_EQ(kAlternateLocalAddr.ipaddr(), + local_port->Candidates()[0].address().ipaddr()); } class SentPacketCounter : public sigslot::has_slots<> { diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc index 94f54dc0a2..2828feac3f 100644 --- a/webrtc/p2p/base/turnport.cc +++ b/webrtc/p2p/base/turnport.cc @@ -10,6 +10,7 @@ #include "webrtc/p2p/base/turnport.h" +#include #include #include "webrtc/p2p/base/common.h" @@ -194,7 +195,6 @@ TurnPort::TurnPort(rtc::Thread* thread, RELAY_PORT_TYPE, factory, network, - socket->GetLocalAddress().ipaddr(), username, password), server_address_(server_address), @@ -214,7 +214,6 @@ TurnPort::TurnPort(rtc::Thread* thread, TurnPort::TurnPort(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, @@ -227,7 +226,6 @@ TurnPort::TurnPort(rtc::Thread* thread, RELAY_PORT_TYPE, factory, network, - ip, min_port, max_port, username, @@ -293,7 +291,7 @@ void TurnPort::PrepareAddress() { if (!IsCompatibleAddress(server_address_.address)) { LOG(LS_ERROR) << "IP address family does not match: " << "server: " << server_address_.address.family() - << " local: " << ip().family(); + << " local: " << Network()->GetBestIP().family(); OnAllocateError(); return; } @@ -322,7 +320,7 @@ bool TurnPort::CreateTurnClientSocket() { if (server_address_.proto == PROTO_UDP && !SharedSocket()) { socket_ = socket_factory()->CreateUdpSocket( - rtc::SocketAddress(ip(), 0), min_port(), max_port()); + rtc::SocketAddress(Network()->GetBestIP(), 0), min_port(), max_port()); } else if (server_address_.proto == PROTO_TCP || server_address_.proto == PROTO_TLS) { RTC_DCHECK(!SharedSocket()); @@ -339,7 +337,7 @@ bool TurnPort::CreateTurnClientSocket() { } socket_ = socket_factory()->CreateClientTcpSocket( - rtc::SocketAddress(ip(), 0), server_address_.address, + rtc::SocketAddress(Network()->GetBestIP(), 0), server_address_.address, proxy(), user_agent(), opts); } @@ -381,33 +379,43 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { RTC_DCHECK(server_address_.proto == PROTO_TCP || server_address_.proto == PROTO_TLS); - // Do not use this port if the socket bound to a different address than - // the one we asked for. This is seen in Chrome, where TCP sockets cannot be - // given a binding address, and the platform is expected to pick the - // correct local address. - + // Do not use this port if the socket bound to an address not associated with + // the desired network interface. This is seen in Chrome, where TCP sockets + // cannot be given a binding address, and the platform is expected to pick + // the correct local address. + // // However, there are two situations in which we allow the bound address to - // differ from the requested address: 1. The bound address is the loopback - // address. This happens when a proxy forces TCP to bind to only the - // localhost address (see issue 3927). 2. The bound address is the "any - // address". This happens when multiple_routes is disabled (see issue 4780). - if (socket->GetLocalAddress().ipaddr() != ip()) { + // not be one of the addresses of the requested interface: + // 1. The bound address is the loopback address. This happens when a proxy + // forces TCP to bind to only the localhost address (see issue 3927). + // 2. The bound address is the "any address". This happens when + // multiple_routes is disabled (see issue 4780). + // + // Note that, aside from minor differences in log statements, this logic is + // identical to that in TcpPort. + const rtc::SocketAddress& socket_address = socket->GetLocalAddress(); + const std::vector& desired_addresses = + Network()->GetIPs(); + if (std::find(desired_addresses.begin(), desired_addresses.end(), + socket_address.ipaddr()) == desired_addresses.end()) { if (socket->GetLocalAddress().IsLoopbackIP()) { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket->GetLocalAddress().ipaddr().ToString() - << ", rather then the local port:" << ip().ToString() + LOG(LS_WARNING) << "Socket is bound to the address:" + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << Network()->ToString() << ". Still allowing it since it's localhost."; - } else if (IPIsAny(ip())) { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket->GetLocalAddress().ipaddr().ToString() - << ", rather then the local port:" << ip().ToString() - << ". Still allowing it since it's any address" + } else if (IPIsAny(Network()->GetBestIP())) { + LOG(LS_WARNING) << "Socket is bound to the address:" + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << Network()->ToString() + << ". Still allowing it since it's the 'any' address" << ", possibly caused by multiple_routes being disabled."; } else { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket->GetLocalAddress().ipaddr().ToString() - << ", rather then the local port:" << ip().ToString() - << ". Discarding TURN port."; + LOG(LS_WARNING) << "Socket is bound to the address:" + << socket_address.ipaddr().ToString() + << ", rather then an address associated with network:" + << Network()->ToString() << ". Discarding TURN port."; OnAllocateError(); return; } @@ -701,7 +709,8 @@ void TurnPort::OnResolveResult(rtc::AsyncResolverInterface* resolver) { // sockets we need hostname along with resolved address. rtc::SocketAddress resolved_address = server_address_.address; if (resolver_->GetError() != 0 || - !resolver_->GetResolvedAddress(ip().family(), &resolved_address)) { + !resolver_->GetResolvedAddress(Network()->GetBestIP().family(), + &resolved_address)) { LOG_J(LS_WARNING, this) << "TURN host lookup received error " << resolver_->GetError(); error_ = resolver_->GetError(); diff --git a/webrtc/p2p/base/turnport.h b/webrtc/p2p/base/turnport.h index 4e4d4b1774..abdaa3dcb4 100644 --- a/webrtc/p2p/base/turnport.h +++ b/webrtc/p2p/base/turnport.h @@ -42,6 +42,7 @@ class TurnPort : public Port { STATE_DISCONNECTED, // TCP connection died, cannot send/receive any // packets. }; + // Create a TURN port using the shared UDP socket, |socket|. static TurnPort* Create(rtc::Thread* thread, rtc::PacketSocketFactory* factory, rtc::Network* network, @@ -56,10 +57,11 @@ class TurnPort : public Port { server_address, credentials, server_priority, origin); } + // Create a TURN port that will use a new socket, bound to |network| and + // using a port in the range between |min_port| and |max_port|. static TurnPort* 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, // ice username. @@ -68,9 +70,9 @@ class TurnPort : public Port { const RelayCredentials& credentials, int server_priority, const std::string& origin) { - return new TurnPort(thread, factory, network, ip, min_port, max_port, - username, password, server_address, credentials, - server_priority, origin); + return new TurnPort(thread, factory, network, min_port, max_port, username, + password, server_address, credentials, server_priority, + origin); } virtual ~TurnPort(); @@ -177,7 +179,6 @@ class TurnPort : public Port { TurnPort(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, diff --git a/webrtc/p2p/base/turnport_unittest.cc b/webrtc/p2p/base/turnport_unittest.cc index 73426cc505..7e0e291def 100644 --- a/webrtc/p2p/base/turnport_unittest.cc +++ b/webrtc/p2p/base/turnport_unittest.cc @@ -11,6 +11,7 @@ #include #endif +#include #include #include "webrtc/p2p/base/basicpacketsocketfactory.h" @@ -143,7 +144,6 @@ class TurnPortTest : public testing::Test, TurnPortTest() : ss_(new TurnPortTestVirtualSocketServer()), main_(ss_.get()), - network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr), turn_ready_(false), @@ -156,7 +156,6 @@ class TurnPortTest : public testing::Test, // so far", so we need to start the fake clock at a nonzero time... // TODO(deadbeef): Fix this. fake_clock_.AdvanceTime(rtc::TimeDelta::FromSeconds(1)); - network_.AddIP(rtc::IPAddress(INADDR_ANY)); } virtual void OnMessage(rtc::Message* msg) { @@ -165,21 +164,6 @@ class TurnPortTest : public testing::Test, test_finish_ = true; } - void ConnectSignalAddressReadyToSetLocalhostAsAltenertativeLocalAddress() { - rtc::AsyncPacketSocket* socket = turn_port_->socket(); - rtc::VirtualSocket* virtual_socket = - ss_->LookupBinding(socket->GetLocalAddress()); - virtual_socket->SignalAddressReady.connect( - this, &TurnPortTest::SetLocalhostAsAltenertativeLocalAddress); - } - - void SetLocalhostAsAltenertativeLocalAddress( - rtc::VirtualSocket* socket, - const rtc::SocketAddress& address) { - SocketAddress local_address("127.0.0.1", 2000); - socket->SetAlternativeLocalAddress(local_address); - } - void OnTurnPortComplete(Port* port) { turn_ready_ = true; } @@ -229,24 +213,24 @@ class TurnPortTest : public testing::Test, return socket; } + rtc::Network* MakeNetwork(const SocketAddress& addr) { + networks_.emplace_back("unittest", "unittest", addr.ipaddr(), 32); + networks_.back().AddIP(addr.ipaddr()); + return &networks_.back(); + } + void CreateTurnPort(const std::string& username, const std::string& password, const ProtocolAddress& server_address) { - CreateTurnPort(kLocalAddr1, username, password, server_address); + CreateTurnPortWithAllParams(MakeNetwork(kLocalAddr1), username, password, + server_address, std::string()); } void CreateTurnPort(const rtc::SocketAddress& local_address, const std::string& username, const std::string& password, const ProtocolAddress& server_address) { - RelayCredentials credentials(username, password); - turn_port_.reset(TurnPort::Create(&main_, &socket_factory_, &network_, - local_address.ipaddr(), 0, 0, - kIceUfrag1, kIcePwd1, - server_address, credentials, 0, - std::string())); - // This TURN port will be the controlling. - turn_port_->SetIceRole(ICEROLE_CONTROLLING); - ConnectSignals(); + CreateTurnPortWithAllParams(MakeNetwork(local_address), username, password, + server_address, std::string()); } // Should be identical to CreateTurnPort but specifies an origin value @@ -256,12 +240,30 @@ class TurnPortTest : public testing::Test, const std::string& password, const ProtocolAddress& server_address, const std::string& origin) { + CreateTurnPortWithAllParams(MakeNetwork(local_address), username, password, + server_address, origin); + } + + void CreateTurnPortWithNetwork(rtc::Network* network, + const std::string& username, + const std::string& password, + const ProtocolAddress& server_address) { + CreateTurnPortWithAllParams(network, username, password, server_address, + std::string()); + } + + // Version of CreateTurnPort that takes all possible parameters; all other + // helper methods call this, such that "SetIceRole" and "ConnectSignals" (and + // possibly other things in the future) only happen in one place. + void CreateTurnPortWithAllParams(rtc::Network* network, + const std::string& username, + const std::string& password, + const ProtocolAddress& server_address, + const std::string& origin) { RelayCredentials credentials(username, password); - turn_port_.reset(TurnPort::Create(&main_, &socket_factory_, &network_, - local_address.ipaddr(), 0, 0, - kIceUfrag1, kIcePwd1, - server_address, credentials, 0, - origin)); + turn_port_.reset(TurnPort::Create(&main_, &socket_factory_, network, 0, 0, + kIceUfrag1, kIcePwd1, server_address, + credentials, 0, origin)); // This TURN port will be the controlling. turn_port_->SetIceRole(ICEROLE_CONTROLLING); ConnectSignals(); @@ -282,8 +284,8 @@ class TurnPortTest : public testing::Test, RelayCredentials credentials(username, password); turn_port_.reset(TurnPort::Create( - &main_, &socket_factory_, &network_, socket_.get(), kIceUfrag1, - kIcePwd1, server_address, credentials, 0, std::string())); + &main_, &socket_factory_, MakeNetwork(kLocalAddr1), socket_.get(), + kIceUfrag1, kIcePwd1, server_address, credentials, 0, std::string())); // This TURN port will be the controlling. turn_port_->SetIceRole(ICEROLE_CONTROLLING); ConnectSignals(); @@ -305,8 +307,8 @@ class TurnPortTest : public testing::Test, void CreateUdpPort() { CreateUdpPort(kLocalAddr2); } void CreateUdpPort(const SocketAddress& address) { - udp_port_.reset(UDPPort::Create(&main_, &socket_factory_, &network_, - address.ipaddr(), 0, 0, kIceUfrag2, + udp_port_.reset(UDPPort::Create(&main_, &socket_factory_, + MakeNetwork(address), 0, 0, kIceUfrag2, kIcePwd2, std::string(), false)); // UDP port will be controlled. udp_port_->SetIceRole(ICEROLE_CONTROLLED); @@ -616,9 +618,12 @@ class TurnPortTest : public testing::Test, protected: rtc::ScopedFakeClock fake_clock_; + // When a "create port" helper method is called with an IP, we create a + // Network with that IP and add it to this list. Using a list instead of a + // vector so that when it grows, pointers aren't invalidated. + std::list networks_; std::unique_ptr ss_; rtc::AutoSocketServerThread main_; - rtc::Network network_; rtc::BasicPacketSocketFactory socket_factory_; std::unique_ptr socket_; TestTurnServer turn_server_; @@ -704,16 +709,79 @@ TEST_F(TurnPortTest, TestTurnTcpAllocate) { // instead the address that TurnPort originally bound to. The candidate pair // impacted by this behavior should still be used. TEST_F(TurnPortTest, TestTurnTcpAllocationWhenProxyChangesAddressToLocalHost) { + SocketAddress local_address("127.0.0.1", 0); + // After calling this, when TurnPort attempts to get a socket bound to + // kLocalAddr, it will end up using localhost instead. + ss_->SetAlternativeLocalAddress(kLocalAddr1.ipaddr(), local_address.ipaddr()); + turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); - CreateTurnPort(kTurnUsername, kTurnPassword, kTurnTcpProtoAddr); + CreateTurnPort(kLocalAddr1, kTurnUsername, kTurnPassword, kTurnTcpProtoAddr); EXPECT_EQ(0, turn_port_->SetOption(rtc::Socket::OPT_SNDBUF, 10 * 1024)); turn_port_->PrepareAddress(); - ConnectSignalAddressReadyToSetLocalhostAsAltenertativeLocalAddress(); EXPECT_TRUE_SIMULATED_WAIT(turn_ready_, kSimulatedRtt * 3, fake_clock_); ASSERT_EQ(1U, turn_port_->Candidates().size()); EXPECT_EQ(kTurnUdpExtAddr.ipaddr(), turn_port_->Candidates()[0].address().ipaddr()); EXPECT_NE(0, turn_port_->Candidates()[0].address().port()); + + // Verify that the socket actually used localhost, otherwise this test isn't + // doing what it meant to. + ASSERT_EQ(local_address.ipaddr(), + turn_port_->Candidates()[0].related_address().ipaddr()); +} + +// If the address the socket ends up bound to does not match any address of the +// TurnPort's Network, then the socket should be discarded and no candidates +// should be signaled. In the context of ICE, where one TurnPort is created for +// each Network, when this happens it's likely that the unexpected address is +// associated with some other Network, which another TurnPort is already +// covering. +TEST_F(TurnPortTest, + TurnTcpAllocationDiscardedIfBoundAddressDoesNotMatchNetwork) { + // Sockets bound to kLocalAddr1 will actually end up with kLocalAddr2. + ss_->SetAlternativeLocalAddress(kLocalAddr1.ipaddr(), kLocalAddr2.ipaddr()); + + // Set up TURN server to use TCP (this logic only exists for TCP). + turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); + + // Create TURN port and tell it to start allocation. + CreateTurnPort(kLocalAddr1, kTurnUsername, kTurnPassword, kTurnTcpProtoAddr); + turn_port_->PrepareAddress(); + + // Shouldn't take more than 1 RTT to realize the bound address isn't the one + // expected. + EXPECT_TRUE_SIMULATED_WAIT(turn_error_, kSimulatedRtt, fake_clock_); +} + +// A caveat for the above logic: if the socket ends up bound to one of the IPs +// associated with the Network, just not the "best" one, this is ok. +TEST_F(TurnPortTest, TurnTcpAllocationNotDiscardedIfNotBoundToBestIP) { + // Sockets bound to kLocalAddr1 will actually end up with kLocalAddr2. + ss_->SetAlternativeLocalAddress(kLocalAddr1.ipaddr(), kLocalAddr2.ipaddr()); + + // Set up a network with kLocalAddr1 as the "best" IP, and kLocalAddr2 as an + // alternate. + rtc::Network* network = MakeNetwork(kLocalAddr1); + network->AddIP(kLocalAddr2.ipaddr()); + ASSERT_EQ(kLocalAddr1.ipaddr(), network->GetBestIP()); + + // Set up TURN server to use TCP (this logic only exists for TCP). + turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); + + // Create TURN port using our special Network, and tell it to start + // allocation. + CreateTurnPortWithNetwork(network, kTurnUsername, kTurnPassword, + kTurnTcpProtoAddr); + turn_port_->PrepareAddress(); + + // Candidate should be gathered as normally. + EXPECT_TRUE_SIMULATED_WAIT(turn_ready_, kSimulatedRtt * 3, fake_clock_); + ASSERT_EQ(1U, turn_port_->Candidates().size()); + + // Verify that the socket actually used the alternate address, otherwise this + // test isn't doing what it meant to. + ASSERT_EQ(kLocalAddr2.ipaddr(), + turn_port_->Candidates()[0].related_address().ipaddr()); } // Testing turn port will attempt to create TCP socket on address resolution diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 091819ac43..ce95895e8a 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -1096,7 +1096,6 @@ AllocationSequence::AllocationSequence(BasicPortAllocatorSession* session, uint32_t flags) : session_(session), network_(network), - ip_(network->GetBestIP()), config_(config), state_(kInit), flags_(flags), @@ -1108,8 +1107,8 @@ AllocationSequence::AllocationSequence(BasicPortAllocatorSession* session, void AllocationSequence::Init() { if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) { udp_socket_.reset(session_->socket_factory()->CreateUdpSocket( - rtc::SocketAddress(ip_, 0), session_->allocator()->min_port(), - session_->allocator()->max_port())); + rtc::SocketAddress(network_->GetBestIP(), 0), + session_->allocator()->min_port(), session_->allocator()->max_port())); if (udp_socket_) { udp_socket_->SignalReadPacket.connect( this, &AllocationSequence::OnReadPacket); @@ -1143,7 +1142,7 @@ void AllocationSequence::DisableEquivalentPhases(rtc::Network* network, return; } - if (!((network == network_) && (ip_ == network->GetBestIP()))) { + if (!((network == network_) && (previous_best_ip_ == network->GetBestIP()))) { // Different network setup; nothing is equivalent. return; } @@ -1173,6 +1172,9 @@ void AllocationSequence::DisableEquivalentPhases(rtc::Network* network, void AllocationSequence::Start() { state_ = kRunning; session_->network_thread()->Post(RTC_FROM_HERE, this, MSG_ALLOCATION_PHASE); + // Take a snapshot of the best IP, so that when DisableEquivalentPhases is + // called next time, we enable all phases if the best IP has since changed. + previous_best_ip_ = network_->GetBestIP(); } void AllocationSequence::Stop() { @@ -1267,7 +1269,7 @@ void AllocationSequence::CreateUDPPorts() { session_->allocator()->origin(), emit_local_candidate_for_anyaddress); } else { port = UDPPort::Create( - session_->network_thread(), session_->socket_factory(), network_, ip_, + session_->network_thread(), session_->socket_factory(), network_, session_->allocator()->min_port(), session_->allocator()->max_port(), session_->username(), session_->password(), session_->allocator()->origin(), emit_local_candidate_for_anyaddress); @@ -1300,13 +1302,11 @@ void AllocationSequence::CreateTCPPorts() { return; } - Port* port = TCPPort::Create(session_->network_thread(), - session_->socket_factory(), - network_, ip_, - session_->allocator()->min_port(), - session_->allocator()->max_port(), - session_->username(), session_->password(), - session_->allocator()->allow_tcp_listen()); + Port* port = TCPPort::Create( + session_->network_thread(), session_->socket_factory(), network_, + session_->allocator()->min_port(), session_->allocator()->max_port(), + session_->username(), session_->password(), + session_->allocator()->allow_tcp_listen()); if (port) { session_->AddAllocatedPort(port, this, true); // Since TCPPort is not created using shared socket, |port| will not be @@ -1330,14 +1330,11 @@ void AllocationSequence::CreateStunPorts() { return; } - StunPort* port = StunPort::Create(session_->network_thread(), - session_->socket_factory(), - network_, ip_, - session_->allocator()->min_port(), - session_->allocator()->max_port(), - session_->username(), session_->password(), - config_->StunServers(), - session_->allocator()->origin()); + StunPort* port = StunPort::Create( + session_->network_thread(), session_->socket_factory(), network_, + session_->allocator()->min_port(), session_->allocator()->max_port(), + session_->username(), session_->password(), config_->StunServers(), + session_->allocator()->origin()); if (port) { session_->AddAllocatedPort(port, this, true); // Since StunPort is not created using shared socket, |port| will not be @@ -1373,12 +1370,10 @@ void AllocationSequence::CreateRelayPorts() { void AllocationSequence::CreateGturnPort(const RelayServerConfig& config) { // TODO(mallinath) - Rename RelayPort to GTurnPort. - RelayPort* port = RelayPort::Create(session_->network_thread(), - session_->socket_factory(), - network_, ip_, - session_->allocator()->min_port(), - session_->allocator()->max_port(), - config_->username, config_->password); + RelayPort* port = RelayPort::Create( + session_->network_thread(), session_->socket_factory(), network_, + session_->allocator()->min_port(), session_->allocator()->max_port(), + config_->username, config_->password); if (port) { // Since RelayPort is not created using shared socket, |port| will not be // added to the dequeue. @@ -1417,12 +1412,12 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { // Do not create a port if the server address family is known and does // not match the local IP address family. int server_ip_family = relay_port->address.ipaddr().family(); - int local_ip_family = ip_.family(); + int local_ip_family = network_->GetBestIP().family(); if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) { LOG(LS_INFO) << "Server and local address families are not compatible. " << "Server address: " << relay_port->address.ipaddr().ToString() - << " Local address: " << ip_.ToString(); + << " Local address: " << network_->GetBestIP().ToString(); continue; } @@ -1444,15 +1439,11 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { // remove entrt from it's map. port->SignalDestroyed.connect(this, &AllocationSequence::OnPortDestroyed); } else { - port = TurnPort::Create(session_->network_thread(), - session_->socket_factory(), - network_, ip_, - session_->allocator()->min_port(), - session_->allocator()->max_port(), - session_->username(), - session_->password(), - *relay_port, config.credentials, config.priority, - session_->allocator()->origin()); + port = TurnPort::Create( + session_->network_thread(), session_->socket_factory(), network_, + session_->allocator()->min_port(), session_->allocator()->max_port(), + session_->username(), session_->password(), *relay_port, + config.credentials, config.priority, session_->allocator()->origin()); } RTC_DCHECK(port != NULL); port->SetTlsCertPolicy(config.tls_cert_policy); diff --git a/webrtc/p2p/client/basicportallocator.h b/webrtc/p2p/client/basicportallocator.h index a991417736..5a4999cce2 100644 --- a/webrtc/p2p/client/basicportallocator.h +++ b/webrtc/p2p/client/basicportallocator.h @@ -365,7 +365,8 @@ class AllocationSequence : public rtc::MessageHandler, BasicPortAllocatorSession* session_; bool network_failed_ = false; rtc::Network* network_; - rtc::IPAddress ip_; + // Compared with the new best IP in DisableEquivalentPhases. + rtc::IPAddress previous_best_ip_; PortConfiguration* config_; State state_; uint32_t flags_; diff --git a/webrtc/rtc_base/virtualsocketserver.cc b/webrtc/rtc_base/virtualsocketserver.cc index e47b01bbcb..23e94a2d65 100644 --- a/webrtc/rtc_base/virtualsocketserver.cc +++ b/webrtc/rtc_base/virtualsocketserver.cc @@ -127,8 +127,6 @@ VirtualSocket::~VirtualSocket() { } SocketAddress VirtualSocket::GetLocalAddress() const { - if (!alternative_local_addr_.IsNil()) - return alternative_local_addr_; return local_addr_; } @@ -140,10 +138,6 @@ void VirtualSocket::SetLocalAddress(const SocketAddress& addr) { local_addr_ = addr; } -void VirtualSocket::SetAlternativeLocalAddress(const SocketAddress& addr) { - alternative_local_addr_ = addr; -} - int VirtualSocket::Bind(const SocketAddress& addr) { if (!local_addr_.IsNil()) { error_ = EINVAL; @@ -642,6 +636,12 @@ void VirtualSocketServer::WakeUp() { wakeup_.Set(); } +void VirtualSocketServer::SetAlternativeLocalAddress( + const rtc::IPAddress& address, + const rtc::IPAddress& alternative) { + alternative_address_mapping_[address] = alternative; +} + bool VirtualSocketServer::ProcessMessagesUntilIdle() { RTC_DCHECK(msg_queue_ == Thread::Current()); stop_on_idle_ = true; @@ -699,12 +699,22 @@ int VirtualSocketServer::Bind(VirtualSocket* socket, int VirtualSocketServer::Bind(VirtualSocket* socket, SocketAddress* addr) { RTC_DCHECK(nullptr != socket); + // Normalize the IP. if (!IPIsUnspec(addr->ipaddr())) { addr->SetIP(addr->ipaddr().Normalized()); } else { RTC_NOTREACHED(); } + // If the IP appears in |alternative_address_mapping_|, meaning the test has + // configured sockets bound to this IP to actually use another IP, replace + // the IP here. + auto alternative = alternative_address_mapping_.find(addr->ipaddr()); + if (alternative != alternative_address_mapping_.end()) { + addr->SetIP(alternative->second); + } + + // Assign a port if not assigned. if (addr->port() == 0) { for (int i = 0; i < kEphemeralPortCount; ++i) { addr->SetPort(GetNextPort()); diff --git a/webrtc/rtc_base/virtualsocketserver.h b/webrtc/rtc_base/virtualsocketserver.h index a08bc0ce38..089e73a13d 100644 --- a/webrtc/rtc_base/virtualsocketserver.h +++ b/webrtc/rtc_base/virtualsocketserver.h @@ -118,6 +118,16 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> { delay_by_ip_[address.ipaddr()] = delay_ms; } + // Used by TurnPortTest and TcpPortTest (for example), to mimic a case where + // a proxy returns the local host address instead of the original one the + // port was bound against. Please see WebRTC issue 3927 for more detail. + // + // If SetAlternativeLocalAddress(A, B) is called, then when something + // attempts to bind a socket to address A, it will get a socket bound to + // address B instead. + void SetAlternativeLocalAddress(const rtc::IPAddress& address, + const rtc::IPAddress& alternative); + typedef std::pair Point; typedef std::vector Function; @@ -273,6 +283,7 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> { uint32_t delay_samples_; std::map delay_by_ip_; + std::map alternative_address_mapping_; std::unique_ptr delay_dist_; CriticalSection delay_crit_; @@ -294,11 +305,6 @@ class VirtualSocket : public AsyncSocket, SocketAddress GetLocalAddress() const override; SocketAddress GetRemoteAddress() const override; - // Used by TurnPortTest to mimic a case where proxy returns local host address - // instead of the original one TurnPort was bound against. Please see WebRTC - // issue 3927 for more detail. - void SetAlternativeLocalAddress(const SocketAddress& addr); - int Bind(const SocketAddress& addr) override; int Connect(const SocketAddress& addr) override; int Close() override; @@ -353,7 +359,6 @@ class VirtualSocket : public AsyncSocket, ConnState state_; int error_; SocketAddress local_addr_; - SocketAddress alternative_local_addr_; SocketAddress remote_addr_; // Pending sockets which can be Accepted