From 5ce1a2a6294f50bb5dade595279cc5f188177dd4 Mon Sep 17 00:00:00 2001 From: tommi Date: Sat, 14 May 2016 03:19:31 -0700 Subject: [PATCH] Reland of Allow the localhost IP address even if it does not match the tcp port address (patchset #1 id:1 of https://codereview.webrtc.org/1979463003/ ) Reason for revert: Relanding this change since the revert didn't make a difference. Original issue's description: > Revert of Allow the localhost IP address even if it does not match the tcp port address (patchset #4 id:120001 of https://codereview.webrtc.org/1914803002/ ) > > Reason for revert: > Speculatively reverting due to failures on the memcheck bot (and possibly other bots): > > https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/5910/steps/video_engine_tests/logs/EndToEndTest.SendsAndReceivesH264 > > Original issue's description: > > This fixes an issue similar to > > https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 > > where the localhost IP does not match the turn port address. > > The issue here is in the TCP port. > > > > BUG= > > R=pthatcher@webrtc.org > > > > Committed: https://crrev.com/6705012904e6cbbefce6fbce0a3f615b7aeafd8f > > Cr-Commit-Position: refs/heads/master@{#12707} > > TBR=pthatcher@webrtc.org,deadbeef@webrtc.org,honghaiz@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= > > Committed: https://crrev.com/1cbf0a73eb4b475e8beb878ea3a4d650191f0c08 > Cr-Commit-Position: refs/heads/master@{#12728} TBR=pthatcher@webrtc.org,deadbeef@webrtc.org,honghaiz@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review-Url: https://codereview.webrtc.org/1979073002 Cr-Commit-Position: refs/heads/master@{#12746} --- webrtc/base/virtualsocketserver.cc | 4 +- webrtc/base/virtualsocketserver.h | 3 + webrtc/p2p/base/tcpport.cc | 39 +++++++------ webrtc/p2p/base/tcpport_unittest.cc | 85 +++++++++++++++++++++++++++++ webrtc/p2p/p2p.gyp | 1 + 5 files changed, 115 insertions(+), 17 deletions(-) create mode 100644 webrtc/p2p/base/tcpport_unittest.cc diff --git a/webrtc/base/virtualsocketserver.cc b/webrtc/base/virtualsocketserver.cc index c6952b4b99..c76fe42f1e 100644 --- a/webrtc/base/virtualsocketserver.cc +++ b/webrtc/base/virtualsocketserver.cc @@ -578,7 +578,9 @@ AsyncSocket* VirtualSocketServer::CreateAsyncSocket(int family, int type) { } VirtualSocket* VirtualSocketServer::CreateSocketInternal(int family, int type) { - return new VirtualSocket(this, family, type, true); + VirtualSocket* socket = new VirtualSocket(this, family, type, true); + SignalSocketCreated(socket); + return socket; } void VirtualSocketServer::SetMessageQueue(MessageQueue* msg_queue) { diff --git a/webrtc/base/virtualsocketserver.h b/webrtc/base/virtualsocketserver.h index a83be28522..897ba9e5eb 100644 --- a/webrtc/base/virtualsocketserver.h +++ b/webrtc/base/virtualsocketserver.h @@ -122,6 +122,9 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> { bool CloseTcpConnections(const SocketAddress& addr_local, const SocketAddress& addr_remote); + // For testing purpose only. Fired when a client socket is created. + sigslot::signal1 SignalSocketCreated; + protected: // Returns a new IP not used before in this network. IPAddress GetNextIP(int family); diff --git a/webrtc/p2p/base/tcpport.cc b/webrtc/p2p/base/tcpport.cc index 59c4f317d4..5ccb8108a0 100644 --- a/webrtc/p2p/base/tcpport.cc +++ b/webrtc/p2p/base/tcpport.cc @@ -387,28 +387,35 @@ void TCPConnection::OnConnect(rtc::AsyncPacketSocket* socket) { // 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::IPAddress& socket_ip = socket->GetLocalAddress().ipaddr(); - if (socket_ip == port()->ip() || IPIsAny(port()->ip())) { - if (socket_ip == port()->ip()) { - LOG_J(LS_VERBOSE, this) << "Connection established to " - << socket->GetRemoteAddress().ToSensitiveString(); - } else { - LOG(LS_WARNING) << "Socket is bound to a different address:" - << socket->GetLocalAddress().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."; - } - set_connected(true); - connection_pending_ = false; + const rtc::SocketAddress& socket_addr = socket->GetLocalAddress(); + if (socket_addr.ipaddr() == port()->ip()) { + 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_ip.ToSensitiveString() + << socket_addr.ipaddr().ToSensitiveString() << ", different from the local candidate IP " << port()->ip().ToSensitiveString(); OnClose(socket, 0); + return; } + + // Connection is established successfully. + set_connected(true); + connection_pending_ = false; } void TCPConnection::OnClose(rtc::AsyncPacketSocket* socket, int error) { diff --git a/webrtc/p2p/base/tcpport_unittest.cc b/webrtc/p2p/base/tcpport_unittest.cc new file mode 100644 index 0000000000..77fb790c53 --- /dev/null +++ b/webrtc/p2p/base/tcpport_unittest.cc @@ -0,0 +1,85 @@ +/* + * Copyright 2016 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "webrtc/base/gunit.h" +#include "webrtc/base/physicalsocketserver.h" +#include "webrtc/base/thread.h" +#include "webrtc/base/virtualsocketserver.h" +#include "webrtc/p2p/base/basicpacketsocketfactory.h" +#include "webrtc/p2p/base/tcpport.h" + +using rtc::SocketAddress; +using cricket::Connection; +using cricket::Port; +using cricket::TCPPort; +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); + +class TCPPortTest : public testing::Test, public sigslot::has_slots<> { + public: + TCPPortTest() + : main_(rtc::Thread::Current()), + pss_(new rtc::PhysicalSocketServer), + ss_(new rtc::VirtualSocketServer(pss_.get())), + ss_scope_(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); + } + + void OnSocketCreated(rtc::VirtualSocket* socket) { + LOG(LS_INFO) << "socket created "; + socket->SignalAddressReady.connect( + this, &TCPPortTest::SetLocalhostAsAlternativeLocalAddress); + } + + 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); + } + + protected: + rtc::Thread* main_; + rtc::scoped_ptr pss_; + rtc::scoped_ptr ss_; + rtc::SocketServerScope ss_scope_; + rtc::Network network_; + rtc::BasicPacketSocketFactory socket_factory_; + std::string username_; + std::string password_; +}; + +TEST_F(TCPPortTest, TestTCPPortWithLocalhostAddress) { + rtc::scoped_ptr lport(CreateTCPPort(kLocalAddr)); + rtc::scoped_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); + EXPECT_TRUE_WAIT(conn->connected(), kTimeout); +} diff --git a/webrtc/p2p/p2p.gyp b/webrtc/p2p/p2p.gyp index b8ef090587..7ec1814bc6 100644 --- a/webrtc/p2p/p2p.gyp +++ b/webrtc/p2p/p2p.gyp @@ -164,6 +164,7 @@ 'base/transport_unittest.cc', 'base/transportcontroller_unittest.cc', 'base/transportdescriptionfactory_unittest.cc', + 'base/tcpport_unittest.cc', 'base/turnport_unittest.cc', 'client/basicportallocator_unittest.cc', 'stunprober/stunprober_unittest.cc',