From 1cbf0a73eb4b475e8beb878ea3a4d650191f0c08 Mon Sep 17 00:00:00 2001 From: tommi Date: Fri, 13 May 2016 07:39:38 -0700 Subject: [PATCH] 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= Review-Url: https://codereview.webrtc.org/1979463003 Cr-Commit-Position: refs/heads/master@{#12728} --- 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, 17 insertions(+), 115 deletions(-) delete mode 100644 webrtc/p2p/base/tcpport_unittest.cc diff --git a/webrtc/base/virtualsocketserver.cc b/webrtc/base/virtualsocketserver.cc index c76fe42f1e..c6952b4b99 100644 --- a/webrtc/base/virtualsocketserver.cc +++ b/webrtc/base/virtualsocketserver.cc @@ -578,9 +578,7 @@ AsyncSocket* VirtualSocketServer::CreateAsyncSocket(int family, int type) { } VirtualSocket* VirtualSocketServer::CreateSocketInternal(int family, int type) { - VirtualSocket* socket = new VirtualSocket(this, family, type, true); - SignalSocketCreated(socket); - return socket; + return new VirtualSocket(this, family, type, true); } void VirtualSocketServer::SetMessageQueue(MessageQueue* msg_queue) { diff --git a/webrtc/base/virtualsocketserver.h b/webrtc/base/virtualsocketserver.h index 897ba9e5eb..a83be28522 100644 --- a/webrtc/base/virtualsocketserver.h +++ b/webrtc/base/virtualsocketserver.h @@ -122,9 +122,6 @@ 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 5ccb8108a0..59c4f317d4 100644 --- a/webrtc/p2p/base/tcpport.cc +++ b/webrtc/p2p/base/tcpport.cc @@ -387,35 +387,28 @@ 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::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."; + 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; } else { LOG_J(LS_WARNING, this) << "Dropping connection as TCP socket bound to IP " - << socket_addr.ipaddr().ToSensitiveString() + << socket_ip.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 deleted file mode 100644 index 77fb790c53..0000000000 --- a/webrtc/p2p/base/tcpport_unittest.cc +++ /dev/null @@ -1,85 +0,0 @@ -/* - * 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 08fac6929d..a15fa52bb5 100644 --- a/webrtc/p2p/p2p.gyp +++ b/webrtc/p2p/p2p.gyp @@ -162,7 +162,6 @@ 'base/transport_unittest.cc', 'base/transportcontroller_unittest.cc', 'base/transportdescriptionfactory_unittest.cc', - 'base/tcpport_unittest.cc', 'base/turnport_unittest.cc', 'client/fakeportallocator.h', 'client/portallocator_unittest.cc',