nlebovits opened a new issue, #12099:
URL: https://github.com/apache/arrow-rs/issues/12099
### Describe the bug
`ThriftCompactInputProtocol::read_vlq` (parquet/src/parquet_thrift.rs) loops
until it sees a byte without the continuation bit. It does not stop after 10
bytes, and it shifts with `wrapping_shl`. For the 11th byte the shift is 70,
which wraps to 6, so the byte is ORed into the low bits of the result. A
malformed footer is therefore decoded to a different value instead of being
rejected. The Thrift compact protocol limits a varint to 10 bytes for 64-bit
values.
Found while fuzzing a downstream GeoParquet reader. One footer produced a
wrapped `num_children` that made `ParquetMetaDataReader::decode_metadata`
request about 4.6 GB (the allocation itself is the same problem as #11954; this
report is about the varint decoding).
### To Reproduce
`Cargo.toml`:
```toml
[dependencies]
parquet = { version = "60.1.0", default-features = false }
```
`main.rs` (run with `wrap`; the `oom` argument runs the 84-byte footer):
```rust
use parquet::basic::{Repetition, Type};
use parquet::column::writer::ColumnWriter;
use parquet::file::metadata::ParquetMetaDataReader;
use parquet::file::properties::WriterProperties;
use parquet::file::writer::SerializedFileWriter;
use parquet::schema::types::Type as SchemaType;
use std::sync::Arc;
// 84-byte Thrift compact FileMetaData: version, then a schema list whose
first
// element has `num_children` encoded as a 15-byte varint (0x93 x14, 0x04).
const FOOTER: [u8; 84] = [0x15, 0x02, 0x19, 0x3c, 0x35, 0x00, 0x18, 0x0d,
0x64, 0x75, 0x63, 0x6b, 0x5f, 0x68, 0x64, 0x73, 0x63, 0x62, 0x65, 0x6d, 0x61,
0x15, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93, 0x93,
0x93, 0x93, 0x04, 0x00, 0x15, 0x04, 0x25, 0x02, 0x18, 0x02, 0x69, 0x64, 0x19,
0x01, 0x00, 0x15, 0x0c, 0x25, 0x02, 0x18, 0x08, 0x67, 0x65, 0x7a, 0x6f, 0x6d,
0x65, 0x19, 0x1c, 0x19, 0x90, 0x01, 0x26, 0xfc, 0xf6, 0x02, 0x16, 0x20, 0x62,
0x31, 0x24, 0x30, 0x61, 0x16, 0xfa, 0x91, 0x01, 0x16, 0x29, 0x00];
/// Thrift FileMetaData bytes of a small valid file (version = 1, so the
first
/// bytes are 0x15 0x02: field 1, i32, zigzag(1) = 2).
fn valid_metadata() -> Vec<u8> {
let schema = SchemaType::group_type_builder("s")
.with_fields(vec![Arc::new(
SchemaType::primitive_type_builder("a", Type::INT32)
.with_repetition(Repetition::REQUIRED)
.build()
.unwrap(),
)])
.build()
.unwrap();
let mut out = Vec::new();
let props = Arc::new(WriterProperties::builder().build());
let mut w = SerializedFileWriter::new(&mut out, Arc::new(schema),
props).unwrap();
let mut rg = w.next_row_group().unwrap();
let mut col = rg.next_column().unwrap().unwrap();
if let ColumnWriter::Int32ColumnWriter(c) = col.untyped() {
c.write_batch(&[1], None, None).unwrap();
}
col.close().unwrap();
rg.close().unwrap();
w.close().unwrap();
let n = u32::from_le_bytes(out[out.len() - 8..out.len() -
4].try_into().unwrap()) as usize;
out[out.len() - 8 - n..out.len() - 8].to_vec()
}
fn main() {
match std::env::args().nth(1).unwrap().as_str() {
"oom" => {
println!("{:?}",
ParquetMetaDataReader::decode_metadata(&FOOTER).map(|_| ()));
}
"wrap" => {
let m = valid_metadata();
assert_eq!(&m[..2], &[0x15, 0x02]);
let v = ParquetMetaDataReader::decode_metadata(&m).unwrap();
println!("original version = {}", v.file_metadata().version());
// Re-encode `version` as an 11-byte varint: 0x82, 0x80 x9, 0x01.
// The 11th byte is shifted by 70 bits, which wraps to 6, so the
// decoded value is 2 | (1 << 6) = 66 (zigzag: 33).
let mut bad = vec![0x15, 0x82];
bad.extend([0x80; 9]);
bad.push(0x01);
bad.extend_from_slice(&m[2..]);
let v = ParquetMetaDataReader::decode_metadata(&bad).unwrap();
println!("11-byte varint version = {}",
v.file_metadata().version());
}
_ => unreachable!(),
}
}
```
Output on parquet 60.1.0 (`ulimit -v 2000000; timeout 60 ./repro-1 wrap`):
```
original version = 1
11-byte varint version = 33
```
`oom`:
```
memory allocation of 4599335088 bytes failed
timeout: the monitored command dumped core (exit 134)
```
Same output on current main (bb47f80).
### Expected behavior
`read_vlq` returns an error for a varint with more than 10 bytes (and for a
10th byte greater than 1), so `decode_metadata` returns `Err` for the 11-byte
`version` above.
### Additional context
Suggested fix: count shifts and return an error once `shift > 63`, for
example
```rust
if shift >= 64 {
return Err(ThriftProtocolError::IntegerOverflow);
}
```
and use `checked_shl`/a check that the 10th byte is at most 1 instead of
`wrapping_shl`. The `oom` footer still allocates on `num_children` until #11954
is fixed. Environment: parquet 60.1.0 and main bb47f80, rustc 1.99.1, Linux
aarch64.
--
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]
