Catching more errors when parsing ICE server URLs.

talk/app/webrtc/peerconnection.cc

Index: talk/app/webrtc/peerconnection.cc
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index ef5836bbdd4a7dac84d33ae484bd8f3c43544486..57579323a3a6b5524be49aa7e472745c065d1cce 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -44,6 +44,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
@@ -58,8 +64,11 @@ 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",
+                                              "invalid"};

[tommi, 2015/09/16 21:51:00]

why do we need this last value?

[Taylor Brandstetter, 2015/09/24 00:25:04]

It isn't; good catch.

 
 enum ServiceType {
   STUN,     // Indicates a STUN server.

[tommi, 2015/09/16 21:51:01]

can you explicitly set this to 0 and add a comment for the enum that we actually
have a loop that assumes that the first value is 0 and all values are
incremental.
It would also be good to have a static_assert that INVALID equals
arraysize(kValidIceServiceTypes) - 1 (or arraysize(kValidIceServiceTypes) if we
don't need the "invalid" string above)

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

@@ -99,7 +108,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
@@ -107,9 +116,15 @@ 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;
   }
+  if ((colonpos + 1) == in_str.length()) {
+    LOG(LS_WARNING) << "Empty hostname in ICE URI: " << in_str;
+    return false;
+  }
+  *service_type = INVALID;
   std::string type = in_str.substr(0, colonpos);
   for (size_t i = 0; i < ARRAY_SIZE(kValidIceServiceTypes); ++i) {
     if (type.compare(kValidIceServiceTypes[i]) == 0) {

[tommi, 2015/09/16 21:51:00]

nit: We don't actually need to create a new string just to compare a substring. 
I think that here we could just do:
if (in_str.compare(0, colonpos, kValidIceServiceTypes[i]) == 0) {

right?  I don't suppose there's a unit test that covers this function btw?

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done. And although there's no unit test for this function *individually*, it's
still covered by a test I added (IceServerParsingTest.ParseStunPrefixes).

@@ -124,11 +139,19 @@ 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.
+  if (in_str.find_first_not_of("0123456789") != std::string::npos) {

[tommi, 2015/09/16 21:51:01]

nit: could use isdigit() in a loop for O(n) ;)

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

+    return false;
+  }
+  return rtc::FromString(in_str, port);

[tommi, 2015/09/16 21:51:00]

out of curiosity I took a look at FromString() and it seems to create an
istringstream from the string before extracting the int and does some more
complex parsing too that you don't need here since you know you'll only be
dealing with digits.  I think this is probably enough and should generate
significantly smaller code:

if (in_str.empty())  <- this could also be the first thing the function checks
  return false;
*port = atoi(in_str.c_str());
return true;

[Taylor Brandstetter, 2015/09/24 00:25:04]

atoi still allows whitespace and non-numeric characters though, so I still want
to do some sort of "is only digits" check. And "FromString(in_str, port)" looks
nicer (in my opinion) and is more consistent with other code than "*port =
atoi(in_str.c_str())"

+}
+
 // 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,

[tommi, 2015/09/16 21:51:01]

what about adding a DCHECK() at the top of this function:

DCHECK(host->empty());

(see the last check in the function)

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

                                     int* port) {
@@ -138,8 +161,8 @@ bool ParseHostnameAndPortFromString(const std::string& in_str,
       *host = in_str.substr(1, closebracket - 1);

[tommi, 2015/09/16 21:51:00]

nit: we don't really need to assign to host until after we've checked the port.
(I'm a fan of not having modified output parameters in case of failures)

[Taylor Brandstetter, 2015/09/24 00:25:05]

Done.

       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;
         }
       }
@@ -150,26 +173,25 @@ bool ParseHostnameAndPortFromString(const std::string& in_str,
     std::string::size_type colonpos = in_str.find(':');
     if (std::string::npos != colonpos) {
       *host = in_str.substr(0, colonpos);

[tommi, 2015/09/16 21:51:00]

same here

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

-      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;
       }
     } else {
       *host = in_str;
     }
   }
+  if (host->empty()) {

[tommi, 2015/09/16 21:51:01]

and here I'd skip the if() and just do:

return !host->empty();

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

+    return false;
+  }
   return true;
 }
 
-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<StunConfiguration>* stun_config,
-                       std::vector<TurnConfiguration>* turn_config) {
+                       StunConfigurations* stun_config,

[tommi, 2015/09/16 21:51:01]

can these params ever be null? If they're never allowed to be, can you add
DCHECKs to that effect?

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

+                       TurnConfigurations* turn_config) {
   // draft-nandakumar-rtcweb-stun-uri-01
   // stunURI       = scheme ":" stun-host [ ":" stun-port ]
   // scheme        = "stun" / "stuns"
@@ -198,32 +220,37 @@ 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;
   }
 
+  // GetServiceTypeAndHostnameFromUri should never give an empty hoststring
   ASSERT(!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;
+  } else if (tokens.size() == kTurnHostTokensNum) {

[tommi, 2015/09/16 21:51:00]

nit: since the previous scope has an early return, the 'else' part isn't needed.

[Taylor Brandstetter, 2015/09/24 00:25:04]

Done.

+    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 {
@@ -238,13 +265,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) {
@@ -254,18 +281,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<std::string> 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];
-        }
-      }
-

[Taylor Brandstetter, 2015/09/15 22:36:05]

I'm pretty confident this code can be safely removed, since it does the same
thing as the tokenizing code above. The only difference is that the code above
operates on the string "user@hostname:port", while the deleted code operates
"user@hostname". But this makes no difference since in either case it just peels
off the name before the '@' sign.

       bool secure = (service_type == TURNS);
-
       turn_config->push_back(TurnConfiguration(address, port,
                                                username,
                                                server.password,
@@ -281,9 +297,13 @@ bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
   return true;
 }
 
+}  // namespace
+
+namespace webrtc {
+
 bool ParseIceServers(const PeerConnectionInterface::IceServers& servers,
-                     std::vector<StunConfiguration>* stun_config,
-                     std::vector<TurnConfiguration>* 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) {
@@ -323,10 +343,6 @@ bool CanAddLocalMediaStream(webrtc::StreamCollectionInterface* current_streams,
   return true;
 }
 
-}  // namespace
-
-namespace webrtc {
-
 PeerConnection::PeerConnection(PeerConnectionFactory* factory)
     : factory_(factory),
       observer_(NULL),