Extend net/base/parse_number.h for parsing of negative numbers, and d…

…etermining if there was overflow/underflow.

BUG=596523

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

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

net/base/host_port_pair.cc

@@ -38,7 +38,7 @@ HostPortPair HostPortPair::FromString(const std::string& str) {
   if (key_port.size() != 2)
     return HostPortPair();
   int port;
-  if (!ParseNonNegativeDecimalInt(key_port[1], &port))
+  if (!ParseInt32(key_port[1], ParseIntFormat::NON_NEGATIVE, &port))
     return HostPortPair();
   if (!IsPortValid(port))
     return HostPortPair();

net/base/ip_address.cc

@@ -180,16 +180,15 @@ bool ParseCIDRBlock(const std::string& cidr_literal,
     return false;
 
   // Parse the prefix length.
-  int number_of_bits = -1;
-  if (!ParseNonNegativeDecimalInt(parts[1], &number_of_bits))
+  uint32_t number_of_bits;
+  if (!ParseUint32(parts[1], &number_of_bits))
     return false;
 
   // Make sure the prefix length is in a valid range.
-  if (number_of_bits < 0 ||
-      number_of_bits > static_cast<int>(ip_address->size() * 8))
+  if (number_of_bits > ip_address->size() * 8)
     return false;
 
-  *prefix_length_in_bits = static_cast<size_t>(number_of_bits);
+  *prefix_length_in_bits = number_of_bits;
   return true;
 }
 

net/base/parse_number.cc

@@ -4,20 +4,123 @@
 
 #include "net/base/parse_number.h"
 
+#include "base/logging.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;
+namespace {
+
+// The string to number conversion functions in //base include the type in the
+// name (like StringToInt64()). The following wrapper methods create a
+// consistent interface to StringToXXX() that calls the appropriate //base
+// version. This simplifies writing generic code with a template.
+
+bool StringToNumber(const base::StringPiece& input, int32_t* output) {
+  // This assumes ints are 32-bits (will fail compile if that ever changes).
+  return base::StringToInt(input, output);
+}
+
+bool StringToNumber(const base::StringPiece& input, uint32_t* output) {
+  // This assumes ints are 32-bits (will fail compile if that ever changes).
+  return base::StringToUint(input, output);
+}
+
+bool StringToNumber(const base::StringPiece& input, int64_t* output) {
+  return base::StringToInt64(input, output);
+}
+
+bool StringToNumber(const base::StringPiece& input, uint64_t* output) {
+  return base::StringToUint64(input, output);
+}
+
+bool SetError(ParseIntError error, ParseIntError* optional_error) {
+  if (optional_error)
+    *optional_error = error;
+  return false;
+}
+
+template <typename T>
+bool ParseIntHelper(const base::StringPiece& input,
+                    ParseIntFormat format,
+                    T* output,
+                    ParseIntError* optional_error) {
+  // Check that the input matches the format before calling StringToNumber().
+  // Numbers must start with either a digit or a negative sign.
+  if (input.empty())
+    return SetError(ParseIntError::FAILED_PARSE, optional_error);
+
+  bool starts_with_negative = input[0] == '-';
+  bool starts_with_digit = input[0] >= '0' && input[0] <= '9';
 
-  int result;
-  if (!base::StringToInt(input, &result))
+  if (!starts_with_digit) {
+    if (format == ParseIntFormat::NON_NEGATIVE || !starts_with_negative)
+      return SetError(ParseIntError::FAILED_PARSE, optional_error);
+  }
+
+  // Dispatch to the appropriate flavor of base::StringToXXX() by calling one of
+  // the type-specific overloads.
+  T result;
+  if (StringToNumber(input, &result)) {
+    *output = result;
+    return true;
+  }
+
+  // Optimization: If the error is not going to be inspected, don't bother
+  // calculating it.
+  if (!optional_error)
     return false;
 
-  *output = result;
-  return true;
+  // Set an error that distinguishes between parsing/underflow/overflow errors.
+  //
+  // Note that the output set by base::StringToXXX() on failure cannot be used
+  // as it has ambiguity with parse errors.
+
+  // Strip any leading negative sign off the number.
+  base::StringPiece numeric_portion =
+      starts_with_negative ? input.substr(1) : input;
+
+  // Test if |numeric_portion| is a valid non-negative integer.
+  if (!numeric_portion.empty() &&
+      numeric_portion.find_first_not_of("0123456789") == std::string::npos) {
+    // If it was, the failure must have been due to underflow/overflow.
+    return SetError(starts_with_negative ? ParseIntError::FAILED_UNDERFLOW
+                                         : ParseIntError::FAILED_OVERFLOW,
+                    optional_error);
+  }
+
+  // Otherwise it was a mundane parsing error.
+  return SetError(ParseIntError::FAILED_PARSE, optional_error);
+}
+
+}  // namespace
+
+bool ParseInt32(const base::StringPiece& input,
+                ParseIntFormat format,
+                int32_t* output,
+                ParseIntError* optional_error) {
+  return ParseIntHelper(input, format, output, optional_error);
+}
+
+bool ParseInt64(const base::StringPiece& input,
+                ParseIntFormat format,
+                int64_t* output,
+                ParseIntError* optional_error) {
+  return ParseIntHelper(input, format, output, optional_error);
+}
+
+bool ParseUint32(const base::StringPiece& input,
+                 uint32_t* output,
+                 ParseIntError* optional_error) {
+  return ParseIntHelper(input, ParseIntFormat::NON_NEGATIVE, output,
+                        optional_error);
+}
+
+bool ParseUint64(const base::StringPiece& input,
+                 uint64_t* output,
+                 ParseIntError* optional_error) {
+  return ParseIntHelper(input, ParseIntFormat::NON_NEGATIVE, output,
+                        optional_error);
 }
 
 }  // namespace net

net/base/parse_number.h

@@ -15,53 +15,100 @@
 // 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.
+// 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 using the base::StringTo*() has historically led to subtle bugs
+//    in the context of parsing network protocols:
 //
-//    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.
+//      * Permitting a leading '+'
+//      * Incorrectly classifying overflow/underflow from a parsing failure
+//      * Allowing negative numbers for non-negative fields
 //
-//    To reduce the risk of such problems, use of these functions over the
-//    //base versions.
+//   This API tries to avoid these problems by picking sensible defaults for
+//   //net code. For more details see crbug.com/596523.
 
 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
+// Format to use when parsing integers.
+enum class ParseIntFormat {
+  // Accepts non-negative base 10 integers of the form:
+  //
+  //    1*DIGIT
+  //
+  // This construction is used in a variety of IETF standards, such as RFC 7230
+  // (HTTP).
+  //
+  // When attempting to parse a negative number using this format, the failure
+  // will be FAILED_PARSE since it violated the expected format (and not
+  // FAILED_UNDERFLOW).
+  //
+  // Also note that inputs need not be in minimal encoding: "0003" is valid and
+  // equivalent to "3".
+  NON_NEGATIVE,
+
+  // Accept optionally negative base 10 integers of the form:
+  //
+  //    ["-"] 1*DIGIT
+  //
+  // In other words, this accepts the same things as NON_NEGATIVE, and
+  // additionally recognizes those numbers prefixed with a '-'.
+  //
+  // Note that by this defintion "-0" IS a valid input.
+  OPTIONALLY_NEGATIVE
+};
+
+// The specific reason why a ParseInt*() function failed.
+enum class ParseIntError {
+  // The parsed number couldn't fit into the provided output type because it was
+  // too high.
+  FAILED_OVERFLOW,
+
+  // The parsed number couldn't fit into the provided output type because it was
+  // too low.
+  FAILED_UNDERFLOW,
+
+  // The number failed to be parsed because it wasn't a valid decimal number (as
+  // determined by the policy).
+  FAILED_PARSE,
+};
+
+// The ParseInt*() functions parse a string representing a number.
 //
-//  Where DIGIT is an ASCII number in the range '0' - '9'
+// The format of the strings that are accepted is controlled by the |format|
+// parameter. This allows rejecting negative numbers.
 //
-//  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
+// These functions return true on success, and fill |*output| with the result.
 //
-//  Examples of recognized inputs are:
-//    "13"
-//    "0"
-//    "00013"
+// On failure, it is guaranteed that |*output| was not modified. If
+// |optional_error| was non-null, then it is filled with the reason for the
+// failure.
+NET_EXPORT bool ParseInt32(const base::StringPiece& input,
+                           ParseIntFormat format,
+                           int32_t* output,
+                           ParseIntError* optional_error = nullptr)
+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseInt64(const base::StringPiece& input,
+                           ParseIntFormat format,
+                           int64_t* output,
+                           ParseIntError* optional_error = nullptr)
+    WARN_UNUSED_RESULT;
+
+// The ParseUint*() functions parse a string representing a number.
 //
-//  Examples of rejected inputs are:
-//    "  13"
-//    "-13"
-//    "+13"
-//    "0x15"
-//    "13.3"
-NET_EXPORT bool ParseNonNegativeDecimalInt(const base::StringPiece& input,
-                                           int* output) WARN_UNUSED_RESULT;
+// These are equivalent to calling ParseInt*() with a format string of
+// ParseIntFormat::NON_NEGATIVE and unsigned output types.
+NET_EXPORT bool ParseUint32(const base::StringPiece& input,
+                            uint32_t* output,
+                            ParseIntError* optional_error = nullptr)
+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseUint64(const base::StringPiece& input,
+                            uint64_t* output,
+                            ParseIntError* optional_error = nullptr)
+    WARN_UNUSED_RESULT;
 
 }  // namespace net