gty404 opened a new pull request, #1084:
URL: https://github.com/apache/iceberg-cpp/pull/1084
## Summary
`Decimal::Rescale` and `Decimal::Compare` index the 39-entry
`kDecimal128PowersOfTen` table using a scale delta that is only checked by
`ICEBERG_DCHECK`, which is a no-op in release (`NDEBUG`) builds. Scale is not
bounded when a `DecimalType` is constructed, so a decimal value typed with an
out-of-range scale — reachable from metadata, e.g. a schema type string such
as `decimal(9,2000000000)` parsed by the JSON type parser — can drive an
out-of-bounds read of that table.
Note: `Literal`'s cast path (`LiteralCaster::CastTo`) already validates scale
before using it, but `Literal::Compare`'s decimal branch calls
`Decimal::Compare` directly with each operand's type scale, with no
validation beforehand — so the gap is reachable independently of casting.
## Fix
- Compute the scale delta in 64 bits so an out-of-range scale cannot cause
signed `int32_t` overflow.
- Replace the DCHECK with a real range check in both `Rescale` and `Compare`:
- `Rescale` returns the existing "would cause data loss" error for
`|delta| > kMaxScale` (zero is passed through unchanged, since it's exact
at every scale).
- `Compare` returns the mathematically correct sign-based ordering,
because aligning a nonzero 128-bit operand across more than `kMaxScale`
digits always overflows — the same data-loss case the existing code
already resolves the same way.
This intentionally does not add a scale bound to `DecimalType`'s constructor:
`LiteralCaster::CastTo` already relies on being able to construct an
out-of-range-scale `DecimalType` and reject it itself with a `Result` error
(see existing tests in `literal_test.cc`), so the fix is scoped to hardening
the actual read site rather than changing that contract.
## Test plan
- Extended `DecimalTest.Rescale` with `|delta| > kMaxScale` cases, including
`INT32_MIN`/`INT32_MAX` (overflow-of-delta), plus the zero-passthrough
case.
- Extended `DecimalTest.Compare` with unequal-scale deltas of 100 and
`INT32_MAX`/`INT32_MIN`, verifying defined, sign-correct orderings.
- Verified all new assertions fail against the pre-fix code (reverting only
`decimal.cc` while keeping the new tests) and pass with the fix.
- `util_test` (421 tests), `schema_test` (578), `expression_test` (538/539,
excluding an exploratory integration test that was dropped for being
non-deterministic under UB) and `json_serde_test` (170) all pass with no
regressions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
