adiyansjah opened a new issue, #12097:
URL: https://github.com/apache/arrow-rs/issues/12097
**Describe the bug**
The arrow-side `DeltaByteArrayDecoder`
(`parquet/src/arrow/decoder/delta_byte_array.rs`, `read`) panics on a corrupt
`DELTA_BYTE_ARRAY` page instead of returning an error. A suffix length decoded
from the page is cast from `i32` to `usize`:
```rust
let suffix_length = *suffix_length as usize;
if self.data_offset + suffix_length > self.data.len() {
return Err(ParquetError::EOF("eof decoding byte array".into()));
}
self.last_value.truncate(prefix_length);
self.last_value
.extend_from_slice(&data[self.data_offset..self.data_offset +
suffix_length]);
```
A negative value becomes a huge `usize`, `self.data_offset + suffix_length`
wraps (release builds), the bounds check passes, and the slice panics:
```
thread 'main' panicked at
parquet/src/arrow/decoder/delta_byte_array.rs:88:41:
slice index starts at 124 but ends at 120
```
(`delta_byte_array.rs:101` in 60.1.0; `:88` on `main` at 39ae0f7c, which has
the same code.) #9796 fixed the same kind of problem for prefix lengths in
`encodings/decoding.rs`; this decoder has the same unchecked cast for both
prefix and suffix lengths (a too-large prefix is harmless because of
`truncate`, a negative suffix is not).
**To Reproduce**
Writes a 630-byte file with one non-null string column (64 synthetic values,
`DELTA_BYTE_ARRAY`, no dictionary, uncompressed), changes one byte of the data
page, and reads it back. Reproduces with 60.1.0 and with `main` at 39ae0f7c, in
`--release`.
```rust
use std::sync::Arc;
use arrow_array::{ArrayRef, RecordBatch, StringArray};
use bytes::Bytes;
use parquet::arrow::{arrow_reader::ParquetRecordBatchReaderBuilder,
ArrowWriter};
use parquet::basic::{Compression, Encoding};
use parquet::file::properties::WriterProperties;
fn main() {
let vals: Vec<String> = (0..64).map(|i| format!("value-{:04}", i *
7)).collect();
let col: ArrayRef = Arc::new(StringArray::from(vals));
let batch = RecordBatch::try_from_iter_with_nullable([("s", col,
false)]).unwrap();
let props = WriterProperties::builder()
.set_dictionary_enabled(false)
.set_encoding(Encoding::DELTA_BYTE_ARRAY)
.set_compression(Compression::UNCOMPRESSED)
.build();
let mut file = Vec::new();
let mut w = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props)).unwrap();
w.write(&batch).unwrap();
w.close().unwrap();
assert_eq!(file.len(), 630);
file[64] = 1; // one byte of the suffix-length stream of the data page
let reader =
ParquetRecordBatchReaderBuilder::try_new(Bytes::from(file)).unwrap().build().unwrap();
for batch in reader {
let _ = batch; // panics here instead of returning Err
}
}
```
**Expected behavior**
An `Err` (for example `ParquetError::General("invalid negative suffix
length")`), like the other decoders return for invalid lengths. A possible fix:
reject negative prefix and suffix lengths before the cast (or when the lengths
are decoded in `DeltaByteArrayDecoder::new`), and use `checked_add` in the
bounds check.
**Additional context**
Found by byte-flip fuzzing of valid files (the reader of a downstream
project catches the panic at a task boundary and reports an error, but a panic
on untrusted input is still a crash for any caller that does not). The same
fuzzing also hit the BitChunks panic fixed in #11691 and the `byte_range`
assertion of #6255; this one does not appear to be reported.
--
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]
