From 7351f4689caae2e53b3ee54874546e8aab769d22 Mon Sep 17 00:00:00 2001 From: Peter Thatcher Date: Thu, 2 Apr 2015 16:39:16 -0700 Subject: [PATCH] Don't send STUN pings if we don't have a remote ufrag and pwd. BUG=4495 R=juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/44029004 Cr-Commit-Position: refs/heads/master@{#8926} --- webrtc/p2p/base/p2ptransportchannel.cc | 8 +++++ webrtc/p2p/base/p2ptransportchannel.h | 4 ++- .../p2p/base/p2ptransportchannel_unittest.cc | 29 ++++++++++++++++++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 735dbd598..031a64ce0 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -1163,6 +1163,14 @@ void P2PTransportChannel::OnPing() { // Is the connection in a state for us to even consider pinging the other side? bool P2PTransportChannel::IsPingable(Connection* conn) { + const Candidate& remote = conn->remote_candidate(); + // We should never get this far with an empty remote ufrag. + ASSERT(!remote.username().empty()); + if (remote.username().empty() || remote.password().empty()) { + // If we don't have an ICE ufrag and pwd, there's no way we can ping. + return false; + } + // An unconnected connection cannot be written to at all, so pinging is out // of the question. if (!conn->connected()) diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index cd852d06a..78c9528eb 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -154,6 +154,9 @@ class P2PTransportChannel : public TransportChannelImpl, // Helper method used only in unittest. rtc::DiffServCodePoint DefaultDscpValue() const; + // Public for unit tests. + Connection* FindNextPingableConnection(); + private: rtc::Thread* thread() { return worker_thread_; } PortAllocatorSession* allocator_session() { @@ -182,7 +185,6 @@ class P2PTransportChannel : public TransportChannelImpl, void RememberRemoteCandidate(const Candidate& remote_candidate, PortInterface* origin_port); bool IsPingable(Connection* conn); - Connection* FindNextPingableConnection(); void PingConnection(Connection* conn); void AddAllocatorSession(PortAllocatorSession* session); void AddConnection(Connection* connection); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index e5df2d7e8..d70ecd86d 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1267,7 +1267,11 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) { PORTALLOCATOR_ENABLE_SHARED_UFRAG, kDefaultStepDelay, kDefaultStepDelay, cricket::ICEPROTO_RFC5245); + // Emulate no remote credentials coming in. + set_clear_remote_candidates_ufrag_pwd(false); CreateChannels(1); + // Only have remote credentials come in for ep2, not ep1. + ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive // candidate. @@ -1279,11 +1283,20 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) { WAIT((best_connection = ep1_ch1()->best_connection()) != NULL, 2000); EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type()); + // Because we don't have a remote pwd, we don't ping yet. + EXPECT_EQ(kIceUfrag[1], + ep1_ch1()->best_connection()->remote_candidate().username()); + EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password()); + EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection()); + ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); ResumeCandidates(1); - WAIT(ep2_ch1()->best_connection() != NULL, 2000); + EXPECT_EQ(kIcePwd[1], + ep1_ch1()->best_connection()->remote_candidate().password()); + EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection()); + WAIT(ep2_ch1()->best_connection() != NULL, 2000); // Verify ep1's best connection is updated to use the 'local' candidate. EXPECT_EQ_WAIT( "local", @@ -1301,7 +1314,11 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) { PORTALLOCATOR_ENABLE_SHARED_UFRAG, kDefaultStepDelay, kDefaultStepDelay, cricket::ICEPROTO_RFC5245); + // Emulate no remote credentials coming in. + set_clear_remote_candidates_ufrag_pwd(false); CreateChannels(1); + // Only have remote credentials come in for ep2, not ep1. + ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]); // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive // candidate. PauseCandidates(1); @@ -1311,9 +1328,19 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) { WAIT(ep1_ch1()->best_connection() != NULL, 2000); EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type()); + // Because we don't have a remote pwd, we don't ping yet. + EXPECT_EQ(kIceUfrag[1], + ep1_ch1()->best_connection()->remote_candidate().username()); + EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password()); + EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection()); + ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); ResumeCandidates(1); + EXPECT_EQ(kIcePwd[1], + ep1_ch1()->best_connection()->remote_candidate().password()); + EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection()); + const cricket::Connection* best_connection = NULL; WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 2000);