Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it permits strings with a leading plus sign, which is invalid in most places where //net parses numbers.

BUG=596523, 596538

Review URL: https://codereview.chromium.org/1829873002

Cr-Commit-Position: refs/heads/master@{#382936}

8 files changed, 174 insertions, 16 deletions

diff --git a/net/base/host_port_pair.cc b/net/base/host_port_pair.cc
index a0dd2d5..441a359 100644
--- a/net/base/host_port_pair.cc
+++ b/net/base/host_port_pair.cc
@@ -10,6 +10,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/ip_endpoint.h"
+#include "net/base/parse_number.h"
 #include "net/base/port_util.h"
 #include "url/gurl.h"
 
@@ -37,7 +38,7 @@ HostPortPair HostPortPair::FromString(const std::string& str) {
   if (key_port.size() != 2)
     return HostPortPair();
   int port;
-  if (!base::StringToInt(key_port[1], &port))
+  if (!ParseNonNegativeDecimalInt(key_port[1], &port))
     return HostPortPair();
   if (!IsPortValid(port))
     return HostPortPair();
diff --git a/net/base/host_port_pair_unittest.cc b/net/base/host_port_pair_unittest.cc
index 8047cc3..72704a0 100644
--- a/net/base/host_port_pair_unittest.cc
+++ b/net/base/host_port_pair_unittest.cc
@@ -38,11 +38,8 @@ TEST(HostPortPairTest, Parsing) {
 
 TEST(HostPortPairTest, BadString) {
   const char* kBadStrings[] = {
-    "foo.com:2:3",
-    "bar.com:two",
-    "www.google.com:-1",
-    "127.0.0.1:65536",
-    "[2001:db8::42]:65536",
+      "foo.com:2:3",       "bar.com:two",     "www.google.com:-1",
+      "www.google.com:+1", "127.0.0.1:65536", "[2001:db8::42]:65536",
   };
 
   for (size_t index = 0; index < arraysize(kBadStrings); ++index) {
diff --git a/net/base/ip_address.cc b/net/base/ip_address.cc
index 26c8cc5..b805317e 100644
--- a/net/base/ip_address.cc
+++ b/net/base/ip_address.cc
@@ -4,10 +4,10 @@
 
 #include "net/base/ip_address.h"
 
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "net/base/ip_address_number.h"
+#include "net/base/parse_number.h"
 #include "url/gurl.h"
 #include "url/url_canon_ip.h"
 
@@ -179,15 +179,9 @@ bool ParseCIDRBlock(const std::string& cidr_literal,
   if (!ip_address->AssignFromIPLiteral(parts[0]))
     return false;
 
-  // TODO(martijnc): Find a more general solution for the overly permissive
-  // base::StringToInt() parsing. https://crbug.com/596523.
-  const base::StringPiece& prefix_length = parts[1];
-  if (prefix_length.starts_with("+"))
-    return false;
-
   // Parse the prefix length.
   int number_of_bits = -1;
-  if (!base::StringToInt(prefix_length, &number_of_bits))
+  if (!ParseNonNegativeDecimalInt(parts[1], &number_of_bits))
     return false;
 
   // Make sure the prefix length is in a valid range.
diff --git a/net/base/parse_number.cc b/net/base/parse_number.cc
new file mode 100644
index 0000000..d315bec
--- /dev/null
+++ b/net/base/parse_number.cc
@@ -0,0 +1,23 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/parse_number.h"
+
+#include "base/strings/string_number_conversions.h"
+
+namespace net {
+
+bool ParseNonNegativeDecimalInt(const base::StringPiece& input, int* output) {
+  if (input.empty() || input[0] > '9' || input[0] < '0')
+    return false;
+
+  int result;
+  if (!base::StringToInt(input, &result))
+    return false;
+
+  *output = result;
+  return true;
+}
+
+}  // namespace net
diff --git a/net/base/parse_number.h b/net/base/parse_number.h
new file mode 100644
index 0000000..0b88cf0
--- /dev/null
+++ b/net/base/parse_number.h
@@ -0,0 +1,68 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_BASE_PARSE_NUMBER_H_
+#define NET_BASE_PARSE_NUMBER_H_
+
+#include "base/compiler_specific.h"
+#include "base/strings/string_piece.h"
+#include "net/base/net_export.h"
+
+// This file contains utility functions for parsing numbers, in the context of
+// network protocols.
+//
+// Q: Doesn't //base already provide these in string_number_conversions.h, with
+//    functions like base::StringToInt()?
+//
+// A: Yes, and those functions are used under the hood by these
+//    implementations.
+//
+//    However using the number parsing functions from //base directly in network
+//    code can lead to subtle bugs, as the //base versions  are more permissive.
+//    For instance "+99" is successfully parsed by base::StringToInt().
+//
+//    However in the majority of places in //net, a leading plus on a number
+//    should be considered invalid. For instance when parsing a host:port pair
+//    you wouldn't want to recognize "foo:+99" as having a port of 99. The same
+//    issue applies when parsing a content-length header.
+//
+//    To reduce the risk of such problems, use of these functions over the
+//    //base versions.
+
+class GURL;
+
+namespace net {
+
+//  Parses a string representing a decimal number to an |int|. Returns true on
+//  success, and fills |*output| with the result. Note that  |*output| is not
+//  modified on failure.
+//
+//  Recognized inputs take the form:
+//    1*DIGIT
+//
+//  Where DIGIT is an ASCII number in the range '0' - '9'
+//
+//  Note that:
+//   * Parsing is locale independent
+//   * Leading zeros are allowed (numbers needn't be in minimal encoding)
+//   * Inputs that would overflow the output are rejected.
+//   * Only accepts integers
+//
+//  Examples of recognized inputs are:
+//    "13"
+//    "0"
+//    "00013"
+//
+//  Examples of rejected inputs are:
+//    "  13"
+//    "-13"
+//    "+13"
+//    "0x15"
+//    "13.3"
+NET_EXPORT bool ParseNonNegativeDecimalInt(const base::StringPiece& input,
+                                           int* output) WARN_UNUSED_RESULT;
+
+}  // namespace net
+
+#endif  // NET_BASE_PARSE_NUMBER_H_
diff --git a/net/base/parse_number_unittest.cc b/net/base/parse_number_unittest.cc
new file mode 100644
index 0000000..79cac9f
--- /dev/null
+++ b/net/base/parse_number_unittest.cc
@@ -0,0 +1,71 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/parse_number.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+namespace {
+
+TEST(ParseNumberTest, IntValidInputs) {
+  const struct {
+    const char* input;
+    int output;
+  } kTests[] = {
+      {"0", 0},     {"00000", 0}, {"003", 3}, {"003", 3}, {"1234566", 1234566},
+      {"987", 987}, {"010", 10},
+  };
+
+  for (const auto& test : kTests) {
+    int result;
+    ASSERT_TRUE(ParseNonNegativeDecimalInt(test.input, &result))
+        << "Failed to parse: " << test.input;
+    EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input;
+  }
+}
+
+TEST(ParseNumberTest, IntInvalidInputs) {
+  const char* kTests[] = {
+      "",
+      "-23",
+      "+42",
+      " 123",
+      "123 ",
+      "123\n",
+      "0xFF",
+      "0x11",
+      "x11",
+      "F11",
+      "AF",
+      "0AF",
+      "0.0",
+      "13.",
+      "13,000",
+      "13.000",
+      "13/5",
+      "9999999999999999999999999999999999999999999999999999999999999999",
+      "Inf",
+      "NaN",
+      "null",
+      "dog",
+  };
+
+  for (const auto& input : kTests) {
+    int result = 0xDEAD;
+    ASSERT_FALSE(ParseNonNegativeDecimalInt(input, &result))
+        << "Succeeded to parse: " << input;
+    EXPECT_EQ(0xDEAD, result) << "Modified output for failed parsing";
+  }
+}
+
+TEST(ParseNumberTest, IntInvalidInputsContainsNul) {
+  int result = 0xDEAD;
+  ASSERT_FALSE(
+      ParseNonNegativeDecimalInt(base::StringPiece("123\0", 4), &result));
+  EXPECT_EQ(0xDEAD, result);
+}
+
+}  // namespace
+}  // namespace net
diff --git a/net/base/sdch_manager.cc b/net/base/sdch_manager.cc
index a827270..4359772 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -16,6 +16,7 @@
 #include "base/time/default_clock.h"
 #include "base/values.h"
 #include "crypto/sha2.h"
+#include "net/base/parse_number.h"
 #include "net/base/sdch_observer.h"
 #include "net/url_request/url_request_http_job.h"
 
@@ -376,12 +377,12 @@ SdchProblemCode SdchManager::AddSdchDictionary(
           return SDCH_DICTIONARY_UNSUPPORTED_VERSION;
       } else if (name == "max-age") {
         int64_t seconds;
+        // TODO(eroman): crbug.com/596541 -- should not accept a leading +.
         base::StringToInt64(value, &seconds);
         expiration = base::Time::Now() + base::TimeDelta::FromSeconds(seconds);
       } else if (name == "port") {
         int port;
-        base::StringToInt(value, &port);
-        if (port >= 0)
+        if (ParseNonNegativeDecimalInt(value, &port))
           ports.insert(port);
       }
     }
diff --git a/net/net.gypi b/net/net.gypi
index b665104..b6efc17 100644
--- a/net/net.gypi
+++ b/net/net.gypi
@@ -53,6 +53,8 @@
       'base/openssl_private_key_store.h',
       'base/openssl_private_key_store_android.cc',
       'base/openssl_private_key_store_memory.cc',
+      'base/parse_number.cc',
+      'base/parse_number.h',
       'base/port_util.cc',
       'base/port_util.h',
       'base/rand_callback.h',
@@ -1363,6 +1365,7 @@
       'base/network_change_notifier_win_unittest.cc',
       'base/network_interfaces_unittest.cc',
       'base/network_quality_estimator_unittest.cc',
+      'base/parse_number_unittest.cc',
       'base/port_util_unittest.cc',
       'base/prioritized_dispatcher_unittest.cc',
       'base/priority_queue_unittest.cc',