diff --git a/src/iceberg/test/decimal_test.cc b/src/iceberg/test/decimal_test.cc index 09fee1db4..8b623ee1d 100644 --- a/src/iceberg/test/decimal_test.cc +++ b/src/iceberg/test/decimal_test.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -714,6 +715,16 @@ TEST(DecimalTest, Rescale) { ASSERT_EQ(Decimal(500000), Decimal(5).Rescale(1, 6).value()); ASSERT_THAT(Decimal(5555555).Rescale(6, 1), IsError(ErrorKind::kInvalid)); + + // Scale deltas beyond kMaxScale must fail cleanly instead of indexing past + // the powers-of-ten table (previously DCHECK-only, a no-op under NDEBUG). + ASSERT_THAT(Decimal(1).Rescale(0, 100), IsError(ErrorKind::kInvalid)); + ASSERT_THAT(Decimal(1).Rescale(100, 0), IsError(ErrorKind::kInvalid)); + ASSERT_THAT(Decimal(1).Rescale(std::numeric_limits::min(), + std::numeric_limits::max()), + IsError(ErrorKind::kInvalid)); + // Zero is exact at every scale. + ASSERT_EQ(Decimal(0), Decimal(0).Rescale(0, 100).value()); } TEST(DecimalTest, Compare) { @@ -768,6 +779,19 @@ TEST(DecimalTest, Compare) { std::partial_ordering::less); ASSERT_EQ(Decimal::Compare(Decimal("-123456789"), Decimal("0"), 2, 3), std::partial_ordering::less); + + // scale deltas beyond kMaxScale must not read past the powers-of-ten table + // (previously DCHECK-only, a no-op under NDEBUG); the lower-scale operand + // dominates because aligning it always overflows. + ASSERT_EQ(Decimal::Compare(Decimal(1), Decimal(5), 100, 0), + std::partial_ordering::less); + ASSERT_EQ(Decimal::Compare(Decimal(5), Decimal(1), 0, 100), + std::partial_ordering::greater); + ASSERT_EQ(Decimal::Compare(Decimal(-1), Decimal(-5), 100, 0), + std::partial_ordering::greater); + ASSERT_EQ(Decimal::Compare(Decimal(1), Decimal(1), std::numeric_limits::max(), + std::numeric_limits::min()), + std::partial_ordering::less); } } // namespace iceberg diff --git a/src/iceberg/test/type_test.cc b/src/iceberg/test/type_test.cc index b3a6ea622..258533802 100644 --- a/src/iceberg/test/type_test.cc +++ b/src/iceberg/test/type_test.cc @@ -519,6 +519,16 @@ TEST(TypeTest, Decimal) { ASSERT_EQ(10, decimal.precision()); ASSERT_EQ(-10, decimal.scale()); } + { + iceberg::DecimalType decimal(38, 100); + ASSERT_EQ(38, decimal.precision()); + ASSERT_EQ(100, decimal.scale()); + } + { + iceberg::DecimalType decimal(38, -100); + ASSERT_EQ(38, decimal.precision()); + ASSERT_EQ(-100, decimal.scale()); + } ASSERT_THAT([]() { iceberg::DecimalType decimal(-1, 10); }, ::testing::ThrowsMessage( ::testing::HasSubstr("precision must be in [0, 38], was -1"))); diff --git a/src/iceberg/util/decimal.cc b/src/iceberg/util/decimal.cc index 0599afa4f..dbaaaeb84 100644 --- a/src/iceberg/util/decimal.cc +++ b/src/iceberg/util/decimal.cc @@ -552,12 +552,26 @@ Result Decimal::Rescale(int32_t orig_scale, int32_t new_scale) const { return *this; } - const int32_t delta_scale = new_scale - orig_scale; + // Compute the delta in 64 bits so unvalidated scales cannot cause signed + // overflow, and range-check it in release builds: kDecimal128PowersOfTen + // has only kMaxScale + 1 entries, so a DCHECK alone would be an + // out-of-bounds read under NDEBUG. + const int64_t delta_scale64 = + static_cast(new_scale) - static_cast(orig_scale); + if (delta_scale64 > kMaxScale || delta_scale64 < -kMaxScale) [[unlikely]] { + if (data_ == 0) { + // Zero is exact at every scale. + return *this; + } + // Shifting a nonzero 128-bit decimal by more than kMaxScale digits either + // overflows (scaling up) or truncates every digit (scaling down). + return Invalid("Rescale {} from {} to {} would cause data loss", ToIntegerString(), + orig_scale, new_scale); + } + const auto delta_scale = static_cast(delta_scale64); const int32_t abs_delta_scale = std::abs(delta_scale); Decimal out; - ICEBERG_DCHECK(abs_delta_scale <= kMaxScale, ""); - auto& multiplier = kDecimal128PowersOfTen[abs_delta_scale]; if (RescaleWouldCauseDataLoss(*this, delta_scale, multiplier, &out)) [[unlikely]] { @@ -590,11 +604,25 @@ std::partial_ordering Decimal::Compare(const Decimal& lhs, const Decimal& rhs, // Both are negative bool negative = lhs.data_ < 0 && rhs.data_ < 0; - const int32_t delta_scale = lhs_scale - rhs_scale; + // Compute the delta in 64 bits so unvalidated scales cannot cause signed + // overflow, and range-check it in release builds: kDecimal128PowersOfTen + // has only kMaxScale + 1 entries, so a DCHECK alone would be an + // out-of-bounds read under NDEBUG. + const int64_t delta_scale64 = + static_cast(lhs_scale) - static_cast(rhs_scale); + if (delta_scale64 > kMaxScale || delta_scale64 < -kMaxScale) [[unlikely]] { + // Aligning the lower-scale operand would multiply a nonzero 128-bit value + // by more than 10^kMaxScale, which always overflows. That is exactly the + // data-loss case below, so return the same sign-based ordering. Both + // operands are nonzero here (handled above). + if (delta_scale64 < 0) { + return negative ? std::partial_ordering::less : std::partial_ordering::greater; + } + return negative ? std::partial_ordering::greater : std::partial_ordering::less; + } + const auto delta_scale = static_cast(delta_scale64); const int32_t abs_delta_scale = std::abs(delta_scale); - ICEBERG_DCHECK(abs_delta_scale <= kMaxScale, ""); - const auto& multiplier = kDecimal128PowersOfTen[abs_delta_scale]; Decimal adjusted_lhs;