diff --git a/p2p/base/tcpport.cc b/p2p/base/tcpport.cc index b780a1e527..a8bda95950 100644 --- a/p2p/base/tcpport.cc +++ b/p2p/base/tcpport.cc @@ -347,9 +347,11 @@ TCPConnection::TCPConnection(TCPPort* port, << ", 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()); + RTC_DCHECK(std::find_if(desired_addresses.begin(), desired_addresses.end(), + [this](const rtc::InterfaceAddress& addr) { + return socket_->GetLocalAddress().ipaddr() == + addr; + }) != desired_addresses.end()); ConnectSocketSignals(socket); } } @@ -429,8 +431,10 @@ void TCPConnection::OnConnect(rtc::AsyncPacketSocket* socket) { 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()) { + if (std::find_if(desired_addresses.begin(), desired_addresses.end(), + [socket_address](const rtc::InterfaceAddress& addr) { + return socket_address.ipaddr() == addr; + }) != desired_addresses.end()) { LOG_J(LS_VERBOSE, this) << "Connection established to " << socket->GetRemoteAddress().ToSensitiveString(); } else { diff --git a/p2p/base/tcpport_unittest.cc b/p2p/base/tcpport_unittest.cc index 12bb437517..6d3fe32fe3 100644 --- a/p2p/base/tcpport_unittest.cc +++ b/p2p/base/tcpport_unittest.cc @@ -26,8 +26,12 @@ using cricket::ICE_PWD_LENGTH; static int kTimeout = 1000; static const SocketAddress kLocalAddr("11.11.11.11", 0); +static const SocketAddress kLocalIPv6Addr("2401:fa00:4:1000:be30:5bff:fee5:c3", + 0); static const SocketAddress kAlternateLocalAddr("1.2.3.4", 0); static const SocketAddress kRemoteAddr("22.22.22.22", 0); +static const SocketAddress kRemoteIPv6Addr("2401:fa00:4:1000:be30:5bff:fee5:c4", + 0); class ConnectionObserver : public sigslot::has_slots<> { public: @@ -155,6 +159,25 @@ TEST_F(TCPPortTest, TCPPortNotDiscardedIfNotBoundToBestIP) { local_port->Candidates()[0].address().ipaddr()); } +// Regression test for crbug.com/webrtc/8972, caused by buggy comparison +// between rtc::IPAddress and rtc::InterfaceAddress. +TEST_F(TCPPortTest, TCPPortNotDiscardedIfBoundToTemporaryIP) { + networks_.emplace_back("unittest", "unittest", kLocalIPv6Addr.ipaddr(), 32); + networks_.back().AddIP(rtc::InterfaceAddress( + kLocalIPv6Addr.ipaddr(), rtc::IPV6_ADDRESS_FLAG_TEMPORARY)); + + auto local_port = CreateTCPPort(&networks_.back()); + auto remote_port = CreateTCPPort(kRemoteIPv6Addr); + local_port->PrepareAddress(); + remote_port->PrepareAddress(); + + // Connection should succeed if the port isn't discarded. + Connection* conn = local_port->CreateConnection(remote_port->Candidates()[0], + Port::ORIGIN_MESSAGE); + ASSERT_NE(nullptr, conn); + EXPECT_TRUE_WAIT(conn->connected(), kTimeout); +} + class SentPacketCounter : public sigslot::has_slots<> { public: explicit SentPacketCounter(TCPPort* p) { diff --git a/p2p/base/turnport.cc b/p2p/base/turnport.cc index ab23318a0f..78746ea9b5 100644 --- a/p2p/base/turnport.cc +++ b/p2p/base/turnport.cc @@ -436,8 +436,10 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { 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 (std::find_if(desired_addresses.begin(), desired_addresses.end(), + [socket_address](const rtc::InterfaceAddress& addr) { + return socket_address.ipaddr() == addr; + }) == desired_addresses.end()) { if (socket->GetLocalAddress().IsLoopbackIP()) { RTC_LOG(LS_WARNING) << "Socket is bound to the address:" << socket_address.ipaddr().ToString() diff --git a/p2p/base/turnport_unittest.cc b/p2p/base/turnport_unittest.cc index 40092c5571..d68beafdc3 100644 --- a/p2p/base/turnport_unittest.cc +++ b/p2p/base/turnport_unittest.cc @@ -840,6 +840,28 @@ TEST_F(TurnPortTest, TurnTcpAllocationNotDiscardedIfNotBoundToBestIP) { turn_port_->Candidates()[0].related_address().ipaddr()); } +// Regression test for crbug.com/webrtc/8972, caused by buggy comparison +// between rtc::IPAddress and rtc::InterfaceAddress. +TEST_F(TurnPortTest, TCPPortNotDiscardedIfBoundToTemporaryIP) { + networks_.emplace_back("unittest", "unittest", kLocalIPv6Addr.ipaddr(), 32); + networks_.back().AddIP(rtc::InterfaceAddress( + kLocalIPv6Addr.ipaddr(), rtc::IPV6_ADDRESS_FLAG_TEMPORARY)); + + // Set up TURN server to use TCP (this logic only exists for TCP). + turn_server_.AddInternalSocket(kTurnIPv6IntAddr, PROTO_TCP); + + // Create TURN port using our special Network, and tell it to start + // allocation. + CreateTurnPortWithNetwork( + &networks_.back(), kTurnUsername, kTurnPassword, + cricket::ProtocolAddress(kTurnIPv6IntAddr, PROTO_TCP)); + 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()); +} + // Testing turn port will attempt to create TCP socket on address resolution // failure. TEST_F(TurnPortTest, TestTurnTcpOnAddressResolveFailure) { diff --git a/rtc_base/ifaddrs_converter.cc b/rtc_base/ifaddrs_converter.cc index 2db99efd6c..586b4e9edf 100644 --- a/rtc_base/ifaddrs_converter.cc +++ b/rtc_base/ifaddrs_converter.cc @@ -22,8 +22,8 @@ bool IfAddrsConverter::ConvertIfAddrsToIPAddress( IPAddress* mask) { switch (interface->ifa_addr->sa_family) { case AF_INET: { - *ip = IPAddress( - reinterpret_cast(interface->ifa_addr)->sin_addr); + *ip = InterfaceAddress(IPAddress( + reinterpret_cast(interface->ifa_addr)->sin_addr)); *mask = IPAddress( reinterpret_cast(interface->ifa_netmask)->sin_addr); return true; diff --git a/rtc_base/ipaddress.h b/rtc_base/ipaddress.h index 4ef7d085b6..63543ba076 100644 --- a/rtc_base/ipaddress.h +++ b/rtc_base/ipaddress.h @@ -126,8 +126,8 @@ class InterfaceAddress : public IPAddress { public: InterfaceAddress() : ipv6_flags_(IPV6_ADDRESS_FLAG_NONE) {} - InterfaceAddress(IPAddress ip) - : IPAddress(ip), ipv6_flags_(IPV6_ADDRESS_FLAG_NONE) {} + explicit InterfaceAddress(IPAddress ip) + : IPAddress(ip), ipv6_flags_(IPV6_ADDRESS_FLAG_NONE) {} InterfaceAddress(IPAddress addr, int ipv6_flags) : IPAddress(addr), ipv6_flags_(ipv6_flags) {} diff --git a/rtc_base/network.cc b/rtc_base/network.cc index 6d59888803..7d543c07cd 100644 --- a/rtc_base/network.cc +++ b/rtc_base/network.cc @@ -633,7 +633,7 @@ bool BasicNetworkManager::CreateNetworks(bool include_ignored, scope_id = v6_addr->sin6_scope_id; ip = IPAddress(v6_addr->sin6_addr); - if (IsIgnoredIPv6(ip)) { + if (IsIgnoredIPv6(InterfaceAddress(ip))) { continue; } diff --git a/rtc_base/network.h b/rtc_base/network.h index 49934cc3ab..2b0f377a3f 100644 --- a/rtc_base/network.h +++ b/rtc_base/network.h @@ -346,6 +346,7 @@ class Network { // Adds an active IP address to this network. Does not check for duplicates. void AddIP(const InterfaceAddress& ip) { ips_.push_back(ip); } + void AddIP(const IPAddress& ip) { ips_.push_back(rtc::InterfaceAddress(ip)); } // Sets the network's IP address list. Returns true if new IP addresses were // detected. Passing true to already_changed skips this check. diff --git a/rtc_base/network_unittest.cc b/rtc_base/network_unittest.cc index bf09c2456d..f4dcc6c86b 100644 --- a/rtc_base/network_unittest.cc +++ b/rtc_base/network_unittest.cc @@ -552,19 +552,16 @@ TEST_F(NetworkTest, TestMultipleIPMergeNetworkList) { // But with two addresses now. EXPECT_EQ(2U, (*it)->GetIPs().size()); EXPECT_NE((*it)->GetIPs().end(), - std::find((*it)->GetIPs().begin(), - (*it)->GetIPs().end(), - check_ip)); + std::find((*it)->GetIPs().begin(), (*it)->GetIPs().end(), + InterfaceAddress(check_ip))); EXPECT_NE((*it)->GetIPs().end(), - std::find((*it)->GetIPs().begin(), - (*it)->GetIPs().end(), - ip)); + std::find((*it)->GetIPs().begin(), (*it)->GetIPs().end(), + InterfaceAddress(ip))); } else { // Check the IP didn't get added anywhere it wasn't supposed to. EXPECT_EQ((*it)->GetIPs().end(), - std::find((*it)->GetIPs().begin(), - (*it)->GetIPs().end(), - ip)); + std::find((*it)->GetIPs().begin(), (*it)->GetIPs().end(), + InterfaceAddress(ip))); } } } @@ -606,9 +603,8 @@ TEST_F(NetworkTest, TestMultiplePublicNetworksOnOneInterfaceMerge) { } else { // Check the IP didn't get added anywhere it wasn't supposed to. EXPECT_EQ((*it)->GetIPs().end(), - std::find((*it)->GetIPs().begin(), - (*it)->GetIPs().end(), - ip)); + std::find((*it)->GetIPs().begin(), (*it)->GetIPs().end(), + InterfaceAddress(ip))); } } } @@ -962,8 +958,8 @@ TEST_F(NetworkTest, TestMergeNetworkList) { // IPAddresses. EXPECT_EQ(list2.size(), 1uL); EXPECT_EQ(list2[0]->GetIPs().size(), 2uL); - EXPECT_EQ(list2[0]->GetIPs()[0], ip1); - EXPECT_EQ(list2[0]->GetIPs()[1], ip2); + EXPECT_EQ(list2[0]->GetIPs()[0], InterfaceAddress(ip1)); + EXPECT_EQ(list2[0]->GetIPs()[1], InterfaceAddress(ip2)); } // Test that MergeNetworkList successfully detects the change if