mardgez13 opened a new pull request, #54309:
URL: https://github.com/apache/arrow/pull/54309
### Rationale for this change
A decimal type's scale can be any `int32_t` value and the scale of a decimal
string comes from its exponent, but `IncreaseScaleBy`, `ReduceScaleBy`,
`GetWholeAndFraction` and the floating-point `PowerOfTen` index fixed-size
tables of powers of ten and only check the index with a `DCHECK`. Truncating
casts, `product`/`hash_product` and `Decimal::ToReal`/`FromReal` pass them
scale changes computed from such scales, so release builds read out of bounds
(see the issue: pyarrow 26.0.0 segfaults on a truncating cast of
`"1E-100000000"` to `decimal128(38, 0)`).
### What changes are included in this PR?
- `scalar_cast_numeric.cc`: the truncating (`allow_decimal_truncate`) casts
from decimal to integer, decimal to decimal and string to decimal compute the
scale change in 64 bits and go through two helpers, `UnsafeIncreaseScaleBy` and
`UnsafeReduceScaleBy`. Reducing the scale by more than `kMaxScale` gives zero,
which is exact since every value of the type is below 10^(kMaxScale + 1).
Increasing it by more than `kMaxScale` returns `Invalid` unless the value is
zero: a non-zero value would overflow, and `allow_decimal_truncate` is about
dropping digits, not about overflow.
- `product` and `hash_product`: a decimal type whose scale is outside `[0,
kMaxScale]` is rejected with `NotImplemented` when the kernel is initialized
(`MultiplyTraits::CheckType`). `MultiplyTraits` rescales by the type's scale,
and the multiplicative identity cannot be represented at a negative scale or a
scale above `kMaxScale`.
- `util/decimal.cc`:
- `ToReal` no longer splits the value into integer and fractional parts
when the scale is above `kMaxScale`: the integer part is always zero then, and
the split looked up `GetWholeAndFraction(scale)`.
- `FromReal` uses the approximate algorithm when the scale is above
`kMaxScale` (as it already did for negative scales), and the approximate
algorithm uses `LargePowerOfTen`, which falls back to `std::pow`, instead of
the table-only `PowerOfTen`.
- The `DCHECK`s on the scale range in `ToReal` and `FromReal` are removed
since any scale is now handled, and `LargePowerOfTen` takes an `int64_t` so
that negating a scale of `INT32_MIN` does not overflow.
`Decimal::Rescale` is handled by #50965 (GH-50964) and the conversion order
of `Decimal32` narrowing casts by #53327 (GH-53288); this PR does not change
either, it only keeps the truncating paths' table lookups in bounds.
### Are these changes tested?
Yes, with new tests for all decimal widths, at scales just past `kMaxScale`
and at `INT32_MIN`/`INT32_MAX`:
- `Cast.DecimalToIntTruncateScaleOutOfRange`,
`Cast.DecimalToDecimalTruncateScaleOutOfRange` and
`Cast.StringToDecimalTruncateScaleOutOfRange`
- `TestDecimalProductKernel.ScaleOutOfRange` and
`GroupBy.ProductDecimalScaleOutOfRange`
- `TestDecimalToReal.TestScaleOutOfRange` and
`TestDecimalFromReal.TestScaleOutOfRange`
Run against the unpatched libraries, each of them reports a
`global-buffer-overflow` or an out-of-bounds index under ASan/UBSan, or fails.
With the fix, `arrow-compute-scalar-cast-test`, `arrow-compute-aggregate-test`,
`arrow-acero-hash-aggregate-test` and the decimal tests of `arrow-utility-test`
pass in an ASan/UBSan `RelWithDebInfo` build.
### Are there any user-facing changes?
- A truncating cast that reduces the scale by more than `kMaxScale` returns
zero, and one that increases it by more than `kMaxScale` returns an error for a
non-zero value, instead of reading out of bounds.
- `product` and `hash_product` on a decimal type with a scale outside `[0,
kMaxScale]` return `NotImplemented` instead of a wrong result or a crash.
- `Decimal32/64/128/256::ToReal` and `FromReal` accept any scale.
**This PR contains a "Critical Fix".** These operations read out of bounds
of static tables, and `product` returned wrong results, for decimal types and
strings with out-of-range scales. Such scales can come from untrusted data, for
example an IPC schema, or a string column cast to decimal; pyarrow 26.0.0
segfaults on the cast above.
### Was AI used for this PR?
In accordance to the [AI generation
guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code),
please disclose below whether and how AI was used in this PR.
**PR code and description written by:**
- [ ] Human
- [x] AI
**Reviewed before submission by:**
- [ ] Human
- [x] AI
- [ ] Not reviewed
An AI coding assistant found the issue, wrote the fix, the regression test
and this description, and ran the tests; a separate AI pass reviewed the diff.
* GitHub Issue: #54307
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
