Subodh-17 opened a new issue, #12200:
URL: https://github.com/apache/arrow-rs/issues/12200
### Describe the bug
In `parquet/src/arrow/array_reader/byte_view_array.rs:367`,
`ByteViewArrayDecoderPlain::read_impl` decodes PLAIN-encoded `BYTE_ARRAY`
records by reading a 4-byte little-endian length prefix and adding it to
`start_offset`:
```rust
let start_offset = end_offset + 4;
...
end_offset = start_offset + len as usize;
if end_offset > buf_len {
return Err(ParquetError::EOF("eof decoding byte array".into()));
}
...
unsafe {
buf.get_unchecked(start_offset..end_offset)
}
```
While PR #12179 (commit `23ce7c3ab35e`) recently hardened
`ByteArrayDecoderPlain::skip` and `ByteViewArrayDecoderPlain::skip` against
corrupt 32-bit length prefixes using `.checked_add(len as usize).filter(...)`,
sibling method `ByteViewArrayDecoderPlain::read_impl` line 367 was not updated
and still performs unchecked addition.
On 32-bit architectures (`wasm32-unknown-unknown`, `wasm32-wasip1`,
`i686`—which arrow-rs officially tests in CI under
`.github/workflows/parquet.yml`):
1. `usize` is 32 bits (`u32`).
2. When decoding a corrupt or malicious Parquet page where `len =
0xFFFF_FFFC` (4,294,967,292) and `start_offset = 4`:
$$4 + 4,294,967,292 = 4,294,967,296 \equiv 0 \pmod{2^{32}}$$
3. In release mode (`-O`), two's complement wrapping arithmetic wraps
`end_offset` to `0`.
4. The subsequent check `if end_offset > buf_len` evaluates `0 > buf_len`,
which is `false` for any non-empty buffer (e.g. `buf_len = 100`).
5. Execution proceeds directly to line 406: `unsafe {
buf.get_unchecked(start_offset..end_offset) }` with slice range `4..0`!
6. Passing an inverted range ($start > end$) to `slice::get_unchecked`
violates its documented safety invariants, invoking **Undefined Behavior** (or
an arithmetic overflow panic in debug mode).
### Environment and Versions
- **Repository:** `apache/arrow-rs`
- **Component:** `parquet/src/arrow/array_reader/byte_view_array.rs`
- **Target Architectures:** `wasm32-unknown-unknown`, `wasm32-wasip1`,
`i686` (32-bit platforms)
- **Arrow-rs Version:** `main` (commit
`23ce7c3ab35ef6e57c69de619cfc073ef72de313` and earlier)
### Minimal Reproduction
A minimal standalone test compiling for `wasm32-unknown-unknown` under
release mode (`rustc --target wasm32-unknown-unknown -O`):
```rust
pub fn test_read_impl_bounds(buf_len: usize, start_offset: usize, len: u32)
-> u32 {
let end_offset = start_offset + len as usize;
if end_offset > buf_len {
return 1; // Handled as EOF
}
if start_offset > end_offset {
return 2; // Guard bypassed! Inverted bounds (Undefined Behavior
precondition violation)
}
0
}
fn main() {
// start_offset = 4, len = 0xFFFF_FFFC, buf_len = 100
let res = test_read_impl_bounds(100, 4, 0xFFFF_FFFC);
assert_eq!(res, 2, "Guard was bypassed due to 32-bit unsigned wrapping");
}
```
When executed under Node / wasm runtime, `res` evaluates to `2`, proving
that the length check is completely bypassed on 32-bit platforms.
### Expected Behavior
Corrupt length prefixes should be safely rejected with
`Err(ParquetError::EOF("eof decoding byte array".into()))` on all target
platforms, without wrapping arithmetic or reaching `buf.get_unchecked(...)`.
### Suggested Fix
Bring `read_impl` into exact parity with `skip()` in line 442 by using
checked arithmetic:
```rust
end_offset = start_offset
.checked_add(len as usize)
.filter(|&end| end <= buf_len)
.ok_or_else(|| ParquetError::EOF("eof decoding byte array".into()))?;
```
--
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]
