From 01cb5f2cee307f3ff0a498c7db867644bd307c8c Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 7 Mar 2018 15:49:32 -0800 Subject: [PATCH] Fix issue where sockets bound to temporary IPv6 addresses are discarded. Also removing the implicit InterfaceAddress constructor that takes an IPAddress, so that issues like this won't happen in the future. And adding a convenience "Network::AddIP" method that takes an IPAddress, so that code doing that (previously relying on the implicit constructor) will continue to work. Bug: webrtc:8972 Change-Id: Id5cf0fca481cfee3f8ab83412fcb41886535bba2 Reviewed-on: https://webrtc-review.googlesource.com/59461 Reviewed-by: Peter Thatcher Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22504} --- p2p/base/tcpport.cc | 14 +++++++++----- p2p/base/tcpport_unittest.cc | 23 +++++++++++++++++++++++ p2p/base/turnport.cc | 6 ++++-- p2p/base/turnport_unittest.cc | 22 ++++++++++++++++++++++ rtc_base/ifaddrs_converter.cc | 4 ++-- rtc_base/ipaddress.h | 4 ++-- rtc_base/network.cc | 2 +- rtc_base/network.h | 1 + rtc_base/network_unittest.cc | 24 ++++++++++-------------- 9 files changed, 74 insertions(+), 26 deletions(-) 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