This is an automated email from the ASF dual-hosted git repository. brycemecum pushed a commit to branch maint-19.0.x in repository https://gitbox.apache.org/repos/asf/arrow.git
commit 8ae3ffb898459fbb10bb0ffdfc22efa46ec3889c Author: Matt Topol <[email protected]> AuthorDate: Mon Jan 13 04:59:51 2025 -0500 GH-45180: [C++][Fuzzing] Fix Negation bug discovered by fuzzing (#45181) ### Rationale for this change If the value for Decimal32 or Decimal64 is `INT32_MIN` or `INT64_MIN` respectively, then UBSAN reports an issue when calling Negate on them due to overflow. ### What changes are included in this PR? Have the `Negate` methods of Decimal32 and Decimal64 use `arrow::internal::SafeSignedNegate`. ### Are these changes tested? Unit tests were added for both cases which were able to reproduce the problem when UBSAN was on without the fix. ### Are there any user-facing changes? No. * OSS-Fuzz issue: https://issues.oss-fuzz.com/issues/371239168 * GitHub Issue: #45180 Authored-by: Matt Topol <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]> --- cpp/src/arrow/util/basic_decimal.cc | 16 ++++++++++++++++ cpp/src/arrow/util/basic_decimal.h | 10 ++-------- cpp/src/arrow/util/decimal_test.cc | 22 ++++++++++++++++++++++ testing | 2 +- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/cpp/src/arrow/util/basic_decimal.cc b/cpp/src/arrow/util/basic_decimal.cc index 22db1e7051..e82078055f 100644 --- a/cpp/src/arrow/util/basic_decimal.cc +++ b/cpp/src/arrow/util/basic_decimal.cc @@ -50,6 +50,11 @@ static constexpr uint64_t kInt64Mask = 0xFFFFFFFFFFFFFFFF; static constexpr uint64_t kInt32Mask = 0xFFFFFFFF; #endif +BasicDecimal32& BasicDecimal32::Negate() { + value_ = arrow::internal::SafeSignedNegate(value_); + return *this; +} + DecimalStatus BasicDecimal32::Divide(const BasicDecimal32& divisor, BasicDecimal32* result, BasicDecimal32* remainder) const { @@ -152,6 +157,11 @@ BasicDecimal32::operator BasicDecimal64() const { return BasicDecimal64(static_cast<int64_t>(value())); } +BasicDecimal64& BasicDecimal64::Negate() { + value_ = arrow::internal::SafeSignedNegate(value_); + return *this; +} + DecimalStatus BasicDecimal64::Divide(const BasicDecimal64& divisor, BasicDecimal64* result, BasicDecimal64* remainder) const { @@ -253,12 +263,18 @@ const BasicDecimal64& BasicDecimal64::GetHalfScaleMultiplier(int32_t scale) { bool BasicDecimal32::FitsInPrecision(int32_t precision) const { DCHECK_GE(precision, 0); DCHECK_LE(precision, kMaxPrecision); + if (value_ == INT32_MIN) { + return false; + } return Abs(*this) < DecimalTraits<BasicDecimal32>::powers_of_ten()[precision]; } bool BasicDecimal64::FitsInPrecision(int32_t precision) const { DCHECK_GE(precision, 0); DCHECK_LE(precision, kMaxPrecision); + if (value_ == INT64_MIN) { + return false; + } return Abs(*this) < DecimalTraits<BasicDecimal64>::powers_of_ten()[precision]; } diff --git a/cpp/src/arrow/util/basic_decimal.h b/cpp/src/arrow/util/basic_decimal.h index b5404bb7bc..638c4870f1 100644 --- a/cpp/src/arrow/util/basic_decimal.h +++ b/cpp/src/arrow/util/basic_decimal.h @@ -276,10 +276,7 @@ class ARROW_EXPORT BasicDecimal32 : public SmallBasicDecimal<int32_t> { using ValueType = int32_t; /// \brief Negate the current value (in-place) - BasicDecimal32& Negate() { - value_ = -value_; - return *this; - } + BasicDecimal32& Negate(); /// \brief Absolute value (in-place) BasicDecimal32& Abs() { return *this < 0 ? Negate() : *this; } @@ -429,10 +426,7 @@ class ARROW_EXPORT BasicDecimal64 : public SmallBasicDecimal<int64_t> { using ValueType = int64_t; /// \brief Negate the current value (in-place) - BasicDecimal64& Negate() { - value_ = -value_; - return *this; - } + BasicDecimal64& Negate(); /// \brief Absolute value (in-place) BasicDecimal64& Abs() { return *this < 0 ? Negate() : *this; } diff --git a/cpp/src/arrow/util/decimal_test.cc b/cpp/src/arrow/util/decimal_test.cc index d2f8ae3b7a..e0aa0b2b85 100644 --- a/cpp/src/arrow/util/decimal_test.cc +++ b/cpp/src/arrow/util/decimal_test.cc @@ -219,6 +219,28 @@ class DecimalFromStringTest : public ::testing::Test { } }; +TEST(Decimal32Test, TestIntMinNegate) { + Decimal32 d(INT32_MIN); + auto neg = d.Negate(); + ASSERT_EQ(neg, Decimal32(arrow::internal::SafeSignedNegate(INT32_MIN))); +} + +TEST(Decimal32Test, TestIntMinFitsPrecision) { + Decimal32 d(INT32_MIN); + ASSERT_FALSE(d.FitsInPrecision(9)); +} + +TEST(Decimal64Test, TestIntMinNegate) { + Decimal64 d(INT64_MIN); + auto neg = d.Negate(); + ASSERT_EQ(neg, Decimal64(arrow::internal::SafeSignedNegate(INT64_MIN))); +} + +TEST(Decimal64Test, TestIntMinFitsPrecision) { + Decimal64 d(INT64_MIN); + ASSERT_FALSE(d.FitsInPrecision(18)); +} + TYPED_TEST_SUITE(DecimalFromStringTest, DecimalTypes); TYPED_TEST(DecimalFromStringTest, Basics) { this->TestBasics(); } diff --git a/testing b/testing index 4d209492d5..d2a1371230 160000 --- a/testing +++ b/testing @@ -1 +1 @@ -Subproject commit 4d209492d514c2d3cb2d392681b9aa00e6d8da1c +Subproject commit d2a13712303498963395318a4eb42872e66aead7
