admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix issue where sockets bound to temporary IPv6 addresses are discarded.

Browse Source 
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 <pthatcher@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22504}
This commit is contained in:
Taylor Brandstetter 2018-03-07 15:49:32 -08:00 committed by Commit Bot
parent 3d976f6066
commit 01cb5f2cee
9 changed files with 74 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
p2p/base/tcpport.cc
View File

@ -347,9 +347,11 @@ TCPConnection::TCPConnection(TCPPort* port,
<< ", port() Network:" << port->Network()->ToString();
const std::vector<rtc::InterfaceAddress>& 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<rtc::InterfaceAddress>& 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 {

23
p2p/base/tcpport_unittest.cc
View File

@ -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) {

6
p2p/base/turnport.cc
View File

@ -436,8 +436,10 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) {
const rtc::SocketAddress& socket_address = socket->GetLocalAddress();
const std::vector<rtc::InterfaceAddress>& 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()

22
p2p/base/turnport_unittest.cc
View File

@ -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) {

4
rtc_base/ifaddrs_converter.cc
View File

@ -22,8 +22,8 @@ bool IfAddrsConverter::ConvertIfAddrsToIPAddress(
IPAddress* mask) {
switch (interface->ifa_addr->sa_family) {
case AF_INET: {
*ip = IPAddress(
reinterpret_cast<sockaddr_in*>(interface->ifa_addr)->sin_addr);
*ip = InterfaceAddress(IPAddress(
reinterpret_cast<sockaddr_in*>(interface->ifa_addr)->sin_addr));
*mask = IPAddress(
reinterpret_cast<sockaddr_in*>(interface->ifa_netmask)->sin_addr);
return true;

4
rtc_base/ipaddress.h
View File

@ -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) {}

2
rtc_base/network.cc
View File

@ -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;
}

1
rtc_base/network.h
View File

@ -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.

24
rtc_base/network_unittest.cc
View File

@ -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