viirya opened a new issue, #12178:
URL: https://github.com/apache/arrow-rs/issues/12178
### Describe the bug
`ByteArrayDecoderPlain` (`parquet/src/arrow/array_reader/byte_array.rs`) can
panic on corrupt PLAIN `BYTE_ARRAY` data instead of returning an error. Both
cases can be reached through `ParquetRecordBatchReader`.
1. `skip` doesn't check value lengths against the page. It advances
`self.offset` by `4 + len` and returns `Ok`, even when that goes past the end
of the buffer. The next `read` on the page then computes `self.buf.len() -
self.offset`. That overflows: debug builds panic with "attempt to subtract with
overflow", and release builds wrap and then panic with "capacity overflow" in
`reserve`. `read` already returns `ParquetError::EOF("eof decoding byte
array")` for the same input.
2. `read` divides by `max_remaining_values` to estimate how many bytes to
reserve. On a DataPage v2 whose header says every value is null (`num_nulls ==
num_values`), `max_remaining_values` is 0. If the definition levels still mark
a value as non-null and the value buffer isn't empty, `read` panics with
"attempt to divide by zero". A dictionary page with `num_values = 0` and a
non-empty buffer reaches the same division through `set_dict`.
### To Reproduce
Case 1, `skip` past the end of the page:
- Write a required `Utf8` column `["first", "second", "third"]` with
`ArrowWriter`, using PLAIN encoding and no dictionary.
- Change the 4-byte length prefix of `"first"` from `5` to `0xFF05`.
- Read it with `ParquetRecordBatchReaderBuilder`, using a `RowSelection` of
`skip(1), select(2)` and `RowSelectionPolicy::Selectors`.
Case 2, all-null v2 page with a value:
- Write a nullable `Utf8` column `[Some("a"), None]` with
`WriterVersion::PARQUET_2_0` and PLAIN encoding.
- Change `num_nulls` in the `DataPageHeaderV2` from 1 to 2.
- Read the file.
Directly against the decoder:
```rust
// Case 1
let mut d = ByteArrayDecoderPlain::new(Bytes::from_static(&[0xFF, 0xFF, 0,
0, b'a']), 2, Some(2), false);
assert_eq!(d.skip(1).unwrap(), 1); // offset is now 65539, past the 5-byte
page
d.read(&mut OffsetBuffer::<i32>::with_capacity(0), 1); // panics
// Case 2
let mut d = ByteArrayDecoderPlain::new(Bytes::from_static(&[1, 0, 0, 0,
b'a']), 0, Some(0), false);
d.read(&mut OffsetBuffer::<i32>::with_capacity(0), 1); // panics: divide by
zero
```
### Expected behavior
Both cases return a `ParquetError` instead of panicking.
### Additional context
This belongs with the other "return an error instead of a panic on a corrupt
Parquet file" fixes, e.g. #12108 for `DELTA_BYTE_ARRAY`.
`ByteViewArrayDecoderPlain::skip` has the same unchecked skip. It doesn't
panic, because its `read` checks `start_offset > buf_len`, but on a corrupt
page `skip` still returns `Ok`.
--
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]
