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

net/base/parse_number_unittest.cc

Index: net/base/parse_number_unittest.cc
diff --git a/net/base/parse_number_unittest.cc b/net/base/parse_number_unittest.cc
index 79cac9f43ba211b26aa8d58d1c943154b387f911..1c6817a7feeb6128da68053f952261dec8bed526 100644
--- a/net/base/parse_number_unittest.cc
+++ b/net/base/parse_number_unittest.cc
@@ -4,68 +4,229 @@
 
 #include "net/base/parse_number.h"
 
+#include <limits>
+
+#include "base/strings/string_number_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 namespace {
 
-TEST(ParseNumberTest, IntValidInputs) {
+// Increments the final character in a string by 1. Used as a simplistic way to
+// increment a numeric string (flawed, but good enough for use case).
+std::string IncrementLastChar(std::string value) {
+  value[value.size() - 1]++;
+  return value;
+}
+
+// Common tests to run for each of the overloaded versions of ParseInteger().
+// Most checks are the same, however there are differences based on the
+// signedness of the output type T, and its range.
+template <typename T>
+void TestParseInteger() {
+  ParseIntegerSignPolicy kAllSignPolicies[] = {
+      ParseIntegerSignPolicy::ANY, ParseIntegerSignPolicy::NON_NEGATIVE};
+
+  // Arbitrary constant, used to test if failure modifies the output parameter.
+  const T kFailResult = T(23614);
+
+  // ------------------------------------
+  // Valid (non-negative) inputs
+  // ------------------------------------
   const struct {
     const char* input;
-    int output;
-  } kTests[] = {
+    T output;
+  } kValidTests[] = {
       {"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;
+  for (const auto& sign_policy : kAllSignPolicies) {
+    for (const auto& test : kValidTests) {
+      T result;
+      auto rv = ParseInteger(test.input, sign_policy, &result);
+      ASSERT_EQ(ParseIntegerResult::OK, rv) << "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",
+  // ------------------------------------
+  // Invalid inputs (regardless of sign policy)
+  // ------------------------------------
+
+  const char* kInvalidParseTests[] = {
+      "",       "-",      "--",    "23-",  "134-34", "- ",   "    ",  "+42",
+      " 123",   "123 ",   "123\n", "0xFF", "-0xFF",  "0x11", "-0x11", "x11",
+      "-x11",   "F11",    "-F11",  "AF",   "-AF",    "0AF",  "0.0",   "13.",
+      "13,000", "13.000", "13/5",  "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";
+  for (const auto& sign_policy : kAllSignPolicies) {
+    for (const auto& input : kInvalidParseTests) {
+      T result = kFailResult;
+
+      auto rv = ParseInteger(input, sign_policy, &result);
+      EXPECT_EQ(ParseIntegerResult::FAILED_PARSE, rv)
+          << "Unexpected result parsing: " << input;
+
+      // On failure the output should not have been changed.
+      EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+    }
+  }
+
+  // ------------------------------------
+  // Valid negative inputs.
+  // ------------------------------------
+
+  const struct {
+    const char* input;
+    int output;  // Using |int| rather than T because T may be unsigned.
+  } kValidNegativeTests[] = {
+      {"-0", 0}, {"-0001", -1}, {"-134", -134}, {"-4135", -4135},
+  };
+
+  for (const auto& sign_policy : kAllSignPolicies) {
+    for (const auto& test : kValidNegativeTests) {
+      T result = kFailResult;
+      auto rv = ParseInteger(test.input, sign_policy, &result);
+
+      // The result depends on the sign policy, and the signedness of the
+      // output type.
+      if (sign_policy == ParseIntegerSignPolicy::NON_NEGATIVE) {
+        // If non-negative numbers were requested, should result in parse
+        // failure (even for "-0" which is in some sense non-negative).
+        EXPECT_EQ(ParseIntegerResult::FAILED_PARSE, rv)
+            << "Unexpected result parsing: " << test.input;
+        // On failure the output should not have been changed.
+        EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+      } else if (!std::numeric_limits<T>::is_signed) {
+        // If the output type was unsigned but the the input was negative,
+        // can't have succeeded.
+        // TODO(eroman): This applies even to "-0" even though it technically
+        // could have been parsed without "underflow". This is a consequence of
+        // how base::StringToIntXXX() works for unsigned types. Meh.
+        EXPECT_EQ(ParseIntegerResult::FAILED_UNDERFLOW, rv)
+            << "Unexpected result parsing: " << test.input;
+        // On failure the output should not have been changed.
+        EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+      } else {
+        // Otherwise success.
+        EXPECT_EQ(ParseIntegerResult::OK, rv) << "Unexpected result parsing: "
+                                              << test.input;
+        EXPECT_EQ(test.output, static_cast<int>(result));
+      }
+    }
+  }
+
+  // ------------------------------------
+  // Parse maximal value
+  // ------------------------------------
+
+  for (const auto& sign_policy : kAllSignPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result;
+    auto rv = ParseInteger(std::to_string(value), sign_policy, &result);
+    EXPECT_EQ(ParseIntegerResult::OK, rv);
+    EXPECT_EQ(value, result);
+  }
+
+  // ------------------------------------
+  // Parse one beyond maximal value (overflow)
+  // ------------------------------------
+
+  for (const auto& sign_policy : kAllSignPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    auto rv = ParseInteger(IncrementLastChar(std::to_string(value)),
+                           sign_policy, &result);
+    EXPECT_EQ(ParseIntegerResult::FAILED_OVERFLOW, rv);
+    EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
   }
+
+  // ------------------------------------
+  // Parse maximal value with trailing whitespace
+  // ------------------------------------
+
+  // base::StringToIntXXX() has weird quirks dealing with whitespace. This
+  // exercises an incorrect way to detect overflow from base::StringToInt().
+
+  for (const auto& sign_policy : kAllSignPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    auto rv = ParseInteger(std::to_string(value) + " ", sign_policy, &result);
+    EXPECT_EQ(ParseIntegerResult::FAILED_PARSE, rv);
+    EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+  }
+
+  // ------------------------------------
+  // Parse minimal value
+  // ------------------------------------
+
+  for (const auto& sign_policy : kAllSignPolicies) {
+    const T value = std::numeric_limits<T>::min();
+    T result = kFailResult;
+    auto rv = ParseInteger(std::to_string(value), sign_policy, &result);
+
+    if (std::numeric_limits<T>::is_signed &&
+        sign_policy == ParseIntegerSignPolicy::NON_NEGATIVE) {
+      EXPECT_EQ(ParseIntegerResult::FAILED_PARSE, rv);
+      EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+    } else {
+      EXPECT_EQ(ParseIntegerResult::OK, rv);
+      EXPECT_EQ(value, result);
+    }
+  }
+
+  // ------------------------------------
+  // Parse one less than minimal value
+  // ------------------------------------
+  {
+    std::string value_str;
+
+    if (std::numeric_limits<T>::is_signed) {
+      value_str =
+          IncrementLastChar(std::to_string(std::numeric_limits<T>::min()));
+    } else {
+      value_str = "-1";
+    }
+
+    T result = kFailResult;
+    auto rv = ParseInteger(value_str, ParseIntegerSignPolicy::ANY, &result);
+
+    EXPECT_EQ(ParseIntegerResult::FAILED_UNDERFLOW, rv);
+    EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+  }
+
+  // ------------------------------------
+  // Parse a value containing embedded NUL
+  // ------------------------------------
+
+  {
+    T result = kFailResult;
+    ASSERT_FALSE(
+        ParseNonNegativeInteger(base::StringPiece("123\0", 4), &result));
+    EXPECT_EQ(kFailResult, result);
+  }
+}
+
+TEST(ParseNumberTest, ParseIntegerInt32) {
+  TestParseInteger<int32_t>();
 }
 
-TEST(ParseNumberTest, IntInvalidInputsContainsNul) {
-  int result = 0xDEAD;
-  ASSERT_FALSE(
-      ParseNonNegativeDecimalInt(base::StringPiece("123\0", 4), &result));
-  EXPECT_EQ(0xDEAD, result);
+TEST(ParseNumberTest, ParseIntegerUint32) {
+  TestParseInteger<uint32_t>();
 }
 
+TEST(ParseNumberTest, ParseIntegerInt64) {
+  TestParseInteger<int64_t>();
+}
+
+TEST(ParseNumberTest, ParseIntegerUint64) {
+  TestParseInteger<uint64_t>();
+}
+
+// TODO(eroman): There are no explicit tests for ParseNonNegativeInteger().
+
 }  // namespace
 }  // namespace net