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

net/base/parse_number_unittest.cc

 // 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 <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
+// ParseIntegerBase10(). 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 TestParseIntegerBase10() {
+  ParseInteger kPolicies[] = {ParseInteger::DISALLOW_NEGATIVE,
+                              ParseInteger::ALLOW_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;
-  }
-}
-
-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& policy : kPolicies) {
+    for (const auto& test : kValidTests) {
+      T result;
+
+      EXPECT_TRUE(ParseIntegerBase10(test.input, policy, &result, nullptr))
+          << "Failed to parse: " << test.input;
+
+      EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input;
+    }
+  }
+
+  // ------------------------------------
+  // 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";
-  }
-}
-
-TEST(ParseNumberTest, IntInvalidInputsContainsNul) {
-  int result = 0xDEAD;
-  ASSERT_FALSE(
-      ParseNonNegativeDecimalInt(base::StringPiece("123\0", 4), &result));
-  EXPECT_EQ(0xDEAD, result);
-}
+  for (const auto& policy : kPolicies) {
+    for (const auto& input : kInvalidParseTests) {
+      T result = kFailResult;
+
+      ParseIntegerError error;
+      EXPECT_FALSE(ParseIntegerBase10(input, policy, &result, &error));
+      EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error)
+          << "Unexpected error 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& policy : kPolicies) {
+    for (const auto& test : kValidNegativeTests) {
+      T result = kFailResult;
+      ParseIntegerError error;
+      bool success = ParseIntegerBase10(test.input, policy, &result, &error);
+
+      // The result depends on the sign policy, and the signedness of the
+      // output type.
+      if (policy == ParseInteger::DISALLOW_NEGATIVE) {
+        // If non-negative numbers were requested, should result in parse
+        // failure (even for "-0" which is in some sense non-negative).
+        EXPECT_FALSE(success);
+        EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+        EXPECT_EQ(kFailResult, result);
+      } 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_FALSE(success);
+        EXPECT_EQ(ParseIntegerError::FAILED_UNDERFLOW, error);
+        EXPECT_EQ(kFailResult, result);
+      } else {
+        // Otherwise success.
+        EXPECT_TRUE(success);
+        EXPECT_EQ(test.output, static_cast<int>(result));
+      }
+    }
+  }
+
+  // ------------------------------------
+  // Parse maximal value
+  // ------------------------------------
+
+  for (const auto& policy : kPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result;
+    EXPECT_TRUE(
+        ParseIntegerBase10(std::to_string(value), policy, &result, nullptr));
+    EXPECT_EQ(value, result);
+  }
+
+  // ------------------------------------
+  // Parse one beyond maximal value (overflow)
+  // ------------------------------------
+
+  for (const auto& policy : kPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(IncrementLastChar(std::to_string(value)),
+                                    policy, &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_OVERFLOW, error);
+    EXPECT_EQ(kFailResult, result);
+  }
+
+  // ------------------------------------
+  // 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& policy : kPolicies) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(std::to_string(value) + " ", policy,
+                                    &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+    EXPECT_EQ(kFailResult, result);
+  }
+
+  // ------------------------------------
+  // Parse minimal value
+  // ------------------------------------
+
+  for (const auto& policy : kPolicies) {
+    const T value = std::numeric_limits<T>::min();
+    T result = kFailResult;
+    ParseIntegerError error;
+    auto success =
+        ParseIntegerBase10(std::to_string(value), policy, &result, &error);
+
+    if (std::numeric_limits<T>::is_signed &&
+        policy == ParseInteger::DISALLOW_NEGATIVE) {
+      EXPECT_FALSE(success);
+      EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+      EXPECT_EQ(kFailResult, result);
+    } else {
+      EXPECT_TRUE(success);
+      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;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(value_str, ParseInteger::ALLOW_NEGATIVE,
+                                    &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_UNDERFLOW, error);
+    EXPECT_EQ(kFailResult, result);
+  }
+
+  // ------------------------------------
+  // Parse a value containing embedded NUL
+  // ------------------------------------
+
+  for (const auto& policy : kPolicies) {
+    T result = kFailResult;
+    ParseIntegerError error;
+    ASSERT_FALSE(ParseIntegerBase10(base::StringPiece("123\0", 4), policy,
+                                    &result, &error));
+    EXPECT_EQ(kFailResult, result);
+    EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+  }
+}
+
+TEST(ParseNumberTest, ParseIntegerBase10Int32) {
+  TestParseIntegerBase10<int32_t>();
+}
+
+/*
+ * TODO(eroman):
+TEST(ParseNumberTest, ParseIntegerBase10Uint32) {
+  TestParseIntegerBase10<uint32_t>();
+}
+*/
+
+TEST(ParseNumberTest, ParseIntegerBase10Int64) {
+  TestParseIntegerBase10<int64_t>();
+}
+
+/*
+ * TODO(eroman):
+TEST(ParseNumberTest, ParseIntegerBase10Uint64) {
+  TestParseIntegerBase10<uint64_t>();
+}
+*/
 
 }  // namespace
 }  // namespace net