From 2764a0bce447b161bb7953cdb6e1d55454d5cf28 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Sun, 6 Sep 2026 10:46:06 -0400 Subject: [PATCH 1/4] GH-49817: [C++] Detect decimal parsing overflow Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: 1fanwang <1fannnw@gmail.com> --- cpp/src/arrow/util/decimal.cc | 37 ++++++++++++++++++++++++------ cpp/src/arrow/util/decimal_test.cc | 16 +++++++++---- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index a9d2fcb02d94..39ea6755b97c 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -768,7 +768,8 @@ std::string Decimal128::ToString(int32_t scale) const { // Iterates over input and for each group of kInt64DecimalDigits multiple out by // the appropriate power of 10 necessary to add source parsed as uint64 and // then adds the parsed value of source. -static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t out_size) { +static inline bool ShiftAndAddWithOverflow(std::string_view input, uint64_t out[], + size_t out_size) { for (size_t posn = 0; posn < input.size();) { const size_t group_size = std::min(kInt64DecimalDigits, input.size() - posn); const uint64_t multiple = kUInt64PowersOfTen[group_size]; @@ -783,8 +784,25 @@ static inline void ShiftAndAdd(std::string_view input, uint64_t out[], size_t ou out[i] = static_cast(tmp & 0xFFFFFFFFFFFFFFFFULL); chunk = static_cast(tmp >> 64); } + if (chunk != 0) { + return true; + } posn += group_size; } + return false; +} + +static inline bool MagnitudeOverflowsSignedDecimal(const uint64_t out[], size_t out_size, + bool negative) { + constexpr uint64_t kSignBit = uint64_t{1} << 63; + const uint64_t high = out[out_size - 1]; + if (high < kSignBit) { + return false; + } + if (!negative || high > kSignBit) { + return true; + } + return std::any_of(out, out + out_size - 1, [](uint64_t word) { return word != 0; }); } namespace { @@ -895,9 +913,14 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out if (out != nullptr) { static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64"); std::array little_endian_array{}; - ShiftAndAdd(dec.whole_digits, little_endian_array.data(), little_endian_array.size()); - ShiftAndAdd(dec.fractional_digits, little_endian_array.data(), - little_endian_array.size()); + if (ShiftAndAddWithOverflow(dec.whole_digits, little_endian_array.data(), + little_endian_array.size()) || + ShiftAndAddWithOverflow(dec.fractional_digits, little_endian_array.data(), + little_endian_array.size()) || + MagnitudeOverflowsSignedDecimal(little_endian_array.data(), + little_endian_array.size(), dec.sign == '-')) { + return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); + } *out = Decimal(bit_util::little_endian::ToNative(little_endian_array)); if (dec.sign == '-') { out->Negate(); @@ -962,9 +985,9 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s, if (out != nullptr) { uint64_t value{0}; - ShiftAndAdd(dec.whole_digits, &value, 1); - ShiftAndAdd(dec.fractional_digits, &value, 1); - if (value > static_cast( + if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1) || + ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1) || + value > static_cast( std::numeric_limits::max())) { return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); } diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 7022c8117802..c667daf97591 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -435,15 +435,19 @@ TEST(Decimal128Test, FromStringLimits) { ASSERT_RAISES(Invalid, Decimal128::FromString("-9e39")); ASSERT_RAISES(Invalid, Decimal128::FromString("9.9e40")); ASSERT_RAISES(Invalid, Decimal128::FromString("-9.9e40")); - // XXX conversion overflows are currently not detected + // XXX conversion overflows after parsing are currently not detected // ASSERT_RAISES(Invalid, Decimal128::FromString("99e38")); // ASSERT_RAISES(Invalid, Decimal128::FromString("-99e38")); // ASSERT_RAISES(Invalid, // Decimal128::FromString("999999999999999999999999999999999999999e1")); // ASSERT_RAISES(Invalid, // Decimal128::FromString("-999999999999999999999999999999999999999e1")); - // ASSERT_RAISES(Invalid, - // Decimal128::FromString("999999999999999999999999999999999999999")); + ASSERT_RAISES(Invalid, Decimal128::FromString( + "1.55555555555555555555555555555555555555555555555555")); + ASSERT_RAISES(Invalid, + Decimal128::FromString("170141183460469231731687303715884105728")); + ASSERT_RAISES(Invalid, + Decimal128::FromString("-170141183460469231731687303715884105729")); // No exponent, many fractional digits AssertDecimalFromString("9.9999999999999999999999999999999999999", dec38times9pos, 38, @@ -541,7 +545,8 @@ TEST(Decimal256Test, FromStringLimits) { ASSERT_RAISES(Invalid, Decimal256::FromString("9.9e78")); ASSERT_RAISES(Invalid, Decimal256::FromString("-9.9e78")); - // XXX conversion overflows are currently not detected + // XXX precision limits and conversion overflows after parsing are currently not + // detected // ASSERT_RAISES(Invalid, Decimal256::FromString("99e76")); // ASSERT_RAISES(Invalid, Decimal256::FromString("-99e76")); // ASSERT_RAISES(Invalid, @@ -550,6 +555,9 @@ TEST(Decimal256Test, FromStringLimits) { // Decimal256::FromString("-9999999999999999999999999999999999999999999999999999999999999999999999999999e1")); // ASSERT_RAISES(Invalid, // Decimal256::FromString("99999999999999999999999999999999999999999999999999999999999999999999999999999")); + ASSERT_RAISES(Invalid, Decimal256::FromString(std::string(78, '9'))); + ASSERT_RAISES(Invalid, Decimal256::FromString("5789604461865809771178549250434395392663" + "4992332820282019728792003956564819968")); // No exponent, many fractional digits AssertDecimalFromString( From 0d5bf6d4bc2ffb22ad4f37783fdb6bb174da8e70 Mon Sep 17 00:00:00 2001 From: Stefan Wang <1fannnw@gmail.com> Date: Mon, 7 Sep 2026 13:24:15 -0700 Subject: [PATCH 2/4] GH-49817: [C++] Handle signed decimal overflow Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com> --- cpp/src/arrow/json/converter_test.cc | 12 ++++++++--- cpp/src/arrow/util/decimal.cc | 32 +++++++++++----------------- cpp/src/arrow/util/decimal_test.cc | 8 +++++++ 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/cpp/src/arrow/json/converter_test.cc b/cpp/src/arrow/json/converter_test.cc index fa85e704bc5e..90828639849a 100644 --- a/cpp/src/arrow/json/converter_test.cc +++ b/cpp/src/arrow/json/converter_test.cc @@ -254,9 +254,15 @@ TEST(ConverterTest, Decimal128And256PrecisionError) { std::shared_ptr parse_array; ASSERT_OK(ParseFromString(options, json_source, &parse_array)); - std::string error_msg = - "Invalid: Failed to convert JSON to " + decimal_type->ToString() + - ": 123456789012345678901234567890.0123456789 requires precision 40"; + std::string error_msg; + if (decimal_type->id() == Type::DECIMAL128) { + error_msg = + "Invalid: The string '123456789012345678901234567890.0123456789' " + "cannot be represented as decimal128"; + } else { + error_msg = "Invalid: Failed to convert JSON to " + decimal_type->ToString() + + ": 123456789012345678901234567890.0123456789 requires precision 40"; + } EXPECT_RAISES_WITH_MESSAGE_THAT( Invalid, ::testing::HasSubstr(error_msg), Convert(decimal_type, parse_array->GetFieldByName(""))); diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index 39ea6755b97c..112a20eaac9b 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -769,7 +769,8 @@ std::string Decimal128::ToString(int32_t scale) const { // the appropriate power of 10 necessary to add source parsed as uint64 and // then adds the parsed value of source. static inline bool ShiftAndAddWithOverflow(std::string_view input, uint64_t out[], - size_t out_size) { + size_t out_size, bool negative) { + constexpr uint64_t kSignBit = uint64_t{1} << 63; for (size_t posn = 0; posn < input.size();) { const size_t group_size = std::min(kInt64DecimalDigits, input.size() - posn); const uint64_t multiple = kUInt64PowersOfTen[group_size]; @@ -787,24 +788,17 @@ static inline bool ShiftAndAddWithOverflow(std::string_view input, uint64_t out[ if (chunk != 0) { return true; } + const uint64_t high = out[out_size - 1]; + if ((high & kSignBit) != 0 && + (!negative || high != kSignBit || + std::any_of(out, out + out_size - 1, [](uint64_t word) { return word != 0; }))) { + return true; + } posn += group_size; } return false; } -static inline bool MagnitudeOverflowsSignedDecimal(const uint64_t out[], size_t out_size, - bool negative) { - constexpr uint64_t kSignBit = uint64_t{1} << 63; - const uint64_t high = out[out_size - 1]; - if (high < kSignBit) { - return false; - } - if (!negative || high > kSignBit) { - return true; - } - return std::any_of(out, out + out_size - 1, [](uint64_t word) { return word != 0; }); -} - namespace { struct DecimalComponents { @@ -914,11 +908,9 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64"); std::array little_endian_array{}; if (ShiftAndAddWithOverflow(dec.whole_digits, little_endian_array.data(), - little_endian_array.size()) || + little_endian_array.size(), dec.sign == '-') || ShiftAndAddWithOverflow(dec.fractional_digits, little_endian_array.data(), - little_endian_array.size()) || - MagnitudeOverflowsSignedDecimal(little_endian_array.data(), - little_endian_array.size(), dec.sign == '-')) { + little_endian_array.size(), dec.sign == '-')) { return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); } *out = Decimal(bit_util::little_endian::ToNative(little_endian_array)); @@ -985,8 +977,8 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s, if (out != nullptr) { uint64_t value{0}; - if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1) || - ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1) || + if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1, dec.sign == '-') || + ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1, dec.sign == '-') || value > static_cast( std::numeric_limits::max())) { return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index c667daf97591..8643a22b4acb 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -444,6 +444,8 @@ TEST(Decimal128Test, FromStringLimits) { // Decimal128::FromString("-999999999999999999999999999999999999999e1")); ASSERT_RAISES(Invalid, Decimal128::FromString( "1.55555555555555555555555555555555555555555555555555")); + AssertDecimalFromString("-170141183460469231731687303715884105728", + Decimal128FromLE({0, uint64_t{1} << 63}), 39, 0); ASSERT_RAISES(Invalid, Decimal128::FromString("170141183460469231731687303715884105728")); ASSERT_RAISES(Invalid, @@ -556,8 +558,14 @@ TEST(Decimal256Test, FromStringLimits) { // ASSERT_RAISES(Invalid, // Decimal256::FromString("99999999999999999999999999999999999999999999999999999999999999999999999999999")); ASSERT_RAISES(Invalid, Decimal256::FromString(std::string(78, '9'))); + AssertDecimalFromString( + "-57896044618658097711785492504343953926634992332820282019728792003956564819968", + Decimal256FromLE({0, 0, 0, uint64_t{1} << 63}), 77, 0); ASSERT_RAISES(Invalid, Decimal256::FromString("5789604461865809771178549250434395392663" "4992332820282019728792003956564819968")); + ASSERT_RAISES(Invalid, + Decimal256::FromString("-5789604461865809771178549250434395392663" + "4992332820282019728792003956564819969")); // No exponent, many fractional digits AssertDecimalFromString( From aa6fc1e361b3c618e8f2ed275ee9ad47b341ad19 Mon Sep 17 00:00:00 2001 From: Stefan Wang <1fannnw@gmail.com> Date: Mon, 7 Sep 2026 13:34:49 -0700 Subject: [PATCH 3/4] GH-49817: [C++] Allow minimum Decimal32 and Decimal64 Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com> --- cpp/src/arrow/util/decimal.cc | 3 ++- cpp/src/arrow/util/decimal_test.cc | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index 112a20eaac9b..c938e66fb0e6 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -980,7 +980,8 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s, if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1, dec.sign == '-') || ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1, dec.sign == '-') || value > static_cast( - std::numeric_limits::max())) { + std::numeric_limits::max()) + + static_cast(dec.sign == '-')) { return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); } diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 8643a22b4acb..82f63338bb65 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -230,6 +230,11 @@ TEST(Decimal32Test, TestIntMinFitsPrecision) { ASSERT_FALSE(d.FitsInPrecision(9)); } +TEST(Decimal32Test, FromStringLimits) { + AssertDecimalFromString("-2147483648", Decimal32(INT32_MIN), 10, 0); + ASSERT_RAISES(Invalid, Decimal32::FromString("-2147483649")); +} + TEST(Decimal64Test, TestIntMinNegate) { Decimal64 d(INT64_MIN); auto neg = d.Negate(); @@ -241,6 +246,11 @@ TEST(Decimal64Test, TestIntMinFitsPrecision) { ASSERT_FALSE(d.FitsInPrecision(18)); } +TEST(Decimal64Test, FromStringLimits) { + AssertDecimalFromString("-9223372036854775808", Decimal64(INT64_MIN), 19, 0); + ASSERT_RAISES(Invalid, Decimal64::FromString("-9223372036854775809")); +} + TYPED_TEST_SUITE(DecimalFromStringTest, DecimalTypes); TYPED_TEST(DecimalFromStringTest, Basics) { this->TestBasics(); } From 4e1279bfd6336701ff9f1ffc23fe96c5d9bccd45 Mon Sep 17 00:00:00 2001 From: Stefan Wang <1fannnw@gmail.com> Date: Mon, 7 Sep 2026 16:44:39 -0700 Subject: [PATCH 4/4] GH-49817: [C++] Validate metadata-only decimal parsing Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: Stefan Wang <1fannnw@gmail.com> --- cpp/src/arrow/util/decimal.cc | 33 +++++++++++++++--------------- cpp/src/arrow/util/decimal_test.cc | 15 ++++++++++++++ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc index c938e66fb0e6..0956e8d22b00 100644 --- a/cpp/src/arrow/util/decimal.cc +++ b/cpp/src/arrow/util/decimal.cc @@ -904,15 +904,15 @@ Status DecimalFromString(const char* type_name, std::string_view s, Decimal* out parsed_scale = static_cast(dec.fractional_digits.size()); } + static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64"); + std::array little_endian_array{}; + if (ShiftAndAddWithOverflow(dec.whole_digits, little_endian_array.data(), + little_endian_array.size(), dec.sign == '-') || + ShiftAndAddWithOverflow(dec.fractional_digits, little_endian_array.data(), + little_endian_array.size(), dec.sign == '-')) { + return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); + } if (out != nullptr) { - static_assert(Decimal::kBitWidth % 64 == 0, "decimal bit-width not a multiple of 64"); - std::array little_endian_array{}; - if (ShiftAndAddWithOverflow(dec.whole_digits, little_endian_array.data(), - little_endian_array.size(), dec.sign == '-') || - ShiftAndAddWithOverflow(dec.fractional_digits, little_endian_array.data(), - little_endian_array.size(), dec.sign == '-')) { - return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); - } *out = Decimal(bit_util::little_endian::ToNative(little_endian_array)); if (dec.sign == '-') { out->Negate(); @@ -975,16 +975,15 @@ Status SimpleDecimalFromString(const char* type_name, std::string_view s, parsed_scale = static_cast(dec.fractional_digits.size()); } + uint64_t value{0}; + if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1, dec.sign == '-') || + ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1, dec.sign == '-') || + value > static_cast( + std::numeric_limits::max()) + + static_cast(dec.sign == '-')) { + return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); + } if (out != nullptr) { - uint64_t value{0}; - if (ShiftAndAddWithOverflow(dec.whole_digits, &value, 1, dec.sign == '-') || - ShiftAndAddWithOverflow(dec.fractional_digits, &value, 1, dec.sign == '-') || - value > static_cast( - std::numeric_limits::max()) + - static_cast(dec.sign == '-')) { - return Status::Invalid("The string '", s, "' cannot be represented as ", type_name); - } - *out = DecimalClass(value); if (dec.sign == '-') { out->Negate(); diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index 82f63338bb65..dab035288772 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -233,6 +233,9 @@ TEST(Decimal32Test, TestIntMinFitsPrecision) { TEST(Decimal32Test, FromStringLimits) { AssertDecimalFromString("-2147483648", Decimal32(INT32_MIN), 10, 0); ASSERT_RAISES(Invalid, Decimal32::FromString("-2147483649")); + int32_t precision, scale; + ASSERT_RAISES(Invalid, + Decimal32::FromString("-2147483649", nullptr, &precision, &scale)); } TEST(Decimal64Test, TestIntMinNegate) { @@ -249,6 +252,9 @@ TEST(Decimal64Test, TestIntMinFitsPrecision) { TEST(Decimal64Test, FromStringLimits) { AssertDecimalFromString("-9223372036854775808", Decimal64(INT64_MIN), 19, 0); ASSERT_RAISES(Invalid, Decimal64::FromString("-9223372036854775809")); + int32_t precision, scale; + ASSERT_RAISES(Invalid, Decimal64::FromString("-9223372036854775809", nullptr, + &precision, &scale)); } TYPED_TEST_SUITE(DecimalFromStringTest, DecimalTypes); @@ -460,6 +466,10 @@ TEST(Decimal128Test, FromStringLimits) { Decimal128::FromString("170141183460469231731687303715884105728")); ASSERT_RAISES(Invalid, Decimal128::FromString("-170141183460469231731687303715884105729")); + int32_t precision, scale; + ASSERT_RAISES( + Invalid, Decimal128::FromString("-170141183460469231731687303715884105729", nullptr, + &precision, &scale)); // No exponent, many fractional digits AssertDecimalFromString("9.9999999999999999999999999999999999999", dec38times9pos, 38, @@ -576,6 +586,11 @@ TEST(Decimal256Test, FromStringLimits) { ASSERT_RAISES(Invalid, Decimal256::FromString("-5789604461865809771178549250434395392663" "4992332820282019728792003956564819969")); + int32_t precision, scale; + ASSERT_RAISES(Invalid, + Decimal256::FromString("-5789604461865809771178549250434395392663" + "4992332820282019728792003956564819969", + nullptr, &precision, &scale)); // No exponent, many fractional digits AssertDecimalFromString(