Merge pull request #431 from aikhanjum/codex/fastfloat-correctness

Reject integer_times_pow10 arguments wider than 64 bits

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.14)
 
 
-project(fast_float VERSION 8.3.1 LANGUAGES CXX)
+project(fast_float VERSION 8.3.2 LANGUAGES CXX)
 set(FASTFLOAT_CXX_STANDARD 11 CACHE STRING "the C++ standard to use for fastfloat")
 set(CMAKE_CXX_STANDARD ${FASTFLOAT_CXX_STANDARD})
 option(FASTFLOAT_TEST "Enable tests" OFF)

MODULE.bazel

@@ -2,7 +2,7 @@
 
 module(
     name = "fast_float",
-    version = "8.3.1",
+    version = "8.3.2",
     compatibility_level = 8,
 )
 

README.md

@@ -482,7 +482,7 @@ float: 12345678 * 10^23 = 1.23456782e+30 (==expected)
 ```
 
 Overloads of `fast_float::integer_times_pow10()` are provided for
-signed and unsigned integer types: `int64_t`, `uint64_t`, etc.
+signed and unsigned integer types up to 64 bits: `int64_t`, `uint64_t`, etc.
 
 
 ## Users and Related Work
@@ -587,7 +587,7 @@ sufficiently recent version of CMake (3.11 or better at least):
 FetchContent_Declare(
   fast_float
   GIT_REPOSITORY https://github.com/fastfloat/fast_float.git
-  GIT_TAG tags/v8.3.1
+  GIT_TAG tags/v8.3.2
   GIT_SHALLOW TRUE)
 
 FetchContent_MakeAvailable(fast_float)
@@ -603,7 +603,7 @@ You may also use [CPM](https://github.com/cpm-cmake/CPM.cmake), like so:
 CPMAddPackage(
   NAME fast_float
   GITHUB_REPOSITORY "fastfloat/fast_float"
-  GIT_TAG v8.3.1)
+  GIT_TAG v8.3.2)
 ```
 
 ## Using as single header
@@ -615,7 +615,7 @@ if desired as described in the command line help.
 
 You may directly download automatically generated single-header files:
 
-<https://github.com/fastfloat/fast_float/releases/download/v8.3.1/fast_float.h>
+<https://github.com/fastfloat/fast_float/releases/download/v8.3.2/fast_float.h>
 
 ## Benchmarking
 

fuzz/from_chars.cc

@@ -1,38 +1,205 @@
 #include "fast_float/fast_float.h"
 #include <fuzzer/FuzzedDataProvider.h>
+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
 #include <string>
 #include <system_error>
+#include <type_traits>
+#include <vector>
+
+#if defined(__has_include)
+#if __has_include(<charconv>) && __cplusplus >= 201703L
+#include <charconv>
+#define FASTFLOAT_FUZZ_CHARCONV 1
+#endif
+#endif
+
+static void fail(char const *check) {
+  fprintf(stderr, "fast_float fuzz failure: %s\n", check);
+  abort();
+}
 
 fast_float::chars_format arbitrary_format(FuzzedDataProvider &fdp) {
   using fast_float::chars_format;
-  switch (fdp.ConsumeIntegralInRange<int>(0, 3)) {
+  chars_format format;
+  switch (fdp.ConsumeIntegralInRange<int>(0, 7)) {
   case 0:
-    return chars_format::scientific;
+    format = chars_format::scientific;
     break;
   case 1:
-    return chars_format::fixed;
+    format = chars_format::fixed;
     break;
   case 2:
-    return chars_format::fixed;
+    format = chars_format::json;
+    break;
+  case 3:
+    format = chars_format::json_or_infnan;
+    break;
+  case 4:
+    format = chars_format::fortran;
+    break;
+  case 5:
+    format = chars_format::javascript;
+    break;
+  case 6:
+    format = chars_format::javascript_sloppy;
+    break;
+  default:
+    format = chars_format::general;
     break;
   }
-  return chars_format::general;
+  if (fdp.ConsumeBool()) {
+    format = format | chars_format::allow_leading_plus;
+  }
+  if (fdp.ConsumeBool()) {
+    format = format | chars_format::skip_white_space;
+  }
+  return format;
+}
+
+template <typename T> uint64_t bits_of(T value) {
+  uint64_t bits = 0;
+  memcpy(&bits, &value, sizeof(T));
+  return bits;
+}
+
+// The same ASCII text must parse identically for every character type: same
+// error code, same number of consumed code units, bit-identical value.
+template <typename UC>
+void check_widened(std::string const &input,
+                   fast_float::from_chars_result_t<char> expected,
+                   double expected_value, fast_float::chars_format format) {
+  std::vector<UC> wide;
+  wide.reserve(input.size());
+  for (char c : input) {
+    wide.push_back(UC(static_cast<unsigned char>(c)));
+  }
+  double value = 0.0;
+  auto result = fast_float::from_chars(wide.data(), wide.data() + wide.size(),
+                                       value, format);
+  if (result.ec != expected.ec ||
+      (result.ptr - wide.data()) != (expected.ptr - input.data())) {
+    fail("wide result disagrees with char result");
+  }
+  if (expected.ec == std::errc() && bits_of(value) != bits_of(expected_value)) {
+    fail("wide value disagrees with char value");
+  }
+}
+
+// Every number format accepted by the library is pure ASCII, so a successful
+// parse must never consume a code unit >= 0x80. A violation means some helper
+// inspected only the low byte of a wide code unit (the bug behind #391).
+// The generated sequences keep an ASCII low byte but take hostile high bits:
+// top-bit-set units (near the minimum of a signed wchar_t, the shape behind
+// #422), sign-extended negative units (the shape behind #395), and non-ASCII
+// code points whose low byte is an ASCII digit (the shape behind #391).
+template <typename UC>
+void check_arbitrary_units(FuzzedDataProvider &fdp,
+                           fast_float::chars_format format) {
+  using unsigned_uc = typename std::make_unsigned<UC>::type;
+  size_t count = fdp.ConsumeIntegralInRange<size_t>(0, 64);
+  std::vector<UC> units;
+  units.reserve(count);
+  for (size_t i = 0; i < count; ++i) {
+    unsigned_uc unit = fdp.ConsumeIntegral<uint8_t>();
+    switch (fdp.ConsumeIntegralInRange<int>(0, 4)) {
+    case 0: // top bit set: near the minimum value when UC is signed
+      unit = unsigned_uc(unit | (unsigned_uc(1) << (sizeof(UC) * 8 - 1)));
+      break;
+    case 1: // all high bits set: a small negative value when UC is signed
+      unit = unsigned_uc(unit | ~unsigned_uc(0xFF));
+      break;
+    case 2: // arbitrary high byte: a non-ASCII code point, ASCII low byte
+      unit = unsigned_uc(
+          unit | unsigned_uc(unsigned_uc(fdp.ConsumeIntegral<uint8_t>()) << 8));
+      break;
+    default: // plain ASCII unit
+      break;
+    }
+    units.push_back(UC(unit));
+  }
+  if (units.empty()) {
+    return;
+  }
+  UC const *begin = units.data();
+  UC const *end = begin + units.size();
+
+  double as_double = 0.0;
+  auto result = fast_float::from_chars(begin, end, as_double, format);
+  if (result.ec == std::errc()) {
+    for (UC const *p = begin; p != result.ptr; ++p) {
+      if (static_cast<unsigned_uc>(*p) >= 0x80) {
+        fail("float parse consumed a non-ASCII code unit");
+      }
+    }
+  }
+  uint16_t as_uint16 = 0;
+  auto int_result = fast_float::from_chars(begin, end, as_uint16);
+  if (int_result.ec == std::errc()) {
+    for (UC const *p = begin; p != int_result.ptr; ++p) {
+      if (static_cast<unsigned_uc>(*p) >= 0x80) {
+        fail("integer parse consumed a non-ASCII code unit");
+      }
+    }
+  }
+}
+
+// fast_float's integer parsing is specified to match std::from_chars.
+template <typename T> void check_int(std::string const &input, int base) {
+  T value{};
+  auto result = fast_float::from_chars(
+      input.data(), input.data() + input.size(), value, base);
+#ifdef FASTFLOAT_FUZZ_CHARCONV
+  T expected{};
+  auto std_result = std::from_chars(input.data(), input.data() + input.size(),
+                                    expected, base);
+  if (result.ec != std_result.ec || result.ptr != std_result.ptr) {
+    fail("integer result disagrees with std::from_chars");
+  }
+  if (result.ec == std::errc() && value != expected) {
+    fail("integer value disagrees with std::from_chars");
+  }
+#else
+  static_cast<void>(result);
+#endif
+}
+
+template <typename T>
+void check_int_all_bases(std::string const &input, int base) {
+  check_int<T>(input, 10);
+  check_int<T>(input, base);
 }
 
 extern "C" int LLVMFuzzerTestOneInput(uint8_t const *data, size_t size) {
   FuzzedDataProvider fdp(data, size);
   fast_float::chars_format format = arbitrary_format(fdp);
-  double result_d = 0.0;
+  int base = fdp.ConsumeIntegralInRange<int>(2, 36);
+
   std::string input_d = fdp.ConsumeRandomLengthString(128);
-  auto answer = fast_float::from_chars(
+  double result_d = 0.0;
+  auto answer_d = fast_float::from_chars(
       input_d.data(), input_d.data() + input_d.size(), result_d, format);
-  std::string input_f = fdp.ConsumeRandomLengthString(128);
   float result_f = 0.0;
-  answer = fast_float::from_chars(
-      input_f.data(), input_f.data() + input_f.size(), result_f, format);
-  int result_i = 0;
+  fast_float::from_chars(input_d.data(), input_d.data() + input_d.size(),
+                         result_f, format);
+  check_widened<wchar_t>(input_d, answer_d, result_d, format);
+  check_widened<char16_t>(input_d, answer_d, result_d, format);
+  check_widened<char32_t>(input_d, answer_d, result_d, format);
+
   std::string input_i = fdp.ConsumeRandomLengthString(128);
-  answer = fast_float::from_chars(input_i.data(),
-                                  input_i.data() + input_i.size(), result_i);
+  check_int_all_bases<uint8_t>(input_i, base);
+  check_int_all_bases<int8_t>(input_i, base);
+  check_int_all_bases<uint16_t>(input_i, base);
+  check_int_all_bases<int16_t>(input_i, base);
+  check_int_all_bases<uint32_t>(input_i, base);
+  check_int_all_bases<int32_t>(input_i, base);
+  check_int_all_bases<uint64_t>(input_i, base);
+  check_int_all_bases<int64_t>(input_i, base);
+
+  check_arbitrary_units<wchar_t>(fdp, format);
+  check_arbitrary_units<char16_t>(fdp, format);
+  check_arbitrary_units<char32_t>(fdp, format);
   return 0;
-}
+}

include/fast_float/ascii_number.h

@@ -32,7 +32,13 @@ template <typename UC> fastfloat_really_inline constexpr bool has_simd_opt() {
 // able to optimize it well.
 template <typename UC>
 fastfloat_really_inline constexpr bool is_integer(UC c) noexcept {
-  return static_cast<unsigned>(c - UC('0')) <= 9u;
+  // UC can be signed (wchar_t is a signed int on Linux and macOS), and the
+  // subtraction is then promoted to int and overflows for code units near the
+  // bottom of the range. Subtracting in the unsigned type wraps instead, which
+  // selects the same code units, as ch_to_digit already does.
+  using UnsignedUC = typename std::make_unsigned<UC>::type;
+  return static_cast<UnsignedUC>(static_cast<UnsignedUC>(c) -
+                                 static_cast<UnsignedUC>(UC('0'))) <= 9u;
 }
 
 fastfloat_really_inline constexpr uint64_t byteswap(uint64_t val) {

include/fast_float/decimal_to_binary.h

@@ -61,9 +61,15 @@ namespace detail {
  *   -ceil(p) + q
  * where
  *   p = log(5**-q)/log(2) = -q * log(5)/log(2)
+ *
+ * The shift must floor towards negative infinity, as an arithmetic right
+ * shift does, but right-shifting a negative value is implementation-defined
+ * before C++20. Biasing the product by 2048 * 65536 keeps the shifted value
+ * non-negative without changing the result: the bias is a multiple of 2^16
+ * and is removed again after the shift.
  */
 constexpr fastfloat_really_inline int32_t power(int32_t q) noexcept {
-  return (((152170 + 65536) * q) >> 16) + 63;
+  return (((152170 + 65536) * q + (2048 * 65536)) >> 16) - 2048 + 63;
 }
 } // namespace detail
 

include/fast_float/float_common.h

@@ -18,7 +18,7 @@
 
 #define FASTFLOAT_VERSION_MAJOR 8
 #define FASTFLOAT_VERSION_MINOR 3
-#define FASTFLOAT_VERSION_PATCH 1
+#define FASTFLOAT_VERSION_PATCH 2
 
 #define FASTFLOAT_STRINGIZE_IMPL(x) #x
 #define FASTFLOAT_STRINGIZE(x) FASTFLOAT_STRINGIZE_IMPL(x)

include/fast_float/parse_number.h

@@ -158,7 +158,10 @@ template <> struct from_chars_caller<std::float32_t> {
     // float32_t
     float val = 0.0f;
     auto ret = from_chars_advanced(first, last, val, options);
-    value = val;
+    // If no characters match the pattern, value must be left unmodified.
+    if (ret.ec != std::errc::invalid_argument) {
+      value = val;
+    }
     return ret;
   }
 };
@@ -175,7 +178,10 @@ template <> struct from_chars_caller<std::float64_t> {
     // float64_t
     double val = 0.0;
     auto ret = from_chars_advanced(first, last, val, options);
-    value = val;
+    // If no characters match the pattern, value must be left unmodified.
+    if (ret.ec != std::errc::invalid_argument) {
+      value = val;
+    }
     return ret;
   }
 };
@@ -503,7 +509,9 @@ FASTFLOAT_CONSTEXPR20
     typename std::enable_if<is_supported_float_type<T>::value, T>::type
     integer_times_pow10(int64_t mantissa, int decimal_exponent) noexcept {
   const bool is_negative = mantissa < 0;
-  const uint64_t m = static_cast<uint64_t>(is_negative ? -mantissa : mantissa);
+  // Negate as unsigned: -mantissa overflows for the most negative int64_t.
+  const uint64_t m = is_negative ? uint64_t(0) - static_cast<uint64_t>(mantissa)
+                                 : static_cast<uint64_t>(mantissa);
 
   T value;
   if (clinger_fast_path_impl(m, decimal_exponent, is_negative, value))
@@ -527,38 +535,42 @@ integer_times_pow10(int64_t mantissa, int decimal_exponent) noexcept {
 // the following overloads are here to avoid surprising ambiguity for int,
 // unsigned, etc.
 template <typename T, typename Int>
-FASTFLOAT_CONSTEXPR20
-    typename std::enable_if<is_supported_float_type<T>::value &&
-                                std::is_integral<Int>::value &&
-                                !std::is_signed<Int>::value,
-                            T>::type
-    integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
+FASTFLOAT_CONSTEXPR20 typename std::enable_if<
+    is_supported_float_type<T>::value && std::is_integral<Int>::value &&
+        sizeof(Int) <= sizeof(uint64_t) && !std::is_signed<Int>::value,
+    T>::type
+integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
   return integer_times_pow10<T>(static_cast<uint64_t>(mantissa),
                                 decimal_exponent);
 }
 
 template <typename T, typename Int>
-FASTFLOAT_CONSTEXPR20
-    typename std::enable_if<is_supported_float_type<T>::value &&
-                                std::is_integral<Int>::value &&
-                                std::is_signed<Int>::value,
-                            T>::type
-    integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
+FASTFLOAT_CONSTEXPR20 typename std::enable_if<
+    is_supported_float_type<T>::value && std::is_integral<Int>::value &&
+        sizeof(Int) <= sizeof(int64_t) && std::is_signed<Int>::value,
+    T>::type
+integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
   return integer_times_pow10<T>(static_cast<int64_t>(mantissa),
                                 decimal_exponent);
 }
 
 template <typename Int>
-FASTFLOAT_CONSTEXPR20 typename std::enable_if<
-    std::is_integral<Int>::value && !std::is_signed<Int>::value, double>::type
-integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
+FASTFLOAT_CONSTEXPR20
+    typename std::enable_if<std::is_integral<Int>::value &&
+                                sizeof(Int) <= sizeof(uint64_t) &&
+                                !std::is_signed<Int>::value,
+                            double>::type
+    integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
   return integer_times_pow10(static_cast<uint64_t>(mantissa), decimal_exponent);
 }
 
 template <typename Int>
-FASTFLOAT_CONSTEXPR20 typename std::enable_if<
-    std::is_integral<Int>::value && std::is_signed<Int>::value, double>::type
-integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
+FASTFLOAT_CONSTEXPR20
+    typename std::enable_if<std::is_integral<Int>::value &&
+                                sizeof(Int) <= sizeof(int64_t) &&
+                                std::is_signed<Int>::value,
+                            double>::type
+    integer_times_pow10(Int mantissa, int decimal_exponent) noexcept {
   return integer_times_pow10(static_cast<int64_t>(mantissa), decimal_exponent);
 }
 

tests/basictest.cpp

@@ -2283,6 +2283,38 @@ void verify_integer_times_pow10(Int mantissa, int decimal_exponent) {
 }
 } // namespace all_supported_types
 
+#if defined(__SIZEOF_INT128__)
+template <typename Int, typename = void>
+struct has_integer_times_pow10 : std::false_type {};
+
+template <typename Int>
+struct has_integer_times_pow10<
+    Int, std::void_t<decltype(fast_float::integer_times_pow10(Int{}, 0))>>
+    : std::true_type {};
+
+template <typename Float, typename Int, typename = void>
+struct has_typed_integer_times_pow10 : std::false_type {};
+
+template <typename Float, typename Int>
+struct has_typed_integer_times_pow10<
+    Float, Int,
+    std::void_t<decltype(fast_float::integer_times_pow10<Float>(Int{}, 0))>>
+    : std::true_type {};
+
+TEST_CASE("integer_times_pow10 rejects integers wider than 64 bits") {
+  CHECK(has_integer_times_pow10<int64_t>::value);
+  CHECK(has_integer_times_pow10<uint64_t>::value);
+  CHECK(has_typed_integer_times_pow10<float, int64_t>::value);
+  CHECK(has_typed_integer_times_pow10<double, uint64_t>::value);
+  CHECK_FALSE(has_integer_times_pow10<__int128_t>::value);
+  CHECK_FALSE(has_integer_times_pow10<__uint128_t>::value);
+  CHECK_FALSE(has_typed_integer_times_pow10<float, __int128_t>::value);
+  CHECK_FALSE(has_typed_integer_times_pow10<float, __uint128_t>::value);
+  CHECK_FALSE(has_typed_integer_times_pow10<double, __int128_t>::value);
+  CHECK_FALSE(has_typed_integer_times_pow10<double, __uint128_t>::value);
+}
+#endif
+
 TEST_CASE("integer_times_pow10") {
   /* explicitly verifying API with different types of integers */
   // double (the "default" overload)
@@ -2485,6 +2517,10 @@ TEST_CASE("integer_times_pow10") {
     all::verify_integer_times_pow10(12345678901234567890ull, -42);
     all::verify_integer_times_pow10(std::numeric_limits<int64_t>::max(), 42);
     all::verify_integer_times_pow10(std::numeric_limits<int64_t>::max(), -42);
+    // the most negative int64_t has no positive counterpart to negate into
+    all::verify_integer_times_pow10(std::numeric_limits<int64_t>::min(), 0);
+    all::verify_integer_times_pow10(std::numeric_limits<int64_t>::min(), 42);
+    all::verify_integer_times_pow10(std::numeric_limits<int64_t>::min(), -42);
     all::verify_integer_times_pow10(std::numeric_limits<uint64_t>::max(), 42);
     all::verify_integer_times_pow10(std::numeric_limits<uint64_t>::max(), -42);
   }

tests/fixedwidthtest.cpp

@@ -2,6 +2,8 @@
 #include <iostream>
 #include <vector>
 #include <cstring>
+#include <limits>
+#include <string_view>
 #include "fast_float/fast_float.h"
 #include <cstdint>
 
@@ -9,6 +11,39 @@
 #include <stdfloat>
 #endif
 
+// If no characters match the pattern, value must be left unmodified
+// (see https://github.com/fastfloat/fast_float/issues/424).
+template <typename T> bool check_invalid_input_preserves_value() {
+  std::vector<std::string_view> const invalid_inputs{"x",  "",   "-",  ".",
+                                                     "e1", "-x", "na", "+"};
+  for (auto const &f : invalid_inputs) {
+    T result = T(42);
+    auto answer = fast_float::from_chars(f.data(), f.data() + f.size(), result);
+    if (answer.ec != std::errc::invalid_argument || answer.ptr != f.data()) {
+      std::cerr << "Expected invalid_argument for input: \"" << f << "\""
+                << std::endl;
+      return false;
+    }
+    if (result != T(42)) {
+      std::cerr << "Value modified on invalid input: \"" << f << "\" got "
+                << result << std::endl;
+      return false;
+    }
+  }
+  // Out-of-range input must still store the documented value.
+  std::string_view const too_large = "1e999";
+  T result = T(42);
+  auto answer = fast_float::from_chars(
+      too_large.data(), too_large.data() + too_large.size(), result);
+  if (answer.ec != std::errc::result_out_of_range ||
+      result != std::numeric_limits<T>::infinity()) {
+    std::cerr << "Expected infinity and result_out_of_range for \"1e999\""
+              << std::endl;
+    return false;
+  }
+  return true;
+}
+
 int main() {
   // Write some testcases for the parsing of floating point numbers in the
   // float32_t type. We use the from_chars function defined in this library.
@@ -33,6 +68,9 @@ int main() {
       return EXIT_FAILURE;
     }
   }
+  if (!check_invalid_input_preserves_value<std::float32_t>()) {
+    return EXIT_FAILURE;
+  }
 #else
   std::cout << "No std::float32_t type available." << std::endl;
 #endif
@@ -60,6 +98,9 @@ int main() {
       return EXIT_FAILURE;
     }
   }
+  if (!check_invalid_input_preserves_value<std::float64_t>()) {
+    return EXIT_FAILURE;
+  }
 #else
   std::cout << "No std::float64_t type available." << std::endl;
 #endif

tests/wide_char_test.cpp

@@ -70,9 +70,34 @@ bool test_non_space_with_space_low_byte() {
   return true;
 }
 
+// is_integer subtracts UC('0') from the code unit. When wchar_t is signed the
+// subtraction is promoted to int and overflows for code units near the bottom
+// of the range, so the sanitized build aborts on input the library is supposed
+// to reject. Such a unit is not a digit, so both entry points must fail.
+bool test_code_unit_below_zero_digit() {
+  if (!std::is_signed<wchar_t>::value) {
+    return true; // only reproducible where wchar_t is signed
+  }
+  wchar_t const input[] = {(std::numeric_limits<wchar_t>::min)(), L'1'};
+  double result;
+  auto answer = fast_float::from_chars(input, input + 2, result);
+  if (answer.ec != std::errc::invalid_argument) {
+    std::cerr << "a code unit below L'0' must not start a number\n";
+    return false;
+  }
+  unsigned long long integer;
+  auto int_answer = fast_float::from_chars(input, input + 2, integer, 10);
+  if (int_answer.ec != std::errc::invalid_argument) {
+    std::cerr << "a code unit below L'0' must not start an integer\n";
+    return false;
+  }
+  return true;
+}
+
 int main() {
   if (test_minus() && test_plus() && test_space() && test_nan() &&
-      test_non_space_with_space_low_byte()) {
+      test_non_space_with_space_low_byte() &&
+      test_code_unit_below_zero_digit()) {
     std::cout << "all ok" << std::endl;
     return EXIT_SUCCESS;
   }