From dea6889ef627507921f4e998fc584881e20b7200 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 27 Mar 2018 10:55:21 -0700 Subject: [PATCH] Add sanity checks of IceConfig parameters. IceConfig contains a set of parameters that affect the behavior of ICE. Inconsistent or conflicting parameters lead to erroneous or unpredicatble behavior in the network stack. Sanity checks are now added to validate IceConfig. TBR=magjed@webrtc.org Bug: webrtc:8993 Change-Id: I708bc3f1ef970872754a82a47a509bda15061ca6 Reviewed-on: https://webrtc-review.googlesource.com/60847 Commit-Queue: Qingsi Wang Reviewed-by: Taylor Brandstetter Reviewed-by: Peter Thatcher Cr-Commit-Position: refs/heads/master@{#22664} --- p2p/base/p2ptransportchannel.cc | 75 +++++++++++++++++-- p2p/base/p2ptransportchannel.h | 2 + p2p/base/p2ptransportchannel_unittest.cc | 8 +- p2p/base/port.cc | 3 + p2p/base/port.h | 4 +- pc/peerconnection.cc | 4 +- .../UnitTests/RTCPeerConnectionTest.mm | 4 +- 7 files changed, 84 insertions(+), 16 deletions(-) diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 5ef81f63e0..088c405538 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -91,10 +91,23 @@ int CompareCandidatePairsByNetworkPreference( return a_and_b_equal; } +uint32_t GetWeakPingIntervalInFieldTrial() { + uint32_t weak_ping_interval = ::strtoul( + webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), + nullptr, 10); + if (weak_ping_interval) { + return static_cast(weak_ping_interval); + } + return cricket::WEAK_PING_INTERVAL; +} + } // unnamed namespace namespace cricket { +using webrtc::RTCErrorType; +using webrtc::RTCError; + bool IceCredentialsChanged(const std::string& old_ufrag, const std::string& old_pwd, const std::string& new_ufrag, @@ -130,12 +143,10 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, true /* presume_writable_when_fully_relayed */, REGATHER_ON_FAILED_NETWORKS_INTERVAL, RECEIVING_SWITCHING_DELAY) { - uint32_t weak_ping_interval = ::strtoul( - webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), - nullptr, 10); - if (weak_ping_interval) { - weak_ping_interval_ = static_cast(weak_ping_interval); - } + weak_ping_interval_ = GetWeakPingIntervalInFieldTrial(); + // Validate IceConfig even for mostly built-in constant default values in case + // we change them. + RTC_DCHECK(ValidateIceConfig(config_).ok()); ice_event_log_.set_event_log(event_log); } @@ -562,12 +573,64 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { RTC_LOG(LS_INFO) << "Set STUN keepalive interval to " << config.stun_keepalive_interval_or_default(); } + + RTC_DCHECK(ValidateIceConfig(config_).ok()); } const IceConfig& P2PTransportChannel::config() const { return config_; } +RTCError P2PTransportChannel::ValidateIceConfig(const IceConfig& config) { + if (config.regather_all_networks_interval_range && + config.continual_gathering_policy == GATHER_ONCE) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "regather_all_networks_interval_range specified but " + "continual gathering policy is GATHER_ONCE"); + } + + if (config.ice_check_interval_strong_connectivity_or_default() < + config.ice_check_interval_weak_connectivity.value_or( + GetWeakPingIntervalInFieldTrial())) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of candidate pairs is shorter when ICE is " + "strongly connected than that when ICE is weakly " + "connected"); + } + + if (config.receiving_timeout_or_default() < + std::max(config.ice_check_interval_strong_connectivity_or_default(), + config.ice_check_min_interval_or_default())) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "Receiving timeout is shorter than the minimal ping interval."); + } + + if (config.backup_connection_ping_interval_or_default() < + config.ice_check_interval_strong_connectivity_or_default()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of backup candidate pairs is shorter than " + "that of general candidate pairs when ICE is strongly " + "connected"); + } + + if (config.stable_writable_connection_ping_interval_or_default() < + config.ice_check_interval_strong_connectivity_or_default()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of stable and writable candidate pairs is " + "shorter than that of general candidate pairs when ICE is " + "strongly connected"); + } + + if (config.ice_unwritable_timeout_or_default() > CONNECTION_WRITE_TIMEOUT) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The timeout period for the writability state to become " + "UNRELIABLE is longer than that to become TIMEOUT."); + } + + return RTCError::OK(); +} + int P2PTransportChannel::check_receiving_interval() const { return std::max(MIN_CHECK_RECEIVING_INTERVAL, config_.receiving_timeout_or_default() / 10); diff --git a/p2p/base/p2ptransportchannel.h b/p2p/base/p2ptransportchannel.h index d811b631bd..cb7bbce72b 100644 --- a/p2p/base/p2ptransportchannel.h +++ b/p2p/base/p2ptransportchannel.h @@ -28,6 +28,7 @@ #include #include "api/candidate.h" +#include "api/rtcerror.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h" #include "logging/rtc_event_log/icelogger.h" #include "p2p/base/candidatepairinterface.h" @@ -106,6 +107,7 @@ class P2PTransportChannel : public IceTransportInternal, // TODO(deadbeef): Use rtc::Optional instead of negative values. void SetIceConfig(const IceConfig& config) override; const IceConfig& config() const; + static webrtc::RTCError ValidateIceConfig(const IceConfig& config); void SetMetricsObserver(webrtc::MetricsObserverInterface* observer) override; // From TransportChannel: diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index a650b22c19..45662ae8e7 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -4423,7 +4423,7 @@ class P2PTransportChannelMostLikelyToWorkFirstTest // we have a selected connection. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestRelayRelayFirstWhenNothingPingedYet) { - const int max_strong_interval = 100; + const int max_strong_interval = 500; P2PTransportChannel& ch = StartTransportChannel(true, max_strong_interval); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); @@ -4480,7 +4480,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, // in the first round. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestRelayRelayFirstWhenEverythingPinged) { - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); @@ -4511,7 +4511,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, // before we re-ping Relay/Relay connections again. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestNoStarvationOnNonRelayConnection) { - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); @@ -4550,7 +4550,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) { config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP)); allocator()->AddTurnServer(config); - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 5f6edb13cc..cfecf7f4f2 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -147,6 +147,9 @@ const int64_t kForgetPacketAfter = 30000; // 30 seconds namespace cricket { +using webrtc::RTCErrorType; +using webrtc::RTCError; + // TODO(ronghuawu): Use "local", "srflx", "prflx" and "relay". But this requires // the signaling part be updated correspondingly as well. const char LOCAL_PORT_TYPE[] = "local"; diff --git a/p2p/base/port.h b/p2p/base/port.h index 5655b968cc..434aaef204 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -19,6 +19,7 @@ #include "api/candidate.h" #include "api/optional.h" +#include "api/rtcerror.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h" #include "logging/rtc_event_log/icelogger.h" @@ -552,9 +553,6 @@ class Connection : public CandidatePairInterface, int unwritable_timeout() const; void set_unwritable_timeout(const rtc::Optional& value_ms) { - // TODO(qingsi): Validate configuration parameters in - // PeerConnection::ValidateConfiguration. - RTC_CHECK_LT(value_ms.value_or(-1), CONNECTION_WRITE_TIMEOUT); unwritable_timeout_ = value_ms; } int unwritable_min_checks() const; diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 84376ce58a..538f5c1f78 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1028,7 +1028,9 @@ RTCError PeerConnection::ValidateConfiguration( "ice_regather_interval_range specified but continual " "gathering policy is GATHER_ONCE"); } - return RTCError::OK(); + auto result = + cricket::P2PTransportChannel::ValidateIceConfig(ParseIceConfig(config)); + return result; } rtc::scoped_refptr diff --git a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm index 65bedde704..686bf2d37d 100644 --- a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm +++ b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm @@ -40,8 +40,8 @@ config.tcpCandidatePolicy = RTCTcpCandidatePolicyDisabled; config.candidateNetworkPolicy = RTCCandidateNetworkPolicyLowCost; const int maxPackets = 60; - const int timeout = 1; - const int interval = 2; + const int timeout = 1500; + const int interval = 2000; config.audioJitterBufferMaxPackets = maxPackets; config.audioJitterBufferFastAccelerate = YES; config.iceConnectionReceivingTimeout = timeout;