Commit 7f6524d
authored
fix(parquet): bound schema num_children before Vec::with_capacity (#9884)
## What
Validate `SchemaElement::num_children` in
`parquet::schema::types::schema_from_array_helper` before passing it to
`Vec::with_capacity`. Reject:
1. **Negative counts** — they wrap to a huge `usize` on the cast, and
they're nonsensical anyway (`num_children` is a count).
2. **Counts that exceed the remaining schema-array entries** — each
declared child must consume at least one entry, so a value larger than
`num_elements - index - 1` is invalid input. Without this bound the cast
feeds `Vec::with_capacity(~i32::MAX)` and `alloc::raw_vec::handle_error`
panics with `capacity overflow` on a 64-bit target.
```rust
if n < 0 {
return Err(general_err!(...));
}
let n_children = n as usize;
let remaining = num_elements.saturating_sub(index + 1);
if n_children > remaining {
return Err(general_err!(...));
}
let mut fields = Vec::with_capacity(n_children);
```
## How it was found
The `parquet/fuzz/thrift_decode` cargo-fuzz harness from #5332 /
[`fuzz/initial-harnesses`](https://github.com/masumi-ryugo/arrow-rs/tree/fuzz/initial-harnesses),
running for ~30 s on `parquet/main` HEAD with no seed corpus, produces
this 22-byte input:
```
00000000 02 00 2b 11 01 08 00 11 11 00 00 00 00 00 00 00 |..+.............|
00000010 f7 f9 11 01 a5 ff |......|
```
Backtrace before this patch:
```
thread '<unnamed>' panicked at raw_vec/mod.rs:28:5: capacity overflow
2: alloc::raw_vec::capacity_overflow
3: alloc::raw_vec::handle_error raw_vec/mod.rs:889:29
4: schema_from_array_helper schema/types.rs:1404
5: parquet_schema_from_array schema/types.rs:1304:17
6: parquet_metadata_from_bytes metadata/thrift/mod.rs:791:31
7: decode_metadata metadata/parser.rs:233:5
8: decode_metadata metadata/reader.rs:823:9
```
After this patch the same bytes return a clean `ParquetError::General`,
no panic, no allocation spike.
## Tests
Three new tests in `schema::types::tests`:
- `test_parquet_schema_from_array_rejects_negative_num_children` —
direct `parquet_schema_from_array` call with `num_children = Some(-1)`.
- `test_parquet_schema_from_array_rejects_overflowing_num_children` —
same with `num_children = Some(i32::MAX)`.
- `test_decode_metadata_oom_repro_schema_overflow_does_not_panic` —
end-to-end via `ParquetMetaDataReader::decode_metadata` using the
22-byte repro.
The existing 107 tests under `schema::` continue to pass.
## Relationship to #9883
Sibling fix, not dependent. #9883 caps the up-front allocation
**inside** the thrift parser (`read_thrift_vec`); this PR caps the
allocation **downstream** of the thrift parser, where
`SchemaElement::num_children` becomes a `Vec::with_capacity` argument.
They live on different code paths — fuzz finds inputs that hit either
one independently — and either can land first.
xref #5332 #9883 #9868 #9874
---------
Co-authored-by: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com>1 parent 5bf8912 commit 7f6524d
1 file changed
Lines changed: 19 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1401 | 1401 | | |
1402 | 1402 | | |
1403 | 1403 | | |
1404 | | - | |
| 1404 | + | |
1405 | 1405 | | |
1406 | 1406 | | |
1407 | 1407 | | |
| |||
2517 | 2517 | | |
2518 | 2518 | | |
2519 | 2519 | | |
| 2520 | + | |
| 2521 | + | |
| 2522 | + | |
| 2523 | + | |
| 2524 | + | |
| 2525 | + | |
| 2526 | + | |
| 2527 | + | |
| 2528 | + | |
| 2529 | + | |
| 2530 | + | |
| 2531 | + | |
| 2532 | + | |
| 2533 | + | |
| 2534 | + | |
| 2535 | + | |
| 2536 | + | |
| 2537 | + | |
2520 | 2538 | | |
0 commit comments