Skip to content

Commit d3bfda7

Browse files
def-claude
andauthored
repr: saturate array-cardinality product to avoid multiply overflow (#37177)
`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 in #36982 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 66475d1 commit d3bfda7

1 file changed

Lines changed: 45 additions & 2 deletions

File tree

src/repr/src/row.rs

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2559,7 +2559,17 @@ impl RowPacker<'_> {
25592559
// information.
25602560
let cardinality = match dims {
25612561
[] => 0,
2562-
dims => dims.iter().map(|d| d.length).product(),
2562+
// Saturate the product: a cardinality that overflows `usize` is
2563+
// impossibly large (no array can hold that many elements), so it can
2564+
// never equal the actual `nelements` and the check below rejects it as
2565+
// `WrongCardinality`. A plain `product()` would panic under overflow
2566+
// checks (debug/fuzz) and silently wrap in release — and a wrapped
2567+
// value could even spuriously match `nelements`, accepting a corrupt
2568+
// array (e.g. dims claiming `[2^32, 2^32]` wrap to 0 elements).
2569+
dims => dims
2570+
.iter()
2571+
.map(|d| d.length)
2572+
.fold(1usize, usize::saturating_mul),
25632573
};
25642574
if nelements != cardinality {
25652575
self.row.data.truncate(start);
@@ -2602,7 +2612,10 @@ impl RowPacker<'_> {
26022612
let mut cardinality: usize = 1;
26032613
for dim in dims {
26042614
num_dims += 1;
2605-
cardinality *= dim.length;
2615+
// Saturate: an overflowing cardinality is impossibly large and is
2616+
// rejected by the `nelements` check below. See the matching note in
2617+
// `push_array_with_unchecked`.
2618+
cardinality = cardinality.saturating_mul(dim.length);
26062619

26072620
self.row
26082621
.data
@@ -3784,6 +3797,36 @@ mod tests {
37843797
assert!(row.data.is_empty());
37853798
}
37863799

3800+
#[mz_ore::test]
3801+
fn test_array_cardinality_overflow() {
3802+
// Dimension lengths whose product overflows `usize` must be rejected as
3803+
// a `WrongCardinality` error, not panic (under overflow checks) or wrap
3804+
// (in release, which could spuriously accept a corrupt array). The
3805+
// product saturates to `usize::MAX`, which no real element count matches.
3806+
let mut row = Row::default();
3807+
let res = row.packer().try_push_array(
3808+
&[
3809+
ArrayDimension {
3810+
lower_bound: 1,
3811+
length: usize::MAX,
3812+
},
3813+
ArrayDimension {
3814+
lower_bound: 1,
3815+
length: 2,
3816+
},
3817+
],
3818+
vec![Datum::Int32(1), Datum::Int32(2)],
3819+
);
3820+
assert_eq!(
3821+
res,
3822+
Err(InvalidArrayError::WrongCardinality {
3823+
actual: 2,
3824+
expected: usize::MAX,
3825+
})
3826+
);
3827+
assert!(row.data.is_empty());
3828+
}
3829+
37873830
#[mz_ore::test]
37883831
fn test_nesting() {
37893832
let mut row = Row::default();

0 commit comments

Comments
 (0)