diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 7aa87fbeba..388fff7786 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -309,7 +309,8 @@ class WebRtcSessionTest : public testing::Test { ss_scope_(fss_.get()), stun_socket_addr_(rtc::SocketAddress(kStunAddrHost, cricket::STUN_SERVER_PORT)), - stun_server_(Thread::Current(), stun_socket_addr_), + stun_server_(cricket::TestStunServer::Create(Thread::Current(), + stun_socket_addr_)), turn_server_(Thread::Current(), kTurnUdpIntAddr, kTurnUdpExtAddr), mediastream_signaling_(channel_manager_.get()), ice_type_(PeerConnectionInterface::kAll) { @@ -1109,7 +1110,7 @@ class WebRtcSessionTest : public testing::Test { rtc::scoped_ptr fss_; rtc::SocketServerScope ss_scope_; rtc::SocketAddress stun_socket_addr_; - cricket::TestStunServer stun_server_; + rtc::scoped_ptr stun_server_; cricket::TestTurnServer turn_server_; rtc::FakeNetworkManager network_manager_; rtc::scoped_ptr allocator_; diff --git a/talk/p2p/base/p2ptransportchannel_unittest.cc b/talk/p2p/base/p2ptransportchannel_unittest.cc index 0b7f882f0a..9bfa84f783 100644 --- a/talk/p2p/base/p2ptransportchannel_unittest.cc +++ b/talk/p2p/base/p2ptransportchannel_unittest.cc @@ -139,7 +139,7 @@ class P2PTransportChannelTestBase : public testing::Test, nss_(new rtc::NATSocketServer(vss_.get())), ss_(new rtc::FirewallSocketServer(nss_.get())), ss_scope_(ss_.get()), - stun_server_(main_, kStunAddr), + stun_server_(cricket::TestStunServer::Create(main_, kStunAddr)), turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr), relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr, kRelayTcpIntAddr, kRelayTcpExtAddr, @@ -745,7 +745,7 @@ class P2PTransportChannelTestBase : public testing::Test, rtc::scoped_ptr nss_; rtc::scoped_ptr ss_; rtc::SocketServerScope ss_scope_; - cricket::TestStunServer stun_server_; + rtc::scoped_ptr stun_server_; cricket::TestTurnServer turn_server_; cricket::TestRelayServer relay_server_; rtc::SocksProxyServer socks_server1_; diff --git a/talk/p2p/base/port_unittest.cc b/talk/p2p/base/port_unittest.cc index 04becfcb7f..a44a12ec8e 100644 --- a/talk/p2p/base/port_unittest.cc +++ b/talk/p2p/base/port_unittest.cc @@ -343,7 +343,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { nat_factory2_(ss_.get(), kNatAddr2), nat_socket_factory1_(&nat_factory1_), nat_socket_factory2_(&nat_factory2_), - stun_server_(main_, kStunAddr), + stun_server_(TestStunServer::Create(main_, kStunAddr)), turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr), relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr, kRelayTcpIntAddr, kRelayTcpExtAddr, @@ -617,7 +617,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { rtc::NATSocketFactory nat_factory2_; rtc::BasicPacketSocketFactory nat_socket_factory1_; rtc::BasicPacketSocketFactory nat_socket_factory2_; - TestStunServer stun_server_; + scoped_ptr stun_server_; TestTurnServer turn_server_; TestRelayServer relay_server_; std::string username_; diff --git a/talk/p2p/base/stunport.cc b/talk/p2p/base/stunport.cc index a178eb4897..a185a35014 100644 --- a/talk/p2p/base/stunport.cc +++ b/talk/p2p/base/stunport.cc @@ -389,10 +389,12 @@ void UDPPort::OnStunBindingRequestSucceeded( } bind_request_succeeded_servers_.insert(stun_server_addr); - if (!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) { - // If socket is shared and |stun_reflected_addr| is equal to local socket - // address then discarding the stun address. - // For STUN related address is local socket address. + // If socket is shared and |stun_reflected_addr| is equal to local socket + // address, or if the same address has been added by another STUN server, + // then discarding the stun address. + // For STUN, related address is the local socket address. + if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) && + !HasCandidateWithAddress(stun_reflected_addr)) { AddAddress(stun_reflected_addr, socket_->GetLocalAddress(), socket_->GetLocalAddress(), UDP_PROTOCOL_NAME, "", STUN_PORT_TYPE, ICE_TYPE_PREFERENCE_SRFLX, 0, false); @@ -443,4 +445,14 @@ void UDPPort::OnSendPacket(const void* data, size_t size, StunRequest* req) { PLOG(LERROR, socket_->GetError()) << "sendto"; } +bool UDPPort::HasCandidateWithAddress(const rtc::SocketAddress& addr) const { + const std::vector& existing_candidates = Candidates(); + std::vector::const_iterator it = existing_candidates.begin(); + for (; it != existing_candidates.end(); ++it) { + if (it->address() == addr) + return true; + } + return false; +} + } // namespace cricket diff --git a/talk/p2p/base/stunport.h b/talk/p2p/base/stunport.h index b3b6d5b1c6..1fc1f64910 100644 --- a/talk/p2p/base/stunport.h +++ b/talk/p2p/base/stunport.h @@ -194,6 +194,8 @@ class UDPPort : public Port { // changed to SignalPortReady. void MaybeSetPortCompleteOrError(); + bool HasCandidateWithAddress(const rtc::SocketAddress& addr) const; + ServerAddresses server_addresses_; ServerAddresses bind_request_succeeded_servers_; ServerAddresses bind_request_failed_servers_; diff --git a/talk/p2p/base/stunport_unittest.cc b/talk/p2p/base/stunport_unittest.cc index 6506181f3b..c5abc48deb 100644 --- a/talk/p2p/base/stunport_unittest.cc +++ b/talk/p2p/base/stunport_unittest.cc @@ -61,9 +61,9 @@ class StunPortTest : public testing::Test, ss_scope_(ss_.get()), network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32), socket_factory_(rtc::Thread::Current()), - stun_server_1_(new cricket::TestStunServer( + stun_server_1_(cricket::TestStunServer::Create( rtc::Thread::Current(), kStunAddr1)), - stun_server_2_(new cricket::TestStunServer( + stun_server_2_(cricket::TestStunServer::Create( rtc::Thread::Current(), kStunAddr2)), done_(false), error_(false), stun_keepalive_delay_(0) { } @@ -150,6 +150,13 @@ class StunPortTest : public testing::Test, stun_keepalive_delay_ = delay; } + cricket::TestStunServer* stun_server_1() { + return stun_server_1_.get(); + } + cricket::TestStunServer* stun_server_2() { + return stun_server_2_.get(); + } + private: rtc::scoped_ptr pss_; rtc::scoped_ptr ss_; @@ -253,8 +260,8 @@ TEST_F(StunPortTest, TestSharedSocketPrepareAddressInvalidHostname) { // No crash is success. } -// Test that candidates can be allocated for multiple STUN servers. -TEST_F(StunPortTest, TestMultipleGoodStunServers) { +// Test that the same address is added only once if two STUN servers are in use. +TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) { ServerAddresses stun_servers; stun_servers.insert(kStunAddr1); stun_servers.insert(kStunAddr2); @@ -262,7 +269,7 @@ TEST_F(StunPortTest, TestMultipleGoodStunServers) { EXPECT_EQ("stun", port()->Type()); PrepareAddress(); EXPECT_TRUE_WAIT(done(), kTimeoutMs); - EXPECT_EQ(2U, port()->Candidates().size()); + EXPECT_EQ(1U, port()->Candidates().size()); } // Test that candidates can be allocated for multiple STUN servers, one of which @@ -277,3 +284,21 @@ TEST_F(StunPortTest, TestMultipleStunServersWithBadServer) { EXPECT_TRUE_WAIT(done(), kTimeoutMs); EXPECT_EQ(1U, port()->Candidates().size()); } + +// Test that two candidates are allocated if the two STUN servers return +// different mapped addresses. +TEST_F(StunPortTest, TestTwoCandidatesWithTwoStunServersAcrossNat) { + const SocketAddress kStunMappedAddr1("77.77.77.77", 0); + const SocketAddress kStunMappedAddr2("88.77.77.77", 0); + stun_server_1()->set_fake_stun_addr(kStunMappedAddr1); + stun_server_2()->set_fake_stun_addr(kStunMappedAddr2); + + ServerAddresses stun_servers; + stun_servers.insert(kStunAddr1); + stun_servers.insert(kStunAddr2); + CreateStunPort(stun_servers); + EXPECT_EQ("stun", port()->Type()); + PrepareAddress(); + EXPECT_TRUE_WAIT(done(), kTimeoutMs); + EXPECT_EQ(2U, port()->Candidates().size()); +} diff --git a/talk/p2p/base/stunserver.cc b/talk/p2p/base/stunserver.cc index d9633f0f6c..df428f9d42 100644 --- a/talk/p2p/base/stunserver.cc +++ b/talk/p2p/base/stunserver.cc @@ -68,19 +68,7 @@ void StunServer::OnPacket( void StunServer::OnBindingRequest( StunMessage* msg, const rtc::SocketAddress& remote_addr) { StunMessage response; - response.SetType(STUN_BINDING_RESPONSE); - response.SetTransactionID(msg->transaction_id()); - - // Tell the user the address that we received their request from. - StunAddressAttribute* mapped_addr; - if (!msg->IsLegacy()) { - mapped_addr = StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS); - } else { - mapped_addr = StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS); - } - mapped_addr->SetAddress(remote_addr); - response.AddAttribute(mapped_addr); - + GetStunBindReqponse(msg, remote_addr, &response); SendResponse(response, remote_addr); } @@ -108,4 +96,21 @@ void StunServer::SendResponse( LOG_ERR(LS_ERROR) << "sendto"; } +void StunServer::GetStunBindReqponse(StunMessage* request, + const rtc::SocketAddress& remote_addr, + StunMessage* response) const { + response->SetType(STUN_BINDING_RESPONSE); + response->SetTransactionID(request->transaction_id()); + + // Tell the user the address that we received their request from. + StunAddressAttribute* mapped_addr; + if (!request->IsLegacy()) { + mapped_addr = StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS); + } else { + mapped_addr = StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS); + } + mapped_addr->SetAddress(remote_addr); + response->AddAttribute(mapped_addr); +} + } // namespace cricket diff --git a/talk/p2p/base/stunserver.h b/talk/p2p/base/stunserver.h index 1e1440844c..a141e5b28b 100644 --- a/talk/p2p/base/stunserver.h +++ b/talk/p2p/base/stunserver.h @@ -51,7 +51,7 @@ class StunServer : public sigslot::has_slots<> { const rtc::PacketTime& packet_time); // Handlers for the different types of STUN/TURN requests: - void OnBindingRequest(StunMessage* msg, + virtual void OnBindingRequest(StunMessage* msg, const rtc::SocketAddress& addr); void OnAllocateRequest(StunMessage* msg, const rtc::SocketAddress& addr); @@ -69,6 +69,11 @@ class StunServer : public sigslot::has_slots<> { void SendResponse(const StunMessage& msg, const rtc::SocketAddress& addr); + // A helper method to compose a STUN binding response. + void GetStunBindReqponse(StunMessage* request, + const rtc::SocketAddress& remote_addr, + StunMessage* response) const; + private: rtc::scoped_ptr socket_; }; diff --git a/talk/p2p/base/teststunserver.h b/talk/p2p/base/teststunserver.h index 6f85009ab6..840150be8e 100644 --- a/talk/p2p/base/teststunserver.h +++ b/talk/p2p/base/teststunserver.h @@ -35,19 +35,39 @@ namespace cricket { // A test STUN server. Useful for unit tests. -class TestStunServer { +class TestStunServer : StunServer { public: - TestStunServer(rtc::Thread* thread, - const rtc::SocketAddress& addr) - : socket_(thread->socketserver()->CreateAsyncSocket(addr.family(), - SOCK_DGRAM)), - udp_socket_(rtc::AsyncUDPSocket::Create(socket_, addr)), - server_(udp_socket_) { + static TestStunServer* Create(rtc::Thread* thread, + const rtc::SocketAddress& addr) { + rtc::AsyncSocket* socket = + thread->socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM); + rtc::AsyncUDPSocket* udp_socket = + rtc::AsyncUDPSocket::Create(socket, addr); + + return new TestStunServer(udp_socket); } + + // Set a fake STUN address to return to the client. + void set_fake_stun_addr(const rtc::SocketAddress& addr) { + fake_stun_addr_ = addr; + } + private: - rtc::AsyncSocket* socket_; - rtc::AsyncUDPSocket* udp_socket_; - cricket::StunServer server_; + explicit TestStunServer(rtc::AsyncUDPSocket* socket) : StunServer(socket) {} + + void OnBindingRequest(StunMessage* msg, + const rtc::SocketAddress& remote_addr) OVERRIDE { + if (fake_stun_addr_.IsNil()) { + StunServer::OnBindingRequest(msg, remote_addr); + } else { + StunMessage response; + GetStunBindReqponse(msg, fake_stun_addr_, &response); + SendResponse(response, remote_addr); + } + } + + private: + rtc::SocketAddress fake_stun_addr_; }; } // namespace cricket diff --git a/talk/p2p/client/portallocator_unittest.cc b/talk/p2p/client/portallocator_unittest.cc index e793064e5b..b1feca2d8c 100644 --- a/talk/p2p/client/portallocator_unittest.cc +++ b/talk/p2p/client/portallocator_unittest.cc @@ -112,7 +112,8 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { ss_scope_(fss_.get()), nat_factory_(vss_.get(), kNatAddr), nat_socket_factory_(&nat_factory_), - stun_server_(Thread::Current(), kStunAddr), + stun_server_(cricket::TestStunServer::Create(Thread::Current(), + kStunAddr)), relay_server_(Thread::Current(), kRelayUdpIntAddr, kRelayUdpExtAddr, kRelayTcpIntAddr, kRelayTcpExtAddr, kRelaySslTcpIntAddr, kRelaySslTcpExtAddr), @@ -280,7 +281,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { rtc::scoped_ptr nat_server_; rtc::NATSocketFactory nat_factory_; rtc::BasicPacketSocketFactory nat_socket_factory_; - cricket::TestStunServer stun_server_; + rtc::scoped_ptr stun_server_; cricket::TestRelayServer relay_server_; cricket::TestTurnServer turn_server_; rtc::FakeNetworkManager network_manager_;