From b19eba3d4bbc70ece91d524e21e2e9d4253ff7a9 Mon Sep 17 00:00:00 2001 From: honghaiz Date: Mon, 3 Aug 2015 10:23:31 -0700 Subject: [PATCH] Fix Turn TCP port issue. Sometimes the port still try to send stun packet when the connection is disconnected, causing an assertion error. BUG=4859 Review URL: https://codereview.webrtc.org/1247573002 Cr-Commit-Position: refs/heads/master@{#9671} --- webrtc/base/asynctcpsocket.cc | 7 ++-- webrtc/p2p/base/turnport.cc | 58 ++++++++++++++++++++-------- webrtc/p2p/base/turnport.h | 13 ++++++- webrtc/p2p/base/turnport_unittest.cc | 22 +++++++++++ 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/webrtc/base/asynctcpsocket.cc b/webrtc/base/asynctcpsocket.cc index 0f7abd5a6c..97d1e17176 100644 --- a/webrtc/base/asynctcpsocket.cc +++ b/webrtc/base/asynctcpsocket.cc @@ -127,10 +127,11 @@ void AsyncTCPSocketBase::SetError(int error) { int AsyncTCPSocketBase::SendTo(const void *pv, size_t cb, const SocketAddress& addr, const rtc::PacketOptions& options) { - if (addr == GetRemoteAddress()) + const SocketAddress& remote_address = GetRemoteAddress(); + if (addr == remote_address) return Send(pv, cb, options); - - ASSERT(false); + // Remote address may be empty if there is a sudden network change. + ASSERT(remote_address.IsNil()); socket_->SetError(ENOTCONN); return -1; } diff --git a/webrtc/p2p/base/turnport.cc b/webrtc/p2p/base/turnport.cc index 27f88dc7b8..2c9cd13045 100644 --- a/webrtc/p2p/base/turnport.cc +++ b/webrtc/p2p/base/turnport.cc @@ -172,8 +172,12 @@ TurnPort::TurnPort(rtc::Thread* thread, const RelayCredentials& credentials, int server_priority, const std::string& origin) - : Port(thread, factory, network, socket->GetLocalAddress().ipaddr(), - username, password), + : Port(thread, + factory, + network, + socket->GetLocalAddress().ipaddr(), + username, + password), server_address_(server_address), credentials_(credentials), socket_(socket), @@ -181,7 +185,7 @@ TurnPort::TurnPort(rtc::Thread* thread, error_(0), request_manager_(thread), next_channel_number_(TURN_CHANNEL_NUMBER_START), - connected_(false), + state_(STATE_CONNECTING), server_priority_(server_priority), allocate_mismatch_retries_(0) { request_manager_.SignalSendPacket.connect(this, &TurnPort::OnSendStunPacket); @@ -200,8 +204,15 @@ TurnPort::TurnPort(rtc::Thread* thread, const RelayCredentials& credentials, int server_priority, const std::string& origin) - : Port(thread, RELAY_PORT_TYPE, factory, network, ip, min_port, max_port, - username, password), + : Port(thread, + RELAY_PORT_TYPE, + factory, + network, + ip, + min_port, + max_port, + username, + password), server_address_(server_address), credentials_(credentials), socket_(NULL), @@ -209,7 +220,7 @@ TurnPort::TurnPort(rtc::Thread* thread, error_(0), request_manager_(thread), next_channel_number_(TURN_CHANNEL_NUMBER_START), - connected_(false), + state_(STATE_CONNECTING), server_priority_(server_priority), allocate_mismatch_retries_(0) { request_manager_.SignalSendPacket.connect(this, &TurnPort::OnSendStunPacket); @@ -221,7 +232,7 @@ TurnPort::~TurnPort() { // release the allocation by sending a refresh with // lifetime 0. - if (connected_) { + if (ready()) { TurnRefreshRequest bye(this); bye.set_lifetime(0); SendRequest(&bye, 0); @@ -261,8 +272,9 @@ void TurnPort::PrepareAddress() { } else { // If protocol family of server address doesn't match with local, return. if (!IsCompatibleAddress(server_address_.address)) { - LOG(LS_ERROR) << "Server IP address family does not match with " - << "local host address family type"; + LOG(LS_ERROR) << "IP address family does not match: " + << "server: " << server_address_.address.family() + << "local: " << ip().family(); OnAllocateError(); return; } @@ -274,8 +286,11 @@ void TurnPort::PrepareAddress() { << ProtoToString(server_address_.proto) << " @ " << server_address_.address.ToSensitiveString(); if (!CreateTurnClientSocket()) { + LOG(LS_ERROR) << "Failed to create TURN client socket"; OnAllocateError(); - } else if (server_address_.proto == PROTO_UDP) { + return; + } + if (server_address_.proto == PROTO_UDP) { // If its UDP, send AllocateRequest now. // For TCP and TLS AllcateRequest will be sent by OnSocketConnect. SendRequest(new TurnAllocateRequest(this), 0); @@ -319,9 +334,13 @@ bool TurnPort::CreateTurnClientSocket() { socket_->SignalReadyToSend.connect(this, &TurnPort::OnReadyToSend); + // TCP port is ready to send stun requests after the socket is connected, + // while UDP port is ready to do so once the socket is created. if (server_address_.proto == PROTO_TCP) { socket_->SignalConnect.connect(this, &TurnPort::OnSocketConnect); socket_->SignalClose.connect(this, &TurnPort::OnSocketClose); + } else { + state_ = STATE_CONNECTED; } return true; } @@ -360,6 +379,7 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { } } + state_ = STATE_CONNECTED; // It is ready to send stun requests. if (server_address_.address.IsUnresolved()) { server_address_.address = socket_->GetRemoteAddress(); } @@ -372,10 +392,11 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { void TurnPort::OnSocketClose(rtc::AsyncPacketSocket* socket, int error) { LOG_J(LS_WARNING, this) << "Connection with server failed, error=" << error; ASSERT(socket == socket_); - if (!connected_) { + if (!ready()) { OnAllocateError(); } - connected_ = false; + request_manager_.Clear(); + state_ = STATE_DISCONNECTED; } void TurnPort::OnAllocateMismatch() { @@ -412,6 +433,10 @@ Connection* TurnPort::CreateConnection(const Candidate& address, return NULL; } + if (state_ == STATE_DISCONNECTED) { + return NULL; + } + // Create an entry, if needed, so we can get our permissions set up correctly. CreateEntry(address.address()); @@ -462,12 +487,12 @@ int TurnPort::SendTo(const void* data, size_t size, bool payload) { // Try to find an entry for this specific address; we should have one. TurnEntry* entry = FindEntry(addr); - ASSERT(entry != NULL); if (!entry) { + LOG(LS_ERROR) << "Did not find the TurnEntry for address " << addr; return 0; } - if (!connected()) { + if (!ready()) { error_ = EWOULDBLOCK; return SOCKET_ERROR; } @@ -536,7 +561,7 @@ void TurnPort::OnReadPacket( } void TurnPort::OnReadyToSend(rtc::AsyncPacketSocket* socket) { - if (connected_) { + if (ready()) { Port::OnReadyToSend(); } } @@ -616,6 +641,7 @@ void TurnPort::OnResolveResult(rtc::AsyncResolverInterface* resolver) { void TurnPort::OnSendStunPacket(const void* data, size_t size, StunRequest* request) { + ASSERT(connected()); rtc::PacketOptions options(DefaultDscpValue()); if (Send(data, size, options) < 0) { LOG_J(LS_ERROR, this) << "Failed to send TURN message, err=" @@ -635,7 +661,7 @@ void TurnPort::OnStunAddress(const rtc::SocketAddress& address) { void TurnPort::OnAllocateSuccess(const rtc::SocketAddress& address, const rtc::SocketAddress& stun_address) { - connected_ = true; + state_ = STATE_READY; rtc::SocketAddress related_address = stun_address; if (!(candidate_filter() & CF_REFLEXIVE)) { diff --git a/webrtc/p2p/base/turnport.h b/webrtc/p2p/base/turnport.h index 5bb7558598..52546e09a3 100644 --- a/webrtc/p2p/base/turnport.h +++ b/webrtc/p2p/base/turnport.h @@ -33,6 +33,12 @@ class TurnEntry; class TurnPort : public Port { public: + enum PortState { + STATE_CONNECTING, // Initial state, cannot send any packets. + STATE_CONNECTED, // Socket connected, ready to send stun requests. + STATE_READY, // Received allocate success, can send any packets. + STATE_DISCONNECTED, // TCP connection died, cannot send any packets. + }; static TurnPort* Create(rtc::Thread* thread, rtc::PacketSocketFactory* factory, rtc::Network* network, @@ -70,7 +76,10 @@ class TurnPort : public Port { // Returns an empty address if the local address has not been assigned. rtc::SocketAddress GetLocalAddress() const; - bool connected() const { return connected_; } + bool ready() const { return state_ == STATE_READY; } + bool connected() const { + return state_ == STATE_READY || state_ == STATE_CONNECTED; + } const RelayCredentials& credentials() const { return credentials_; } virtual void PrepareAddress(); @@ -225,7 +234,7 @@ class TurnPort : public Port { int next_channel_number_; EntryList entries_; - bool connected_; + PortState state_; // By default the value will be set to 0. This value will be used in // calculating the candidate priority. int server_priority_; diff --git a/webrtc/p2p/base/turnport_unittest.cc b/webrtc/p2p/base/turnport_unittest.cc index 3172ba252f..00a62fa70b 100644 --- a/webrtc/p2p/base/turnport_unittest.cc +++ b/webrtc/p2p/base/turnport_unittest.cc @@ -627,6 +627,28 @@ TEST_F(TurnPortTest, TestTurnTcpAllocateMismatch) { EXPECT_NE(first_addr, turn_port_->socket()->GetLocalAddress()); } +// Test that CreateConnection will return null if port becomes disconnected. +TEST_F(TurnPortTest, TestCreateConnectionWhenSocketClosed) { + turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP); + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnTcpProtoAddr); + turn_port_->PrepareAddress(); + ASSERT_TRUE_WAIT(turn_ready_, kTimeout); + + CreateUdpPort(); + udp_port_->PrepareAddress(); + ASSERT_TRUE_WAIT(udp_ready_, kTimeout); + // Create a connection. + Connection* conn1 = turn_port_->CreateConnection(udp_port_->Candidates()[0], + Port::ORIGIN_MESSAGE); + ASSERT_TRUE(conn1 != NULL); + + // Close the socket and create a connection again. + turn_port_->OnSocketClose(turn_port_->socket(), 1); + conn1 = turn_port_->CreateConnection(udp_port_->Candidates()[0], + Port::ORIGIN_MESSAGE); + ASSERT_TRUE(conn1 == NULL); +} + // Test try-alternate-server feature. TEST_F(TurnPortTest, TestTurnAlternateServerUDP) { TestTurnAlternateServer(cricket::PROTO_UDP);