Catching more errors when parsing ICE server URLs.

talk/app/webrtc/peerconnection.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 26 matching lines @@
 #include "talk/app/webrtc/streamcollection.h"
 #include "webrtc/p2p/client/basicportallocator.h"
 #include "talk/session/media/channelmanager.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringencode.h"
 #include "webrtc/system_wrappers/interface/field_trial.h"
 
 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
 static const size_t kTurnHostTokensNum = 2;
 // Number of tokens must be preset when TURN uri has transport param.
 static const size_t kTurnTransportTokensNum = 2;
 // The default stun port.
 static const int kDefaultStunPort = 3478;
 static const int kDefaultStunTlsPort = 5349;
 static const char kTransport[] = "transport";
 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.

   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.
 };
 
 enum {
   MSG_SET_SESSIONDESCRIPTION_SUCCESS = 0,
   MSG_SET_SESSIONDESCRIPTION_FAILED,
   MSG_GETSTATS,
@@ skipping 16 matching lines @@
   }
   rtc::scoped_refptr<webrtc::StatsObserver> observer;
   rtc::scoped_refptr<webrtc::MediaStreamTrackInterface> track;
 };
 
 // |in_str| should be of format
 // stunURI       = scheme ":" stun-host [ ":" stun-port ]
 // 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
 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).

       *service_type = static_cast<ServiceType>(i);
       break;
     }
   }
   if (*service_type == INVALID) {
     return false;
   }
   *hostname = in_str.substr(colonpos + 1, std::string::npos);
   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) {
   if (in_str.at(0) == '[') {

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

Is there ever a chance that in_str is empty?

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

Not any longer.

     std::string::size_type closebracket = in_str.rfind(']');
     if (closebracket != std::string::npos) {
       *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;
         }
       }
     } else {
       return false;
     }
   } else {
     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"
   // stun-host     = IP-literal / IPv4address / reg-name
   // stun-port     = *DIGIT
 
   // draft-petithuguenin-behave-turn-uris-01
   // turnURI       = scheme ":" turn-host [ ":" turn-port ]
   //                 [ "?transport=" transport ]
   // scheme        = "turn" / "turns"
   // transport     = "udp" / "tcp" / transport-ext
   // transport-ext = 1*unreserved
   // turn-host     = IP-literal / IPv4address / reg-name
   // turn-port     = *DIGIT
   std::vector<std::string> tokens;
   std::string turn_transport_type = kUdpTransportType;
   ASSERT(!url.empty());
   rtc::tokenize(url, '?', &tokens);
   std::string uri_without_transport = tokens[0];
   // Let's look into transport= param, if it exists.
   if (tokens.size() == kTurnTransportTokensNum) {  // ?transport= is present.
     std::string uri_transport_param = tokens[1];
     rtc::tokenize(uri_transport_param, '=', &tokens);
     if (tokens[0] == kTransport) {
       // As per above grammar transport param will be consist of lower case
       // 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 {
     hoststring = tokens[0];
   }
 
   int port = kDefaultStunPort;
   if (service_type == TURNS) {
     port = kDefaultStunTlsPort;
     turn_transport_type = kTcpTransportType;
   }
 
   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) {
     case STUN:
     case STUNS:
       stun_config->push_back(StunConfiguration(address, port));
       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,
                                                turn_transport_type,
                                                secure));
       break;
     }
     case INVALID:
     default:
       LOG(WARNING) << "Configuration not supported: " << url;
       return false;
   }
   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) {
         if (url.empty()) {
           LOG(WARNING) << "Empty uri.";
           continue;
         }
         if (!ParseIceServerUrl(server, url, stun_config, turn_config)) {
           return false;
         }
@@ skipping 19 matching lines @@
     return false;
   if (current_streams->find(new_stream->label()) != NULL) {
     LOG(LS_ERROR) << "MediaStream with label " << new_stream->label()
                   << " is already added.";
     return false;
   }
 
   return true;
 }
 
-}  // namespace
-
-namespace webrtc {
-
 PeerConnection::PeerConnection(PeerConnectionFactory* factory)
     : factory_(factory),
       observer_(NULL),
       uma_observer_(NULL),
       signaling_state_(kStable),
       ice_state_(kIceNew),
       ice_connection_state_(kIceConnectionNew),
       ice_gathering_state_(kIceGatheringNew) {
 }
 
@@ skipping 564 matching lines @@
     if (ice_gathering_state_ != kIceGatheringComplete) {
       ice_gathering_state_ = kIceGatheringComplete;
       observer_->OnIceGatheringChange(ice_gathering_state_);
     }
   }
   observer_->OnSignalingChange(signaling_state_);
   observer_->OnStateChange(PeerConnectionObserver::kSignalingState);
 }
 
 }  // namespace webrtc