Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflo

net/base/parse_number.h

 // 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.
+// 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.
+// 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 DISALLOW_NEGATIVE, and
+  // additionally recognizes those numbers with a leading '-'.
+  //
+  // 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*() family of functions parse a string representing a number.
 //
-//  Recognized inputs take the form:
-//    1*DIGIT
+// Returns true on success, and fills |*output| with the result. If

[mmenke, 2016/04/07 20:42:53]

Should these be plural, since they refer to all the follow functions?

i.e. "These return true on success, and fill..."

[eroman, 2016/04/08 18:57:32]

Reworded the comment blocks.

+// |optional_error| was non-null, then it is filled with the reason for the
+// failure.
 //
-//  Where DIGIT is an ASCII number in the range '0' - '9'
+// On failure it is guaranteed that |*output| was not modified.

[mmenke, 2016/04/07 20:42:53]

Maybe just:  "On failure |*output| will not be modified." for easier parising
("it is" in previous paragraph followed by "On failure it is..." here seems a
bit harder to parse)

[eroman, 2016/04/08 18:57:32]

Done.

 //
-//  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;
+// In the case of signed output types, |format| controls whether negative
+// numbers are accepted. When parsing to an unsigned output the format behaves
+// as if NON_NEGATIVE.
+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;
+
+NET_EXPORT bool ParseUint32(const base::StringPiece& input,

[mmenke, 2016/04/07 20:42:53]

Should these have a separate comment?  Don't think it's a huge deal, but the
format comment doesn't apply to them.  Could even put the uint methods first,
and then say "Like the Uint variants, except ParseIntFormat".  Anyhow, probably
fine as-is.

[eroman, 2016/04/08 18:57:32]

Done.

+                            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
 
 #endif  // NET_BASE_PARSE_NUMBER_H_