This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new b1bb4808d2 GH-45180: [C++][Fuzzing] Fix Negation bug discovered by
fuzzing (#45181)
b1bb4808d2 is described below
commit b1bb4808d2a9e7d136c8a10bf544d67f3b906fb6
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
