This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new a2f3034ffa fix(parquet): bound row-group count by remaining input
before reserving (#11419)
a2f3034ffa is described below
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
