This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11419-4056f9ce95a4e28e9bf2d3b652a9938328a287ab in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit a2f3034ffaf107c6b2afb13371d128e892551ac5 Author: Panagiotis Moustafellos <[email protected]> AuthorDate: Wed Oct 7 19:52:11 2026 +0000 fix(parquet): bound row-group count by remaining input before reserving (#11419) # Which issue does this PR close? - Closes #11418. # Rationale for this change Decoding a Parquet footer reserves a `Vec` sized from the declared row-group count before reading any row group: ```rust let mut rg_vec = Vec::with_capacity(list_ident.size as usize); ``` `list_ident.size` comes straight off the wire and `RowGroupMetaData` is a large struct, so a footer of a few bytes that declares a huge row-group count asks for `count * size_of::<RowGroupMetaData>()` up front, before a single row group is read. On a host under memory pressure that allocation aborts the process. PR #10979 bounded `read_thrift_vec` by the remaining input. The row-group list is decoded inline in `parquet_metadata_from_bytes` and does not go through that helper, so the bound did not reach it. The other footer lists (schema elements, column chunks, sorting columns, key/values, column orders) do go through `read_thrift_vec`. This is the one that did not. # What changes are included in this PR? - Apply the same remaining-input bound inline at the row-group list: each row group is at least one byte on the wire, so a declared count larger than the remaining input is malformed and is rejected before the reserve. - A test (`row_group_count_exceeding_remaining_input_is_rejected`) builds a valid footer prefix and declares an over-sized `row_groups` list, then asserts the decode reports the bound. Without the bound the same input instead fails later with `Required field columns is missing`, so the test is shown failing against the unfixed code. # Are these changes tested? Yes. The new test passes with the bound and fails without it, as described above. # Are there any user-facing changes? A malformed footer that declares more row groups than its bytes can hold now returns a `Parquet error` describing the bound, rather than attempting a large allocation. This was found while hardening the Parquet external-table reader in [Ravel](https://github.com/NOFireAI/ravel). --------- Signed-off-by: Panagiotis Moustafellos <[email protected]> Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Ed Seidl <[email protected]> --- parquet/src/file/metadata/thrift/mod.rs | 50 +++++++++++++++++++++++++++++++-- parquet/src/parquet_thrift.rs | 29 +++++++++++-------- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/parquet/src/file/metadata/thrift/mod.rs b/parquet/src/file/metadata/thrift/mod.rs index 8bc0cbbd0b..dffaacf440 100644 --- a/parquet/src/file/metadata/thrift/mod.rs +++ b/parquet/src/file/metadata/thrift/mod.rs @@ -51,7 +51,7 @@ use crate::{ parquet_thrift::{ ElementType, FieldType, ReadThrift, ThriftCompactInputProtocol, ThriftCompactOutputProtocol, ThriftSliceInputProtocol, WriteThrift, WriteThriftField, - read_thrift_vec, validate_list_type, + check_list_size, read_thrift_vec, validate_list_type, }, schema::types::{ ColumnDescriptor, SchemaDescriptor, TypePtr, num_nodes, parquet_schema_from_array, @@ -820,7 +820,10 @@ 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; + // Validate list size. + check_list_size(prot.remaining_bytes(), size)?; + let mut rg_vec = Vec::with_capacity(size); for _ in 0..list_ident.size { rg_vec.push(read_row_group(&mut prot, schema_descr, options)?); @@ -2095,4 +2098,47 @@ pub(crate) mod tests { vec![None, Some(1), Some(2)], ); } + + /// Test row group on-wire size validation. + #[test] + fn row_group_count_exceeding_remaining_input_is_rejected() { + use crate::file::metadata::thrift::parquet_metadata_from_bytes; + use crate::parquet_thrift::{FieldType, WriteThriftField}; + use crate::schema::types::Type as SchemaType; + + // Minimal valid schema: a group with one INT32 column. + let field = SchemaType::primitive_type_builder("c", PhysicalType::INT32) + .build() + .unwrap(); + let schema = SchemaType::group_type_builder("schema") + .with_fields(vec![Arc::new(field)]) + .build() + .unwrap(); + let root: TypePtr = Arc::new(schema); + let schema_len = num_nodes(&root).unwrap(); + + let mut buf = Vec::new(); + let mut w = ThriftCompactOutputProtocol::new(&mut buf); + // FileMetaData field 1: version. + 1i32.write_thrift_field(&mut w, 1, 0).unwrap(); + // field 2: schema (required before row groups are read). + w.write_field_begin(FieldType::List, 2, 1).unwrap(); + w.write_list_begin(ElementType::Struct, schema_len).unwrap(); + write_schema(&root, &mut w).unwrap(); + // field 3: num_rows. + 0i64.write_thrift_field(&mut w, 3, 2).unwrap(); + // field 4: row_groups, declaring 1024 structs with no element bytes + // after the header. Only the struct-stop byte remains. + w.write_field_begin(FieldType::List, 4, 3).unwrap(); + w.write_list_begin(ElementType::Struct, 1024).unwrap(); + w.write_struct_end().unwrap(); + + let err = parquet_metadata_from_bytes(&buf, None) + .expect_err("over-declared row-group count must be rejected") + .to_string(); + assert!( + err.contains("exceeds remaining input length"), + "expected a remaining-input bound error, got: {err}" + ); + } } diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index 9e69fc382b..39d0128f14 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -734,18 +734,7 @@ where let list_ident = prot.read_list_begin()?; validate_list_type(T::ELEMENT_TYPE, &list_ident)?; let size = list_ident.size as usize; - // Each list element occupies at least one byte on the wire. Bound the - // declared count by remaining input before reserving, so a malformed - // header cannot abort the process with a huge allocation. - if let Some(remaining) = prot.remaining_bytes() - && size > remaining - { - return Err(general_err!( - "Thrift list size {} exceeds remaining input length {}", - size, - remaining - )); - } + check_list_size(prot.remaining_bytes(), size)?; let mut res = Vec::with_capacity(size); for _ in 0..list_ident.size { let val = T::read_thrift(prot)?; @@ -765,6 +754,22 @@ pub(crate) fn validate_list_type(expected: ElementType, got: &ListIdentifier) -> Ok(()) } +/// Reject a declared list size that cannot fit the remaining input. Each list +/// element is at least one byte on the wire, so a size past the remaining +/// bytes is a malformed header; bounding it before reserving stops a huge +/// allocation. `remaining` is `None` when the protocol is not backed by a +/// finite buffer, in which case there is nothing to check against. +pub(crate) fn check_list_size(remaining: Option<usize>, size: usize) -> Result<()> { + if let Some(remaining) = remaining + && size > remaining + { + return Err(general_err!( + "Thrift list size {size} exceeds remaining input length {remaining}" + )); + } + Ok(()) +} + ///////////////////////// // thrift compact output
