Skip to content

Commit b1bb480

Browse files
authoredJan 13, 2025
apacheGH-45180: [C++][Fuzzing] Fix Negation bug discovered by fuzzing (apache#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: apache#45180 Authored-by: Matt Topol <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
1 parent 913cb58 commit b1bb480

File tree

4 files changed

+41
-9
lines changed

4 files changed

+41
-9
lines changed
 

‎cpp/src/arrow/util/basic_decimal.cc

+16
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ static constexpr uint64_t kInt64Mask = 0xFFFFFFFFFFFFFFFF;
5050
static constexpr uint64_t kInt32Mask = 0xFFFFFFFF;
5151
#endif
5252

53+
BasicDecimal32& BasicDecimal32::Negate() {
54+
value_ = arrow::internal::SafeSignedNegate(value_);
55+
return *this;
56+
}
57+
5358
DecimalStatus BasicDecimal32::Divide(const BasicDecimal32& divisor,
5459
BasicDecimal32* result,
5560
BasicDecimal32* remainder) const {
@@ -152,6 +157,11 @@ BasicDecimal32::operator BasicDecimal64() const {
152157
return BasicDecimal64(static_cast<int64_t>(value()));
153158
}
154159

160+
BasicDecimal64& BasicDecimal64::Negate() {
161+
value_ = arrow::internal::SafeSignedNegate(value_);
162+
return *this;
163+
}
164+
155165
DecimalStatus BasicDecimal64::Divide(const BasicDecimal64& divisor,
156166
BasicDecimal64* result,
157167
BasicDecimal64* remainder) const {
@@ -253,12 +263,18 @@ const BasicDecimal64& BasicDecimal64::GetHalfScaleMultiplier(int32_t scale) {
253263
bool BasicDecimal32::FitsInPrecision(int32_t precision) const {
254264
DCHECK_GE(precision, 0);
255265
DCHECK_LE(precision, kMaxPrecision);
266+
if (value_ == INT32_MIN) {
267+
return false;
268+
}
256269
return Abs(*this) < DecimalTraits<BasicDecimal32>::powers_of_ten()[precision];
257270
}
258271

259272
bool BasicDecimal64::FitsInPrecision(int32_t precision) const {
260273
DCHECK_GE(precision, 0);
261274
DCHECK_LE(precision, kMaxPrecision);
275+
if (value_ == INT64_MIN) {
276+
return false;
277+
}
262278
return Abs(*this) < DecimalTraits<BasicDecimal64>::powers_of_ten()[precision];
263279
}
264280

‎cpp/src/arrow/util/basic_decimal.h

+2-8
Original file line numberDiff line numberDiff line change
@@ -276,10 +276,7 @@ class ARROW_EXPORT BasicDecimal32 : public SmallBasicDecimal<int32_t> {
276276
using ValueType = int32_t;
277277

278278
/// \brief Negate the current value (in-place)
279-
BasicDecimal32& Negate() {
280-
value_ = -value_;
281-
return *this;
282-
}
279+
BasicDecimal32& Negate();
283280

284281
/// \brief Absolute value (in-place)
285282
BasicDecimal32& Abs() { return *this < 0 ? Negate() : *this; }
@@ -429,10 +426,7 @@ class ARROW_EXPORT BasicDecimal64 : public SmallBasicDecimal<int64_t> {
429426
using ValueType = int64_t;
430427

431428
/// \brief Negate the current value (in-place)
432-
BasicDecimal64& Negate() {
433-
value_ = -value_;
434-
return *this;
435-
}
429+
BasicDecimal64& Negate();
436430

437431
/// \brief Absolute value (in-place)
438432
BasicDecimal64& Abs() { return *this < 0 ? Negate() : *this; }

‎cpp/src/arrow/util/decimal_test.cc

+22
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,28 @@ class DecimalFromStringTest : public ::testing::Test {
219219
}
220220
};
221221

222+
TEST(Decimal32Test, TestIntMinNegate) {
223+
Decimal32 d(INT32_MIN);
224+
auto neg = d.Negate();
225+
ASSERT_EQ(neg, Decimal32(arrow::internal::SafeSignedNegate(INT32_MIN)));
226+
}
227+
228+
TEST(Decimal32Test, TestIntMinFitsPrecision) {
229+
Decimal32 d(INT32_MIN);
230+
ASSERT_FALSE(d.FitsInPrecision(9));
231+
}
232+
233+
TEST(Decimal64Test, TestIntMinNegate) {
234+
Decimal64 d(INT64_MIN);
235+
auto neg = d.Negate();
236+
ASSERT_EQ(neg, Decimal64(arrow::internal::SafeSignedNegate(INT64_MIN)));
237+
}
238+
239+
TEST(Decimal64Test, TestIntMinFitsPrecision) {
240+
Decimal64 d(INT64_MIN);
241+
ASSERT_FALSE(d.FitsInPrecision(18));
242+
}
243+
222244
TYPED_TEST_SUITE(DecimalFromStringTest, DecimalTypes);
223245

224246
TYPED_TEST(DecimalFromStringTest, Basics) { this->TestBasics(); }

0 commit comments

Comments
 (0)
Please sign in to comment.