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}

api/peer_connection_interface.cc

@@ -87,6 +87,13 @@ PeerConnectionFactoryInterface::CreatePeerConnection(
   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 {};

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<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);

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();
 }

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.

pc/peer_connection.cc

@@ -387,7 +387,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator!=(
   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 @@ rtc::scoped_refptr<PeerConnection> 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> PeerConnection::Create(
       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 @@ 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<StreamCollectionInterface> PeerConnection::local_streams() {

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<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 @@ class PeerConnection : public PeerConnectionInternal,
   ~PeerConnection() override;
 
  private:
-  bool Initialize(
+  RTCError Initialize(
       const PeerConnectionInterface::RTCConfiguration& configuration,
       PeerConnectionDependencies dependencies);
 

pc/peer_connection_factory.cc

@@ -206,6 +206,19 @@ rtc::scoped_refptr<PeerConnectionInterface>
 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 @@ PeerConnectionFactory::CreatePeerConnection(
       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>

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<rtc::scoped_refptr<PeerConnectionInterface>>
+  CreatePeerConnectionOrError(
+      const PeerConnectionInterface::RTCConfiguration& configuration,
+      PeerConnectionDependencies dependencies) override;
+
   RtpCapabilities GetRtpSenderCapabilities(
       cricket::MediaType kind) const override;
 

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);