Add create function for PeerConnection that can return an error.
Needed in order to return different codes for different failures
in initialization.
Sideswipe: Check TURN URL hostnames for illegal characters.
Bug: webrtc:12238
Change-Id: I1af3a37b9654b83b268304f7356049f9f3786b7a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/195541
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32710}
diff --git a/api/peer_connection_interface.cc b/api/peer_connection_interface.cc
index f82e84b..e1d94dd 100644
--- a/api/peer_connection_interface.cc
+++ b/api/peer_connection_interface.cc
@@ -87,6 +87,13 @@
return nullptr;
}
+RTCErrorOr<rtc::scoped_refptr<PeerConnectionInterface>>
+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 0e6cd5c..92d965b 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -1428,6 +1428,12 @@
// configuration and a PeerConnectionDependencies structure.
// TODO(benwright): Make pure virtual once downstream mock PC factory classes
// are updated.
+ virtual RTCErrorOr<rtc::scoped_refptr<PeerConnectionInterface>>
+ 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<PeerConnectionInterface> CreatePeerConnection(
const PeerConnectionInterface::RTCConfiguration& configuration,
PeerConnectionDependencies dependencies);
diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc
index 2400fd5..4764137 100644
--- a/pc/ice_server_parsing.cc
+++ b/pc/ice_server_parsing.cc
@@ -31,6 +31,15 @@
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 @@
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 @@
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 @@
} 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 2625b24..e4dbd3a 100644
--- a/pc/ice_server_parsing_unittest.cc
+++ b/pc/ice_server_parsing_unittest.cc
@@ -182,6 +182,11 @@
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 3760a01..9ba7dae 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -387,7 +387,7 @@
return !(*this == o);
}
-rtc::scoped_refptr<PeerConnection> PeerConnection::Create(
+RTCErrorOr<rtc::scoped_refptr<PeerConnection>> PeerConnection::Create(
rtc::scoped_refptr<ConnectionContext> context,
const PeerConnectionFactoryInterface::Options& options,
std::unique_ptr<RtcEventLog> event_log,
@@ -397,22 +397,26 @@
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 @@
new rtc::RefCountedObject<PeerConnection>(
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 @@
});
}
-bool PeerConnection::Initialize(
+RTCError PeerConnection::Initialize(
const PeerConnectionInterface::RTCConfiguration& configuration,
PeerConnectionDependencies dependencies) {
RTC_DCHECK_RUN_ON(signaling_thread());
@@ -511,7 +517,7 @@
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 @@
},
delay_ms);
- return true;
+ return RTCError::OK();
}
rtc::scoped_refptr<StreamCollectionInterface> PeerConnection::local_streams() {
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 52a302b..8768ebb 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -124,7 +124,7 @@
//
// 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<PeerConnection> Create(
+ static RTCErrorOr<rtc::scoped_refptr<PeerConnection>> Create(
rtc::scoped_refptr<ConnectionContext> context,
const PeerConnectionFactoryInterface::Options& options,
std::unique_ptr<RtcEventLog> event_log,
@@ -459,7 +459,7 @@
~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 da42e5a..f4f72c7 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -206,6 +206,19 @@
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<rtc::scoped_refptr<PeerConnectionInterface>>
+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 @@
RTC_FROM_HERE,
rtc::Bind(&PeerConnectionFactory::CreateCall_w, this, event_log.get()));
- rtc::scoped_refptr<PeerConnection> 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<PeerConnectionInterface> result_proxy =
+ PeerConnectionProxy::Create(signaling_thread(), result.MoveValue());
+ return result_proxy;
}
rtc::scoped_refptr<MediaStreamInterface>
diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h
index 427207f..9c4a2b0 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 @@
const PeerConnectionInterface::RTCConfiguration& configuration,
PeerConnectionDependencies dependencies) override;
+ RTCErrorOr<rtc::scoped_refptr<PeerConnectionInterface>>
+ 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 8730ac4..39b9a73 100644
--- a/pc/peer_connection_histogram_unittest.cc
+++ b/pc/peer_connection_histogram_unittest.cc
@@ -527,9 +527,9 @@
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, 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);