diff --git a/api/peer_connection_interface.cc b/api/peer_connection_interface.cc index f82e84b80f..e1d94dd8c7 100644 --- a/api/peer_connection_interface.cc +++ b/api/peer_connection_interface.cc @@ -87,6 +87,13 @@ PeerConnectionFactoryInterface::CreatePeerConnection( return nullptr; } +RTCErrorOr> +PeerConnectionFactoryInterface::CreatePeerConnectionOrError( + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies) { + return RTCError(RTCErrorType::INTERNAL_ERROR); +} + RtpCapabilities PeerConnectionFactoryInterface::GetRtpSenderCapabilities( cricket::MediaType kind) const { return {}; diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 0e6cd5ce07..92d965b328 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1428,6 +1428,12 @@ class RTC_EXPORT PeerConnectionFactoryInterface // configuration and a PeerConnectionDependencies structure. // TODO(benwright): Make pure virtual once downstream mock PC factory classes // are updated. + virtual RTCErrorOr> + CreatePeerConnectionOrError( + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies); + // Deprecated creator - does not return an error code on error. + // TODO(bugs.webrtc.org:12238): Deprecate and remove. virtual rtc::scoped_refptr CreatePeerConnection( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies); diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc index 2400fd516f..47641375de 100644 --- a/pc/ice_server_parsing.cc +++ b/pc/ice_server_parsing.cc @@ -31,6 +31,15 @@ static const int kDefaultStunPort = 3478; static const int kDefaultStunTlsPort = 5349; static const char kTransport[] = "transport"; +// Allowed characters in hostname per RFC 3986 Appendix A "reg-name" +static const char kRegNameCharacters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789" + "-._~" // unreserved + "%" // pct-encoded + "!$&'()*+,;="; // sub-delims + // NOTE: Must be in the same order as the ServiceType enum. static const char* kValidIceServiceTypes[] = {"stun", "stuns", "turn", "turns"}; @@ -99,6 +108,7 @@ static bool ParseHostnameAndPortFromString(const std::string& in_str, int* port) { RTC_DCHECK(host->empty()); if (in_str.at(0) == '[') { + // IP_literal syntax std::string::size_type closebracket = in_str.rfind(']'); if (closebracket != std::string::npos) { std::string::size_type colonpos = in_str.find(':', closebracket); @@ -113,6 +123,7 @@ static bool ParseHostnameAndPortFromString(const std::string& in_str, return false; } } else { + // IPv4address or reg-name syntax std::string::size_type colonpos = in_str.find(':'); if (std::string::npos != colonpos) { if (!ParsePort(in_str.substr(colonpos + 1, std::string::npos), port)) { @@ -122,6 +133,10 @@ static bool ParseHostnameAndPortFromString(const std::string& in_str, } else { *host = in_str; } + // RFC 3986 section 3.2.2 and Appendix A - "reg-name" syntax + if (host->find_first_not_of(kRegNameCharacters) != std::string::npos) { + return false; + } } return !host->empty(); } diff --git a/pc/ice_server_parsing_unittest.cc b/pc/ice_server_parsing_unittest.cc index 2625b24590..e4dbd3a0f5 100644 --- a/pc/ice_server_parsing_unittest.cc +++ b/pc/ice_server_parsing_unittest.cc @@ -182,6 +182,11 @@ TEST_F(IceServerParsingTest, ParseHostnameAndPort) { EXPECT_FALSE(ParseUrl("stun:[1:2:3:4:5:6:7:8]junk:1000")); EXPECT_FALSE(ParseUrl("stun::5555")); EXPECT_FALSE(ParseUrl("stun:")); + // Test illegal URLs according to RFC 3986 (URI generic syntax) + // and RFC 7064 (URI schemes for STUN and TURN) + EXPECT_FALSE(ParseUrl("stun:/hostname")); // / is not allowed + EXPECT_FALSE(ParseUrl("stun:?hostname")); // ? is not allowed + EXPECT_FALSE(ParseUrl("stun:#hostname")); // # is not allowed } // Test parsing the "?transport=xxx" part of the URL. diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3760a01c9f..9ba7daefa1 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -387,7 +387,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator!=( return !(*this == o); } -rtc::scoped_refptr PeerConnection::Create( +RTCErrorOr> PeerConnection::Create( rtc::scoped_refptr context, const PeerConnectionFactoryInterface::Options& options, std::unique_ptr event_log, @@ -397,22 +397,26 @@ rtc::scoped_refptr PeerConnection::Create( RTCError config_error = cricket::P2PTransportChannel::ValidateIceConfig( ParseIceConfig(configuration)); if (!config_error.ok()) { - RTC_LOG(LS_ERROR) << "Invalid configuration: " << config_error.message(); - return nullptr; + RTC_LOG(LS_ERROR) << "Invalid ICE configuration: " + << config_error.message(); + return config_error; } if (!dependencies.allocator) { RTC_LOG(LS_ERROR) << "PeerConnection initialized without a PortAllocator? " "This shouldn't happen if using PeerConnectionFactory."; - return nullptr; + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "Attempt to create a PeerConnection without a PortAllocatorFactory"); } if (!dependencies.observer) { // TODO(deadbeef): Why do we do this? RTC_LOG(LS_ERROR) << "PeerConnection initialized without a " "PeerConnectionObserver"; - return nullptr; + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Attempt to create a PeerConnection without an observer"); } bool is_unified_plan = @@ -422,8 +426,10 @@ rtc::scoped_refptr PeerConnection::Create( new rtc::RefCountedObject( context, options, is_unified_plan, std::move(event_log), std::move(call), dependencies)); - if (!pc->Initialize(configuration, std::move(dependencies))) { - return nullptr; + RTCError init_error = pc->Initialize(configuration, std::move(dependencies)); + if (!init_error.ok()) { + RTC_LOG(LS_ERROR) << "PeerConnection initialization failed"; + return init_error; } return pc; } @@ -499,7 +505,7 @@ PeerConnection::~PeerConnection() { }); } -bool PeerConnection::Initialize( +RTCError PeerConnection::Initialize( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -511,7 +517,7 @@ bool PeerConnection::Initialize( RTCErrorType parse_error = ParseIceServers(configuration.servers, &stun_servers, &turn_servers); if (parse_error != RTCErrorType::NONE) { - return false; + return RTCError(parse_error, "ICE server parse failed"); } // Add the turn logging id to all turn servers @@ -660,7 +666,7 @@ bool PeerConnection::Initialize( }, delay_ms); - return true; + return RTCError::OK(); } rtc::scoped_refptr PeerConnection::local_streams() { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 52a302b2de..8768ebb133 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -124,7 +124,7 @@ class PeerConnection : public PeerConnectionInternal, // // Note that the function takes ownership of dependencies, and will // either use them or release them, whether it succeeds or fails. - static rtc::scoped_refptr Create( + static RTCErrorOr> Create( rtc::scoped_refptr context, const PeerConnectionFactoryInterface::Options& options, std::unique_ptr event_log, @@ -459,7 +459,7 @@ class PeerConnection : public PeerConnectionInternal, ~PeerConnection() override; private: - bool Initialize( + RTCError Initialize( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies); diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index da42e5a096..f4f72c75f8 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -206,6 +206,19 @@ rtc::scoped_refptr PeerConnectionFactory::CreatePeerConnection( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) { + auto result = + CreatePeerConnectionOrError(configuration, std::move(dependencies)); + if (result.ok()) { + return result.MoveValue(); + } else { + return nullptr; + } +} + +RTCErrorOr> +PeerConnectionFactory::CreatePeerConnectionOrError( + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies) { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(!(dependencies.allocator && dependencies.packet_socket_factory)) << "You can't set both allocator and packet_socket_factory; " @@ -250,13 +263,15 @@ PeerConnectionFactory::CreatePeerConnection( RTC_FROM_HERE, rtc::Bind(&PeerConnectionFactory::CreateCall_w, this, event_log.get())); - rtc::scoped_refptr pc = PeerConnection::Create( - context_, options_, std::move(event_log), std::move(call), configuration, - std::move(dependencies)); - if (!pc) { - return nullptr; + auto result = PeerConnection::Create(context_, options_, std::move(event_log), + std::move(call), configuration, + std::move(dependencies)); + if (!result.ok()) { + return result.MoveError(); } - return PeerConnectionProxy::Create(signaling_thread(), pc); + rtc::scoped_refptr result_proxy = + PeerConnectionProxy::Create(signaling_thread(), result.MoveValue()); + return result_proxy; } rtc::scoped_refptr diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index 427207f9cc..9c4a2b0526 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -25,6 +25,7 @@ #include "api/neteq/neteq_factory.h" #include "api/network_state_predictor.h" #include "api/peer_connection_interface.h" +#include "api/rtc_error.h" #include "api/rtc_event_log/rtc_event_log.h" #include "api/rtc_event_log/rtc_event_log_factory_interface.h" #include "api/rtp_parameters.h" @@ -72,6 +73,11 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) override; + RTCErrorOr> + CreatePeerConnectionOrError( + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies) override; + RtpCapabilities GetRtpSenderCapabilities( cricket::MediaType kind) const override; diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index 8730ac4bb4..39b9a73a46 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -527,9 +527,9 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) { TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurn) { RTCConfiguration configuration; PeerConnection::IceServer server; - server.urls = {"stun:dummy.stun.server/"}; + server.urls = {"stun:dummy.stun.server"}; configuration.servers.push_back(server); - server.urls = {"turn:dummy.turn.server/"}; + server.urls = {"turn:dummy.turn.server"}; server.username = "username"; server.password = "password"; configuration.servers.push_back(server); @@ -547,9 +547,9 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurn) { TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { RTCConfiguration configuration; PeerConnection::IceServer server; - server.urls = {"stun:dummy.stun.server/"}; + server.urls = {"stun:dummy.stun.server"}; configuration.servers.push_back(server); - server.urls = {"turn:dummy.turn.server/"}; + server.urls = {"turn:dummy.turn.server"}; server.username = "username"; server.password = "password"; configuration.servers.push_back(server);