PlenoraETL opened a new issue, #10722:
URL: https://github.com/apache/arrow-rs/issues/10722
**Describe the bug**
`DictIndexDecoder::new` reads the RLE bit width from the first byte of a
dictionary-encoded data page's value section and passes it to `RleDecoder`
without any range check, and without checking that the slice is non-empty
(`parquet/src/arrow/decoder/dictionary_index.rs`):
```rust
pub fn new(data: Bytes, num_levels: usize, num_values: Option<usize>) ->
Result<Self> {
let bit_width = data[0]; // no bounds check, no range check
let mut decoder = RleDecoder::new(bit_width);
decoder.set_data(data.slice(1..))?;
...
}
```
The Parquet specification bounds the dictionary index bit width to `0..=32`
for
`i32` indices. A file declaring a larger value reaches
`BitReader::get_batch::<i32>`, which is documented to panic:
```rust
/// # Panics
///
/// This function panics if
/// - `num_bits` is larger than the bit-capacity of `T`
pub fn get_batch<T: FromBitpacked>(&mut self, batch: &mut [T], num_bits:
usize) -> usize {
debug_assert!(num_bits <= size_of::<T>() * 8);
```
Two distinct issues at the same line:
1. **`bit_width` is not range-checked.** With `debug_assertions` on, this is
the
`debug_assert!` above. With `debug_assertions` off but `overflow-checks`
on,
the panic simply moves a few lines down inside `get_batch`. Either way an
untrusted file panics the reader rather than returning `Err`.
2. **`data[0]` is not bounds-checked.** An empty value section panics with an
index-out-of-range before the bit width is even read.
**To Reproduce**
Any dictionary-encoded column whose data page value section begins with a
byte
greater than 32, read through the plain Arrow reader:
```rust
use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder;
let reader =
ParquetRecordBatchReaderBuilder::try_new(std::fs::File::open("crafted.parquet")?)?
.build()?;
for batch in reader {
let _ = batch?;
}
```
Observed with `debug_assertions` on:
```
thread 'main' panicked at parquet-59.1.0/src/util/bit_util.rs:697:
assertion failed: num_bits <= size_of::<T>() * 8
get_batch<i32>
DictIndexDecoder::read
ByteArrayColumnValueDecoder::read
read_records_with_reservation
```
With `debug_assertions` off and `overflow-checks` on, the same input panics a
few lines later inside `get_batch`. No unsafe code and no unusual reader
configuration are involved; it reproduces in about 25 ms and is
deterministic.
**Expected behavior**
An untrusted file should produce `Err(ParquetError)`, not a panic crossing
the
library boundary. Callers reading untrusted Parquet currently have to wrap
every
read in `catch_unwind` to preserve their own error contract.
**Suggested fix**
`DictIndexDecoder::new` already returns `Result`, so both checks fit
naturally:
```rust
let bit_width = *data.first().ok_or_else(|| {
ParquetError::General("dictionary index page is empty".into())
})?;
if bit_width > 32 {
return Err(ParquetError::General(format!(
"dictionary index bit width {bit_width} exceeds the maximum of 32"
)));
}
```
The same bound would be worth asserting in `RleDecoder::new`, so that other
callers cannot construct a decoder that is guaranteed to panic on first use.
**Additional context**
Found by coverage-guided fuzzing of a Parquet reader under AddressSanitizer
with
`overflow-checks = true`. The result is a denial of service through panic;
there
is no memory unsafety and no out-of-bounds read.
Version: `parquet` 59.1.0.
--
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]
