From 0a6c4ca942f3a25c15c7af64a9515d381c34cd9c Mon Sep 17 00:00:00 2001 From: deadbeef Date: Tue, 6 Oct 2015 11:38:28 -0700 Subject: [PATCH] Catching more errors when parsing ICE server URLs. Every malformed URL should now produce an error message in JS, rather than silently failing and possibly printing a warning message to the console (and possibly crashing). Also added some unit tests, and made "ParseIceServers" public. BUG=445002 Review URL: https://codereview.webrtc.org/1344143002 Cr-Commit-Position: refs/heads/master@{#10186} --- talk/app/webrtc/peerconnection.cc | 161 +++++++++------- talk/app/webrtc/peerconnection.h | 6 + talk/app/webrtc/peerconnection_unittest.cc | 177 ++++++++++++++++++ .../webrtc/peerconnectionfactory_unittest.cc | 5 - .../peerconnectioninterface_unittest.cc | 37 ++-- webrtc/base/stringencode.cc | 16 +- webrtc/base/stringencode.h | 5 + webrtc/base/stringencode_unittest.cc | 17 ++ 8 files changed, 334 insertions(+), 90 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 36875a6f93..bf9a80d9b7 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -28,6 +28,7 @@ #include "talk/app/webrtc/peerconnection.h" #include +#include // for isdigit #include "talk/app/webrtc/dtmfsender.h" #include "talk/app/webrtc/jsepicecandidate.h" @@ -45,6 +46,12 @@ namespace { using webrtc::PeerConnectionInterface; +using webrtc::StunConfigurations; +using webrtc::TurnConfigurations; +typedef webrtc::PortAllocatorFactoryInterface::StunConfiguration + StunConfiguration; +typedef webrtc::PortAllocatorFactoryInterface::TurnConfiguration + TurnConfiguration; // The min number of tokens must present in Turn host uri. // e.g. user@turn.example.org @@ -59,16 +66,20 @@ static const char kUdpTransportType[] = "udp"; static const char kTcpTransportType[] = "tcp"; // NOTE: Must be in the same order as the ServiceType enum. -static const char* kValidIceServiceTypes[] = { - "stun", "stuns", "turn", "turns", "invalid" }; +static const char* kValidIceServiceTypes[] = {"stun", "stuns", "turn", "turns"}; +// NOTE: A loop below assumes that the first value of this enum is 0 and all +// other values are incremental. enum ServiceType { - STUN, // Indicates a STUN server. - STUNS, // Indicates a STUN server used with a TLS session. - TURN, // Indicates a TURN server - TURNS, // Indicates a TURN server used with a TLS session. - INVALID, // Unknown. + STUN = 0, // Indicates a STUN server. + STUNS, // Indicates a STUN server used with a TLS session. + TURN, // Indicates a TURN server + TURNS, // Indicates a TURN server used with a TLS session. + INVALID, // Unknown. }; +static_assert(INVALID == ARRAY_SIZE(kValidIceServiceTypes), + "kValidIceServiceTypes must have as many strings as ServiceType " + "has values."); enum { MSG_SET_SESSIONDESCRIPTION_SUCCESS = 0, @@ -100,7 +111,7 @@ struct GetStatsMsg : public rtc::MessageData { // scheme = "stun" / "stuns" // stun-host = IP-literal / IPv4address / reg-name // stun-port = *DIGIT - +// // draft-petithuguenin-behave-turn-uris-01 // turnURI = scheme ":" turn-host [ ":" turn-port ] // turn-host = username@IP-literal / IPv4address / reg-name @@ -108,12 +119,17 @@ bool GetServiceTypeAndHostnameFromUri(const std::string& in_str, ServiceType* service_type, std::string* hostname) { const std::string::size_type colonpos = in_str.find(':'); - if (colonpos == std::string::npos || (colonpos + 1) == in_str.length()) { + if (colonpos == std::string::npos) { + LOG(LS_WARNING) << "Missing ':' in ICE URI: " << in_str; return false; } - std::string type = in_str.substr(0, colonpos); + if ((colonpos + 1) == in_str.length()) { + LOG(LS_WARNING) << "Empty hostname in ICE URI: " << in_str; + return false; + } + *service_type = INVALID; for (size_t i = 0; i < ARRAY_SIZE(kValidIceServiceTypes); ++i) { - if (type.compare(kValidIceServiceTypes[i]) == 0) { + if (in_str.compare(0, colonpos, kValidIceServiceTypes[i]) == 0) { *service_type = static_cast(i); break; } @@ -125,52 +141,59 @@ bool GetServiceTypeAndHostnameFromUri(const std::string& in_str, return true; } +bool ParsePort(const std::string& in_str, int* port) { + // Make sure port only contains digits. FromString doesn't check this. + for (const char& c : in_str) { + if (!std::isdigit(c)) { + return false; + } + } + return rtc::FromString(in_str, port); +} + // This method parses IPv6 and IPv4 literal strings, along with hostnames in // standard hostname:port format. // Consider following formats as correct. // |hostname:port|, |[IPV6 address]:port|, |IPv4 address|:port, -// |hostname|, |[IPv6 address]|, |IPv4 address| +// |hostname|, |[IPv6 address]|, |IPv4 address|. bool ParseHostnameAndPortFromString(const std::string& in_str, std::string* host, int* port) { + RTC_DCHECK(host->empty()); if (in_str.at(0) == '[') { std::string::size_type closebracket = in_str.rfind(']'); if (closebracket != std::string::npos) { - *host = in_str.substr(1, closebracket - 1); std::string::size_type colonpos = in_str.find(':', closebracket); if (std::string::npos != colonpos) { - if (!rtc::FromString( - in_str.substr(closebracket + 2, std::string::npos), port)) { + if (!ParsePort(in_str.substr(closebracket + 2, std::string::npos), + port)) { return false; } } + *host = in_str.substr(1, closebracket - 1); } else { return false; } } else { std::string::size_type colonpos = in_str.find(':'); if (std::string::npos != colonpos) { - *host = in_str.substr(0, colonpos); - if (!rtc::FromString( - in_str.substr(colonpos + 1, std::string::npos), port)) { + if (!ParsePort(in_str.substr(colonpos + 1, std::string::npos), port)) { return false; } + *host = in_str.substr(0, colonpos); } else { *host = in_str; } } - return true; + return !host->empty(); } -typedef webrtc::PortAllocatorFactoryInterface::StunConfiguration - StunConfiguration; -typedef webrtc::PortAllocatorFactoryInterface::TurnConfiguration - TurnConfiguration; - +// Adds a StunConfiguration or TurnConfiguration to the appropriate list, +// by parsing |url| and using the username/password in |server|. bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, const std::string& url, - std::vector* stun_config, - std::vector* turn_config) { + StunConfigurations* stun_config, + TurnConfigurations* turn_config) { // draft-nandakumar-rtcweb-stun-uri-01 // stunURI = scheme ":" stun-host [ ":" stun-port ] // scheme = "stun" / "stuns" @@ -185,9 +208,11 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, // transport-ext = 1*unreserved // turn-host = IP-literal / IPv4address / reg-name // turn-port = *DIGIT + RTC_DCHECK(stun_config != nullptr); + RTC_DCHECK(turn_config != nullptr); std::vector tokens; std::string turn_transport_type = kUdpTransportType; - ASSERT(!url.empty()); + RTC_DCHECK(!url.empty()); rtc::tokenize(url, '?', &tokens); std::string uri_without_transport = tokens[0]; // Let's look into transport= param, if it exists. @@ -199,32 +224,38 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, // letters. if (tokens[1] != kUdpTransportType && tokens[1] != kTcpTransportType) { LOG(LS_WARNING) << "Transport param should always be udp or tcp."; - return true; + return false; } turn_transport_type = tokens[1]; } } std::string hoststring; - ServiceType service_type = INVALID; + ServiceType service_type; if (!GetServiceTypeAndHostnameFromUri(uri_without_transport, &service_type, &hoststring)) { - LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " - << uri_without_transport; - return true; + LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url; + return false; } - ASSERT(!hoststring.empty()); + // GetServiceTypeAndHostnameFromUri should never give an empty hoststring + RTC_DCHECK(!hoststring.empty()); // Let's break hostname. tokens.clear(); - rtc::tokenize(hoststring, '@', &tokens); - ASSERT(!tokens.empty()); + rtc::tokenize_with_empty_tokens(hoststring, '@', &tokens); + std::string username(server.username); - // TODO(pthatcher): What's the right thing to do if tokens.size() is >2? - // E.g. a string like "foo@bar@bat". - if (tokens.size() >= kTurnHostTokensNum) { + if (tokens.size() > kTurnHostTokensNum) { + LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring; + return false; + } + if (tokens.size() == kTurnHostTokensNum) { + if (tokens[0].empty() || tokens[1].empty()) { + LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring; + return false; + } username.assign(rtc::s_url_decode(tokens[0])); hoststring = tokens[1]; } else { @@ -239,13 +270,13 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, std::string address; if (!ParseHostnameAndPortFromString(hoststring, &address, &port)) { - LOG(WARNING) << "Invalid Hostname format: " << uri_without_transport; - return true; + LOG(WARNING) << "Invalid hostname format: " << uri_without_transport; + return false; } if (port <= 0 || port > 0xffff) { LOG(WARNING) << "Invalid port: " << port; - return true; + return false; } switch (service_type) { @@ -255,18 +286,7 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, break; case TURN: case TURNS: { - if (username.empty()) { - // Turn url example from the spec |url:"turn:user@turn.example.org"|. - std::vector turn_tokens; - rtc::tokenize(address, '@', &turn_tokens); - if (turn_tokens.size() == kTurnHostTokensNum) { - username.assign(rtc::s_url_decode(turn_tokens[0])); - address = turn_tokens[1]; - } - } - bool secure = (service_type == TURNS); - turn_config->push_back(TurnConfiguration(address, port, username, server.password, @@ -282,15 +302,19 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server, return true; } +} // namespace + +namespace webrtc { + bool ParseIceServers(const PeerConnectionInterface::IceServers& servers, - std::vector* stun_config, - std::vector* turn_config) { + StunConfigurations* stun_config, + TurnConfigurations* turn_config) { for (const webrtc::PeerConnectionInterface::IceServer& server : servers) { if (!server.urls.empty()) { for (const std::string& url : server.urls) { if (url.empty()) { - LOG(WARNING) << "Empty uri."; - continue; + LOG(LS_ERROR) << "Empty uri."; + return false; } if (!ParseIceServerUrl(server, url, stun_config, turn_config)) { return false; @@ -302,7 +326,8 @@ bool ParseIceServers(const PeerConnectionInterface::IceServers& servers, return false; } } else { - LOG(WARNING) << "Empty uri."; + LOG(LS_ERROR) << "Empty uri."; + return false; } } return true; @@ -324,10 +349,6 @@ bool CanAddLocalMediaStream(webrtc::StreamCollectionInterface* current_streams, return true; } -} // namespace - -namespace webrtc { - PeerConnection::PeerConnection(PeerConnectionFactory* factory) : factory_(factory), observer_(NULL), @@ -339,7 +360,7 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory) } PeerConnection::~PeerConnection() { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); if (mediastream_signaling_) { mediastream_signaling_->TearDown(); } @@ -359,7 +380,7 @@ bool PeerConnection::Initialize( PortAllocatorFactoryInterface* allocator_factory, rtc::scoped_ptr dtls_identity_store, PeerConnectionObserver* observer) { - ASSERT(observer != NULL); + RTC_DCHECK(observer != NULL); if (!observer) return false; observer_ = observer; @@ -498,7 +519,7 @@ PeerConnection::GetReceivers() const { bool PeerConnection::GetStats(StatsObserver* observer, MediaStreamTrackInterface* track, StatsOutputLevel level) { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); if (!VERIFY(observer != NULL)) { LOG(LS_ERROR) << "GetStats - observer is NULL."; return false; @@ -820,7 +841,7 @@ void PeerConnection::OnMessage(rtc::Message* msg) { break; } default: - ASSERT(false && "Not implemented"); + RTC_DCHECK(false && "Not implemented"); break; } } @@ -927,7 +948,7 @@ void PeerConnection::OnRemoveLocalStream(MediaStreamInterface* stream) { void PeerConnection::OnIceConnectionChange( PeerConnectionInterface::IceConnectionState new_state) { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); // After transitioning to "closed", ignore any additional states from // WebRtcSession (such as "disconnected"). if (ice_connection_state_ == kIceConnectionClosed) { @@ -939,7 +960,7 @@ void PeerConnection::OnIceConnectionChange( void PeerConnection::OnIceGatheringChange( PeerConnectionInterface::IceGatheringState new_state) { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); if (IsClosed()) { return; } @@ -948,17 +969,17 @@ void PeerConnection::OnIceGatheringChange( } void PeerConnection::OnIceCandidate(const IceCandidateInterface* candidate) { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); observer_->OnIceCandidate(candidate); } void PeerConnection::OnIceComplete() { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); observer_->OnIceComplete(); } void PeerConnection::OnIceConnectionReceivingChange(bool receiving) { - ASSERT(signaling_thread()->IsCurrent()); + RTC_DCHECK(signaling_thread()->IsCurrent()); observer_->OnIceConnectionReceivingChange(receiving); } diff --git a/talk/app/webrtc/peerconnection.h b/talk/app/webrtc/peerconnection.h index e996132df0..8a8701931a 100644 --- a/talk/app/webrtc/peerconnection.h +++ b/talk/app/webrtc/peerconnection.h @@ -48,6 +48,12 @@ typedef std::vector typedef std::vector TurnConfigurations; +// Parses the URLs for each server in |servers| to build |stun_config| and +// |turn_config|. +bool ParseIceServers(const PeerConnectionInterface::IceServers& servers, + StunConfigurations* stun_config, + TurnConfigurations* turn_config); + // PeerConnection implements the PeerConnectionInterface interface. // It uses MediaStreamSignaling and WebRtcSession to implement // the PeerConnection functionality. diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc index d593f888ff..c8b0c8cf6a 100644 --- a/talk/app/webrtc/peerconnection_unittest.cc +++ b/talk/app/webrtc/peerconnection_unittest.cc @@ -37,6 +37,7 @@ #include "talk/app/webrtc/fakeportallocatorfactory.h" #include "talk/app/webrtc/localaudiosource.h" #include "talk/app/webrtc/mediastreaminterface.h" +#include "talk/app/webrtc/peerconnection.h" #include "talk/app/webrtc/peerconnectionfactory.h" #include "talk/app/webrtc/peerconnectioninterface.h" #include "talk/app/webrtc/test/fakeaudiocapturemodule.h" @@ -1630,4 +1631,180 @@ TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTest(); } +class IceServerParsingTest : public testing::Test { + public: + // Convenience for parsing a single URL. + bool ParseUrl(const std::string& url) { + return ParseUrl(url, std::string(), std::string()); + } + + bool ParseUrl(const std::string& url, + const std::string& username, + const std::string& password) { + PeerConnectionInterface::IceServers servers; + PeerConnectionInterface::IceServer server; + server.urls.push_back(url); + server.username = username; + server.password = password; + servers.push_back(server); + return webrtc::ParseIceServers(servers, &stun_configurations_, + &turn_configurations_); + } + + protected: + webrtc::StunConfigurations stun_configurations_; + webrtc::TurnConfigurations turn_configurations_; +}; + +// Make sure all STUN/TURN prefixes are parsed correctly. +TEST_F(IceServerParsingTest, ParseStunPrefixes) { + EXPECT_TRUE(ParseUrl("stun:hostname")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ(0U, turn_configurations_.size()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stuns:hostname")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ(0U, turn_configurations_.size()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("turn:hostname")); + EXPECT_EQ(0U, stun_configurations_.size()); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_FALSE(turn_configurations_[0].secure); + turn_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("turns:hostname")); + EXPECT_EQ(0U, stun_configurations_.size()); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_TRUE(turn_configurations_[0].secure); + turn_configurations_.clear(); + + // invalid prefixes + EXPECT_FALSE(ParseUrl("stunn:hostname")); + EXPECT_FALSE(ParseUrl(":hostname")); + EXPECT_FALSE(ParseUrl(":")); + EXPECT_FALSE(ParseUrl("")); +} + +TEST_F(IceServerParsingTest, VerifyDefaults) { + // TURNS defaults + EXPECT_TRUE(ParseUrl("turns:hostname")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ(5349, turn_configurations_[0].server.port()); + EXPECT_EQ("tcp", turn_configurations_[0].transport_type); + turn_configurations_.clear(); + + // TURN defaults + EXPECT_TRUE(ParseUrl("turn:hostname")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ(3478, turn_configurations_[0].server.port()); + EXPECT_EQ("udp", turn_configurations_[0].transport_type); + turn_configurations_.clear(); + + // STUN defaults + EXPECT_TRUE(ParseUrl("stun:hostname")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ(3478, stun_configurations_[0].server.port()); + stun_configurations_.clear(); +} + +// Check that the 6 combinations of IPv4/IPv6/hostname and with/without port +// can be parsed correctly. +TEST_F(IceServerParsingTest, ParseHostnameAndPort) { + EXPECT_TRUE(ParseUrl("stun:1.2.3.4:1234")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("1.2.3.4", stun_configurations_[0].server.hostname()); + EXPECT_EQ(1234, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stun:[1:2:3:4:5:6:7:8]:4321")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("1:2:3:4:5:6:7:8", stun_configurations_[0].server.hostname()); + EXPECT_EQ(4321, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stun:hostname:9999")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("hostname", stun_configurations_[0].server.hostname()); + EXPECT_EQ(9999, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stun:1.2.3.4")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("1.2.3.4", stun_configurations_[0].server.hostname()); + EXPECT_EQ(3478, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stun:[1:2:3:4:5:6:7:8]")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("1:2:3:4:5:6:7:8", stun_configurations_[0].server.hostname()); + EXPECT_EQ(3478, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("stun:hostname")); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ("hostname", stun_configurations_[0].server.hostname()); + EXPECT_EQ(3478, stun_configurations_[0].server.port()); + stun_configurations_.clear(); + + // Try some invalid hostname:port strings. + EXPECT_FALSE(ParseUrl("stun:hostname:99a99")); + EXPECT_FALSE(ParseUrl("stun:hostname:-1")); + EXPECT_FALSE(ParseUrl("stun:hostname:")); + EXPECT_FALSE(ParseUrl("stun:[1:2:3:4:5:6:7:8]junk:1000")); + EXPECT_FALSE(ParseUrl("stun::5555")); + EXPECT_FALSE(ParseUrl("stun:")); +} + +// Test parsing the "?transport=xxx" part of the URL. +TEST_F(IceServerParsingTest, ParseTransport) { + EXPECT_TRUE(ParseUrl("turn:hostname:1234?transport=tcp")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ("tcp", turn_configurations_[0].transport_type); + turn_configurations_.clear(); + + EXPECT_TRUE(ParseUrl("turn:hostname?transport=udp")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ("udp", turn_configurations_[0].transport_type); + turn_configurations_.clear(); + + EXPECT_FALSE(ParseUrl("turn:hostname?transport=invalid")); +} + +// Test parsing ICE username contained in URL. +TEST_F(IceServerParsingTest, ParseUsername) { + EXPECT_TRUE(ParseUrl("turn:user@hostname")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ("user", turn_configurations_[0].username); + turn_configurations_.clear(); + + EXPECT_FALSE(ParseUrl("turn:@hostname")); + EXPECT_FALSE(ParseUrl("turn:username@")); + EXPECT_FALSE(ParseUrl("turn:@")); + EXPECT_FALSE(ParseUrl("turn:user@name@hostname")); +} + +// Test that username and password from IceServer is copied into the resulting +// TurnConfiguration. +TEST_F(IceServerParsingTest, CopyUsernameAndPasswordFromIceServer) { + EXPECT_TRUE(ParseUrl("turn:hostname", "username", "password")); + EXPECT_EQ(1U, turn_configurations_.size()); + EXPECT_EQ("username", turn_configurations_[0].username); + EXPECT_EQ("password", turn_configurations_[0].password); +} + +// Ensure that if a server has multiple URLs, each one is parsed. +TEST_F(IceServerParsingTest, ParseMultipleUrls) { + PeerConnectionInterface::IceServers servers; + PeerConnectionInterface::IceServer server; + server.urls.push_back("stun:hostname"); + server.urls.push_back("turn:hostname"); + servers.push_back(server); + EXPECT_TRUE(webrtc::ParseIceServers(servers, &stun_configurations_, + &turn_configurations_)); + EXPECT_EQ(1U, stun_configurations_.size()); + EXPECT_EQ(1U, turn_configurations_.size()); +} + #endif // if !defined(THREAD_SANITIZER) diff --git a/talk/app/webrtc/peerconnectionfactory_unittest.cc b/talk/app/webrtc/peerconnectionfactory_unittest.cc index 8cf08f549b..f1d5353abd 100644 --- a/talk/app/webrtc/peerconnectionfactory_unittest.cc +++ b/talk/app/webrtc/peerconnectionfactory_unittest.cc @@ -79,8 +79,6 @@ static const char kStunIceServerWithIPv4AddressWithoutPort[] = "stun:1.2.3.4"; static const char kStunIceServerWithIPv6Address[] = "stun:[2401:fa00:4::]:1234"; static const char kStunIceServerWithIPv6AddressWithoutPort[] = "stun:[2401:fa00:4::]"; -static const char kStunIceServerWithInvalidIPv6Address[] = - "stun:[2401:fa00:4:::3478"; static const char kTurnIceServerWithIPv6Address[] = "turn:test@[2401:fa00:4::]:1234"; @@ -208,7 +206,6 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServers) { TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServersUrls) { PeerConnectionInterface::RTCConfiguration config; webrtc::PeerConnectionInterface::IceServer ice_server; - ice_server.urls.push_back(""); // Empty URLs should be ignored. ice_server.urls.push_back(kStunIceServer); ice_server.urls.push_back(kTurnIceServer); ice_server.urls.push_back(kTurnIceServerWithTransport); @@ -368,8 +365,6 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIPLiteralAddress) { config.servers.push_back(ice_server); ice_server.uri = kStunIceServerWithIPv6AddressWithoutPort; config.servers.push_back(ice_server); - ice_server.uri = kStunIceServerWithInvalidIPv6Address; - config.servers.push_back(ice_server); ice_server.uri = kTurnIceServerWithIPv6Address; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc index 03ceaf2e10..5e135df647 100644 --- a/talk/app/webrtc/peerconnectioninterface_unittest.cc +++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc @@ -247,9 +247,11 @@ class PeerConnectionInterfaceTest : public testing::Test { webrtc::MediaConstraintsInterface* constraints) { PeerConnectionInterface::IceServer server; PeerConnectionInterface::IceServers servers; - server.uri = uri; - server.password = password; - servers.push_back(server); + if (!uri.empty()) { + server.uri = uri; + server.password = password; + servers.push_back(server); + } port_allocator_factory_ = FakePortAllocatorFactory::Create(); @@ -281,6 +283,21 @@ class PeerConnectionInterfaceTest : public testing::Test { EXPECT_EQ(PeerConnectionInterface::kStable, observer_.state_); } + void CreatePeerConnectionExpectFail(const std::string& uri) { + PeerConnectionInterface::IceServer server; + PeerConnectionInterface::IceServers servers; + server.uri = uri; + servers.push_back(server); + + scoped_ptr dtls_identity_store; + port_allocator_factory_ = FakePortAllocatorFactory::Create(); + scoped_refptr pc; + pc = pc_factory_->CreatePeerConnection( + servers, nullptr, port_allocator_factory_.get(), + dtls_identity_store.Pass(), &observer_); + ASSERT_EQ(nullptr, pc); + } + void CreatePeerConnectionWithDifferentConfigurations() { CreatePeerConnection(kStunAddressOnly, "", NULL); EXPECT_EQ(1u, port_allocator_factory_->stun_configs().size()); @@ -290,17 +307,9 @@ class PeerConnectionInterfaceTest : public testing::Test { EXPECT_EQ(kDefaultStunPort, port_allocator_factory_->stun_configs()[0].server.port()); - CreatePeerConnection(kStunInvalidPort, "", NULL); - EXPECT_EQ(0u, port_allocator_factory_->stun_configs().size()); - EXPECT_EQ(0u, port_allocator_factory_->turn_configs().size()); - - CreatePeerConnection(kStunAddressPortAndMore1, "", NULL); - EXPECT_EQ(0u, port_allocator_factory_->stun_configs().size()); - EXPECT_EQ(0u, port_allocator_factory_->turn_configs().size()); - - CreatePeerConnection(kStunAddressPortAndMore2, "", NULL); - EXPECT_EQ(0u, port_allocator_factory_->stun_configs().size()); - EXPECT_EQ(0u, port_allocator_factory_->turn_configs().size()); + CreatePeerConnectionExpectFail(kStunInvalidPort); + CreatePeerConnectionExpectFail(kStunAddressPortAndMore1); + CreatePeerConnectionExpectFail(kStunAddressPortAndMore2); CreatePeerConnection(kTurnIceServerUri, kTurnPassword, NULL); EXPECT_EQ(0u, port_allocator_factory_->stun_configs().size()); diff --git a/webrtc/base/stringencode.cc b/webrtc/base/stringencode.cc index 2930e5776c..01b41a633a 100644 --- a/webrtc/base/stringencode.cc +++ b/webrtc/base/stringencode.cc @@ -556,7 +556,6 @@ std::string s_transform(const std::string& source, Transform t) { size_t tokenize(const std::string& source, char delimiter, std::vector* fields) { - RTC_DCHECK(fields); fields->clear(); size_t last = 0; for (size_t i = 0; i < source.length(); ++i) { @@ -573,6 +572,21 @@ size_t tokenize(const std::string& source, char delimiter, return fields->size(); } +size_t tokenize_with_empty_tokens(const std::string& source, + char delimiter, + std::vector* fields) { + fields->clear(); + size_t last = 0; + for (size_t i = 0; i < source.length(); ++i) { + if (source[i] == delimiter) { + fields->push_back(source.substr(last, i - last)); + last = i + 1; + } + } + fields->push_back(source.substr(last, source.length() - last)); + return fields->size(); +} + size_t tokenize_append(const std::string& source, char delimiter, std::vector* fields) { if (!fields) return 0; diff --git a/webrtc/base/stringencode.h b/webrtc/base/stringencode.h index 0b9ed0e875..8f78ad1a64 100644 --- a/webrtc/base/stringencode.h +++ b/webrtc/base/stringencode.h @@ -146,6 +146,11 @@ size_t split(const std::string& source, char delimiter, size_t tokenize(const std::string& source, char delimiter, std::vector* fields); +// Tokenize, including the empty tokens. +size_t tokenize_with_empty_tokens(const std::string& source, + char delimiter, + std::vector* fields); + // Tokenize and append the tokens to fields. Return the new size of fields. size_t tokenize_append(const std::string& source, char delimiter, std::vector* fields); diff --git a/webrtc/base/stringencode_unittest.cc b/webrtc/base/stringencode_unittest.cc index 77fae35fb9..406d9c7d4a 100644 --- a/webrtc/base/stringencode_unittest.cc +++ b/webrtc/base/stringencode_unittest.cc @@ -298,6 +298,22 @@ TEST(TokenizeTest, TokenizeWithMarks) { ASSERT_STREQ("E F", fields.at(3).c_str()); } +TEST(TokenizeTest, TokenizeWithEmptyTokens) { + std::vector fields; + EXPECT_EQ(3ul, tokenize_with_empty_tokens("a.b.c", '.', &fields)); + EXPECT_EQ("a", fields[0]); + EXPECT_EQ("b", fields[1]); + EXPECT_EQ("c", fields[2]); + + EXPECT_EQ(3ul, tokenize_with_empty_tokens("..c", '.', &fields)); + EXPECT_TRUE(fields[0].empty()); + EXPECT_TRUE(fields[1].empty()); + EXPECT_EQ("c", fields[2]); + + EXPECT_EQ(1ul, tokenize_with_empty_tokens("", '.', &fields)); + EXPECT_TRUE(fields[0].empty()); +} + TEST(TokenizeFirstTest, NoLeadingSpaces) { std::string token; std::string rest; @@ -428,4 +444,5 @@ TEST(BoolTest, RoundTrip) { EXPECT_TRUE(FromString(ToString(false), &value)); EXPECT_FALSE(value); } + } // namespace rtc