etseidl commented on code in PR #11419:
URL: https://github.com/apache/arrow-rs/pull/11419#discussion_r4201059267
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -820,7 +820,23 @@ pub(crate) fn parquet_metadata_from_bytes(
let list_ident = prot.read_list_begin()?;
// check for list of struct
validate_list_type(ElementType::Struct, &list_ident)?;
- let mut rg_vec = Vec::with_capacity(list_ident.size as usize);
+ let size = list_ident.size as usize;
+ // This list is decoded inline rather than through
+ // `read_thrift_vec`, so it needs the same bound: each row
+ // group occupies at least one byte on the wire, so a declared
+ // count larger than the remaining input is malformed. Reject
+ // it before reserving, so a tiny footer cannot drive a
+ // multi-gigabyte allocation.
Review Comment:
```suggestion
// Validate list size. See
crate::parquet_thrift::read_thrift_vec
```
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -2095,4 +2111,51 @@ pub(crate) mod tests {
vec![None, Some(1), Some(2)],
);
}
+
+ /// A footer whose `row_groups` list declares more elements than the
+ /// remaining input could hold is rejected before the reserve, so a tiny
+ /// footer cannot drive a large allocation. This list is decoded inline
+ /// rather than through `read_thrift_vec`, so it carries its own bound.
Review Comment:
```suggestion
/// Test row group on-wire size validation.
```
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -820,7 +820,23 @@ pub(crate) fn parquet_metadata_from_bytes(
let list_ident = prot.read_list_begin()?;
// check for list of struct
validate_list_type(ElementType::Struct, &list_ident)?;
- let mut rg_vec = Vec::with_capacity(list_ident.size as usize);
+ let size = list_ident.size as usize;
+ // This list is decoded inline rather than through
+ // `read_thrift_vec`, so it needs the same bound: each row
+ // group occupies at least one byte on the wire, so a declared
+ // count larger than the remaining input is malformed. Reject
+ // it before reserving, so a tiny footer cannot drive a
+ // multi-gigabyte allocation.
+ if let Some(remaining) = prot.remaining_bytes()
Review Comment:
Rather than repeating this code again, I wonder if we should add a list size
validator either as a standalone function or add it to the
`ThriftCompactInputProtocol` trait. Use that new function here and in
`read_thrift_vec`.
--
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]
