From 8110beda7f98623e4510f99ed51a05d126437642 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Thu, 13 Jul 2017 13:27:23 -0700 Subject: [PATCH] Add additional check when setting RTCConfiguration Check that ice_regather_interval_range is set only when continual regathering is also set. Bug: webrtc:7969 Change-Id: Ifcfeee744d817cf00914418d7e682f11528faf05 Reviewed-on: https://chromium-review.googlesource.com/569358 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#19009} --- webrtc/p2p/base/p2ptransportchannel.cc | 2 ++ .../p2p/base/p2ptransportchannel_unittest.cc | 2 -- webrtc/pc/peerconnection.cc | 31 +++++++++++++++++ webrtc/pc/peerconnection.h | 6 ++++ webrtc/pc/peerconnectioninterface_unittest.cc | 33 +++++++++++++++++++ 5 files changed, 72 insertions(+), 2 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 2c9ee13765..2cc2944e9f 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -433,6 +433,8 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { } if (config.regather_all_networks_interval_range) { + // Config validation is assumed to have already happened at the API layer. + RTC_DCHECK(config.continual_gathering_policy != GATHER_ONCE); config_.regather_all_networks_interval_range = config.regather_all_networks_interval_range; LOG(LS_INFO) << "Set regather_all_networks_interval_range to " diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index c37b818091..eaa41a5d2e 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1374,8 +1374,6 @@ TEST_F(P2PTransportChannelTest, TestIceRegatherOnAllNetworksContinual) { config1.regather_all_networks_interval_range.emplace( kRegatherInterval, kRegatherInterval); IceConfig config2; - config2.regather_all_networks_interval_range.emplace( - kRegatherInterval, kRegatherInterval); CreateChannels(config1, config2); EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && diff --git a/webrtc/pc/peerconnection.cc b/webrtc/pc/peerconnection.cc index 42664b04ef..1a5fd6c5a9 100644 --- a/webrtc/pc/peerconnection.cc +++ b/webrtc/pc/peerconnection.cc @@ -216,6 +216,13 @@ bool SafeSetError(webrtc::RTCErrorType type, webrtc::RTCError* error) { return type == webrtc::RTCErrorType::NONE; } +bool SafeSetError(webrtc::RTCError error, webrtc::RTCError* error_out) { + if (error_out) { + *error_out = std::move(error); + } + return error.ok(); +} + } // namespace namespace webrtc { @@ -444,6 +451,13 @@ bool PeerConnection::Initialize( std::unique_ptr cert_generator, PeerConnectionObserver* observer) { TRACE_EVENT0("webrtc", "PeerConnection::Initialize"); + + RTCError config_error = ValidateConfiguration(configuration); + if (!config_error.ok()) { + LOG(LS_ERROR) << "Invalid configuration: " << config_error.message(); + return false; + } + if (!allocator) { LOG(LS_ERROR) << "PeerConnection initialized without a PortAllocator? " << "This shouldn't happen if using PeerConnectionFactory."; @@ -517,6 +531,17 @@ bool PeerConnection::Initialize( return true; } +RTCError PeerConnection::ValidateConfiguration( + const RTCConfiguration& config) const { + if (config.ice_regather_interval_range && + config.continual_gathering_policy == GATHER_ONCE) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "ice_regather_interval_range specified but continual " + "gathering policy is GATHER_ONCE"); + } + return RTCError::OK(); +} + rtc::scoped_refptr PeerConnection::local_streams() { return local_streams_; @@ -1161,6 +1186,12 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); } + // Validate the modified configuration. + RTCError validate_error = ValidateConfiguration(modified_config); + if (!validate_error.ok()) { + return SafeSetError(std::move(validate_error), error); + } + // Note that this isn't possible through chromium, since it's an unsigned // short in WebIDL. if (configuration.ice_candidate_pool_size < 0 || diff --git a/webrtc/pc/peerconnection.h b/webrtc/pc/peerconnection.h index e563d51397..6fec348354 100644 --- a/webrtc/pc/peerconnection.h +++ b/webrtc/pc/peerconnection.h @@ -394,6 +394,12 @@ class PeerConnection : public PeerConnectionInterface, // This function should only be called from the worker thread. void StopRtcEventLog_w(); + // Ensures the configuration doesn't have any parameters with invalid values, + // or values that conflict with other parameters. + // + // Returns RTCError::OK() if there are no issues. + RTCError ValidateConfiguration(const RTCConfiguration& config) const; + // Storing the factory as a scoped reference pointer ensures that the memory // in the PeerConnectionFactoryImpl remains available as long as the // PeerConnection is running. It is passed to PeerConnection as a raw pointer. diff --git a/webrtc/pc/peerconnectioninterface_unittest.cc b/webrtc/pc/peerconnectioninterface_unittest.cc index c10324a94b..a6d80b76fc 100644 --- a/webrtc/pc/peerconnectioninterface_unittest.cc +++ b/webrtc/pc/peerconnectioninterface_unittest.cc @@ -797,6 +797,17 @@ class PeerConnectionInterfaceTest : public testing::Test { EXPECT_EQ(nullptr, pc); } + void CreatePeerConnectionExpectFail( + PeerConnectionInterface::RTCConfiguration config) { + PeerConnectionInterface::IceServer server; + server.uri = kTurnIceServerUri; + server.password = kTurnPassword; + config.servers.push_back(server); + rtc::scoped_refptr pc = + pc_factory_->CreatePeerConnection(config, nullptr, nullptr, &observer_); + EXPECT_EQ(nullptr, pc); + } + void CreatePeerConnectionWithDifferentConfigurations() { CreatePeerConnectionWithIceServer(kStunAddressOnly, ""); EXPECT_EQ(1u, port_allocator_->stun_servers().size()); @@ -3399,6 +3410,28 @@ TEST_F(PeerConnectionInterfaceTest, SetBitrateMaxNegativeFails) { EXPECT_FALSE(pc_->SetBitrate(bitrate).ok()); } +// ice_regather_interval_range requires WebRTC to be configured for continual +// gathering already. +TEST_F(PeerConnectionInterfaceTest, + SetIceRegatherIntervalRangeWithoutContinualGatheringFails) { + PeerConnectionInterface::RTCConfiguration config; + config.ice_regather_interval_range.emplace(1000, 2000); + config.continual_gathering_policy = + PeerConnectionInterface::ContinualGatheringPolicy::GATHER_ONCE; + CreatePeerConnectionExpectFail(config); +} + +// Ensures that there is no error when ice_regather_interval_range is set with +// continual gathering enabled. +TEST_F(PeerConnectionInterfaceTest, + SetIceRegatherIntervalRangeWithContinualGathering) { + PeerConnectionInterface::RTCConfiguration config; + config.ice_regather_interval_range.emplace(1000, 2000); + config.continual_gathering_policy = + PeerConnectionInterface::ContinualGatheringPolicy::GATHER_CONTINUALLY; + CreatePeerConnection(config, nullptr); +} + // The current bitrate from Call's BitrateConfigMask is currently clamped by // Call's BitrateConfig, which comes from the SDP or a default value. This test // checks that a call to SetBitrate with a current bitrate that will be clamped