Allow url::SchemeHostPort to hold non-file scheme without port

url/scheme_host_port.cc

Index: url/scheme_host_port.cc
diff --git a/url/scheme_host_port.cc b/url/scheme_host_port.cc
index c2fe830e377fa2edde0260265fd5a6b013fd4d35..aa92cbb1054531ce1d4e793dccab6363018fae8f 100644
--- a/url/scheme_host_port.cc
+++ b/url/scheme_host_port.cc
@@ -16,25 +16,20 @@
 
 namespace url {
 
-SchemeHostPort::SchemeHostPort() : port_(0) {
-}
+namespace {
 
-SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
-                               base::StringPiece host,
-                               uint16 port)
-    : scheme_(scheme.data(), scheme.length()),
-      host_(host.data(), host.length()),
-      port_(port) {
-  // Try to canonicalize the host (copy/pasted from net/base. :( ).
-  const url::Component raw_host_component(0, static_cast<int>(host.length()));
+bool IsCanonicalHost(const base::StringPiece& host) {
   std::string canon_host;
-  url::StdStringCanonOutput canon_host_output(&canon_host);
-  url::CanonHostInfo host_info;
-  url::CanonicalizeHostVerbose(host.data(), raw_host_component,
+
+  // Try to canonicalize the host (copy/pasted from net/base. :( ).
+  const Component raw_host_component(0, static_cast<int>(host.length()));

[Ryan Sleevi, 2015/08/13 22:32:12]

Same DANGER remark applies below.

[tyoshino (SeeGerritForStatus), 2015/08/14 06:29:01]

Done.

+  StdStringCanonOutput canon_host_output(&canon_host);
+  CanonHostInfo host_info;
+  CanonicalizeHostVerbose(host.data(), raw_host_component,
                                &canon_host_output, &host_info);
 
   if (host_info.out_host.is_nonempty() &&
-      host_info.family != url::CanonHostInfo::BROKEN) {
+      host_info.family != CanonHostInfo::BROKEN) {
     // Success!  Assert that there's no extra garbage.
     canon_host_output.Complete();
     DCHECK_EQ(host_info.out_host.len, static_cast<int>(canon_host.length()));
@@ -43,44 +38,95 @@ SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
     canon_host.clear();
   }
 
-  // Return an invalid SchemeHostPort object if any of the following conditions
-  // hold:
-  //
-  // 1. The provided scheme is non-standard, 'blob:', or 'filesystem:'.
-  // 2. The provided host is non-canonical.
-  // 3. The scheme is 'file' and the port is non-zero.
-  // 4. The scheme is not 'file', and the port is zero or the host is empty.
-  bool isUnsupportedScheme =
-      !url::IsStandard(scheme.data(),
-                       url::Component(0, static_cast<int>(scheme.length()))) ||
-      scheme == kFileSystemScheme || scheme == kBlobScheme;
-  bool isNoncanonicalHost = host != canon_host;
-  bool isFileSchemeWithPort = scheme == kFileScheme && port != 0;
-  bool isNonFileSchemeWithoutPortOrHost =
-      scheme != kFileScheme && (port == 0 || host.empty());
-  if (isUnsupportedScheme || isNoncanonicalHost || isFileSchemeWithPort ||
-      isNonFileSchemeWithoutPortOrHost) {
-    scheme_.clear();
-    host_.clear();
-    port_ = 0;
-  }
+  return host == canon_host;
 }
 
-SchemeHostPort::SchemeHostPort(const GURL& url) : port_(0) {
-  if (!url.is_valid() || !url.IsStandard())
-    return;
+bool IsValidInput(const base::StringPiece& scheme,
+                  const base::StringPiece& host,
+                  uint16 port) {
+  SchemeType scheme_type = SCHEME_WITH_PORT;
+  bool is_standard = GetStandardSchemeType(
+      scheme.data(),
+      Component(0, static_cast<int>(scheme.length())),

[Ryan Sleevi, 2015/08/13 22:32:12]

DANGER: I missed this in the original review, but because |scheme| might have
come from IPC (thus hostile input), it's possible that scheme.length() >
std::numeric_limits<int>::max(), thus creating an invalid component.

It may make more sense to do base::checked_cast<int>(scheme.length()), which
will still crash the browser process on an invalid IPC, but would be strictly
safer.

(Or we could rely on the fact that the IPC filter will have filtered these out
as being messages that are too large, but the nature of this constructor, and
the inherent danger, makes me think we're better off with checked_cast<>'s)

[tyoshino (SeeGerritForStatus), 2015/08/14 06:29:01]

Agreed. Changed to checked_cast.

+      &scheme_type);
+  if (!is_standard)
+    return false;
 
   // These schemes do not follow the generic URL syntax, so we treat them as
   // invalid (scheme, host, port) tuples (even though such URLs' _Origin_ might
   // have a (scheme, host, port) tuple, they themselves do not).
-  if (url.SchemeIsBlob() || url.SchemeIsFileSystem())
+  if (scheme == kFileSystemScheme || scheme == kBlobScheme)
+    return false;
+
+  switch (scheme_type) {
+    case SCHEME_WITH_PORT:
+      // A URL with |scheme| is required to have the host and port (may be
+      // omitted in a serialization if it's the same as the default value).
+      // Return an invalid instance if either of them is not given.
+      if (host.empty() || port == 0)
+        return false;
+
+      if (!IsCanonicalHost(host))
+        return false;
+
+      return true;
+
+    case SCHEME_WITHOUT_PORT:
+      if (port != 0) {
+        // Return an invalid object if the scheme is 'file' and the port is

[Ryan Sleevi, 2015/08/13 22:32:12]

This comment seems out of date/incorrect; that is, it doesn't just apply to
'file' schemed objects.

[tyoshino (SeeGerritForStatus), 2015/08/14 06:29:01]

Updated

+        // non-zero.
+        return false;
+      }
+
+      if (!IsCanonicalHost(host))
+        return false;
+
+      return true;
+
+    case SCHEME_WITHOUT_AUTHORITY:
+      return false;
+
+    default:
+      NOTREACHED();
+      return false;
+  }
+}
+
+}  // namespace
+
+SchemeHostPort::SchemeHostPort() : port_(0) {
+}
+
+SchemeHostPort::SchemeHostPort(base::StringPiece scheme,
+                               base::StringPiece host,
+                               uint16 port)
+    : port_(0) {
+  if (!IsValidInput(scheme, host, port))
     return;
 
-  scheme_ = url.scheme();
-  host_ = url.host();
-  port_ = url.EffectiveIntPort() == url::PORT_UNSPECIFIED
-              ? 0
-              : url.EffectiveIntPort();
+  scheme.CopyToString(&scheme_);
+  host.CopyToString(&host_);
+  port_ = port;
+}
+
+SchemeHostPort::SchemeHostPort(const GURL& url) : port_(0) {
+  if (!url.is_valid())
+    return;
+
+  const std::string& scheme = url.scheme();
+  const std::string& host = url.host();
+
+  // A valid GURL never returns PORT_INVALID.
+  int port = url.EffectiveIntPort();
+  if (port == PORT_UNSPECIFIED)
+    port = 0;
+
+  if (!IsValidInput(scheme, host, port))
+    return;
+
+  scheme_ = scheme;
+  host_ = host;
+  port_ = port;
 }
 
 SchemeHostPort::~SchemeHostPort() {
@@ -95,15 +141,20 @@ std::string SchemeHostPort::Serialize() const {
   if (IsInvalid())
     return result;
 
-  bool is_default_port =
-      port_ == url::DefaultPortForScheme(scheme_.data(),
-                                         static_cast<int>(scheme_.length()));
-
   result.append(scheme_);
   result.append(kStandardSchemeSeparator);
   result.append(host_);
 
-  if (scheme_ != kFileScheme && !is_default_port) {
+  if (port_ == 0)
+    return result;
+
+  // Omit the port component if the port matches with the default port
+  // defined for the scheme, if any.
+  int default_port = DefaultPortForScheme(scheme_.data(),
+                                          static_cast<int>(scheme_.length()));
+  if (default_port == PORT_UNSPECIFIED)
+    return result;
+  if (port_ != default_port) {
     result.push_back(':');
     result.append(base::IntToString(port_));
   }