repr: saturate array-cardinality product to avoid multiply overflow#37177
Merged
Conversation
`RowPacker::push_array_with_unchecked` and `push_array_with_row_major` compute an array's expected cardinality as the product of its dimension lengths and compare it against the actual number of elements pushed. The product was an unchecked `usize` multiply (`dims.iter()...product()` / `cardinality *= dim.length`), so dimension lengths whose product exceeds `usize::MAX` overflowed. Under overflow checks (debug, and the cargo-fuzz build) this panics; in release it silently wraps, and a wrapped value can even spuriously match the actual element count — accepting a corrupt array (e.g. dims claiming `[2^32, 2^32]` wrap to a cardinality of 0, matching an empty element list). This is reachable from `Row::decode` over an attacker- or corruption-supplied `ProtoRow`, since the proto array dimensions are not otherwise bounded. Saturate the product to `usize::MAX` instead. An overflowing cardinality is impossibly large — no array can hold that many elements — so it never equals the real element count and the existing check rejects it as `WrongCardinality`, turning the panic/silent-wrap into a clean error on both build profiles. Found by the `repr::row_codec_roundtrip` cargo-fuzz target. Tests: adds `test_array_cardinality_overflow` to `row.rs`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
13 tasks
frankmcsherry
added a commit
that referenced
this pull request
Jun 24, 2026
Rebased the int-unification spike onto current main. The base spike commit cherry-picked cleanly; this commit re-applies the mechanical follow-on work against the new base and migrates the references that landed in main after the original branch point (#37155, #37177, #36776 count-aware-aggregates, #36984): - production: signed-sum fold + SumInt16/32 narrow (relation/func.rs), the generate_series-unoptimized literal (sql/func.rs), and the generate_series collapse (projection_pushdown.rs: ReprScalarType::Int64 -> Int, and the literal_i64 closure's int2/4/8 arms collapse to one Datum::Int arm). - test/bench/doctest: ~292 Datum::IntN -> Datum::Int/UInt sites, plus the PropDatum::Int32 narrowings and an i64-typed literal in casts.rs. Workspace builds clean under --all-targets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
frankmcsherry
added a commit
that referenced
this pull request
Jun 24, 2026
Rebased the int-unification spike onto current main. The base spike commit cherry-picked cleanly; this commit re-applies the mechanical follow-on work against the new base and migrates the references that landed in main after the original branch point (#37155, #37177, #36776 count-aware-aggregates, #36984): - production: signed-sum fold + SumInt16/32 narrow (relation/func.rs), the generate_series-unoptimized literal (sql/func.rs), and the generate_series collapse (projection_pushdown.rs: ReprScalarType::Int64 -> Int, and the literal_i64 closure's int2/4/8 arms collapse to one Datum::Int arm). - test/bench/doctest: ~292 Datum::IntN -> Datum::Int/UInt sites, plus the PropDatum::Int32 narrowings and an i64-typed literal in casts.rs. Workspace builds clean under --all-targets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RowPacker::push_array_with_uncheckedandpush_array_with_row_majorcompute an array's expected cardinality as the product of its dimension lengths and compare it against the actual number of elements pushed. The product was an uncheckedusizemultiply (dims.iter()...product()/cardinality *= dim.length), so dimension lengths whose product exceedsusize::MAXoverflowed.Under overflow checks (debug, and the cargo-fuzz build) this panics; in release it silently wraps, and a wrapped value can even spuriously match the actual element count — accepting a corrupt array (e.g. dims claiming
[2^32, 2^32]wrap to a cardinality of 0, matching an empty element list). This is reachable fromRow::decodeover an attacker- or corruption-suppliedProtoRow, since the proto array dimensions are not otherwise bounded.Saturate the product to
usize::MAXinstead. An overflowing cardinality is impossibly large — no array can hold that many elements — so it never equals the real element count and the existing check rejects it asWrongCardinality, turning the panic/silent-wrap into a clean error on both build profiles.Found by the
repr::row_codec_roundtripcargo-fuzz target in #36982