From d06933b5e3886777e9a871879a3ac89d61fd7721 Mon Sep 17 00:00:00 2001 From: masumi ryugo <280057467+masumi-ryugo@users.noreply.github.com> Date: Fri, 1 May 2026 23:34:31 +0900 Subject: [PATCH 1/5] fix(parquet): bound read_thrift_vec allocation, validate list size The Thrift compact-protocol list header carries an `i32` `size` that is read directly from untrusted input. `read_thrift_vec` was passing this value straight to `Vec::with_capacity`, so a few bytes of attacker-crafted input could request a multi-GB-to-multi-hundred-GB allocation and panic in `alloc::raw_vec::handle_error` with "capacity overflow" before any element was decoded. Per review (#9868): the fix is split between the protocol layer and the allocation site, instead of the original `remaining_bytes()`-based clamp. * `ThriftCompactInputProtocol::read_list_begin` now decodes the long-form size as `i32::try_from(self.read_vlq()?)?` instead of `as _`. A varint whose value exceeds `i32::MAX` is rejected at the protocol layer rather than wrapping into a negative `i32` and being smuggled downstream. This requires `From for ThriftProtocolError` (new `IntegerOverflow` variant). * `read_thrift_vec` allocates with `Vec::try_reserve_exact(size as usize)` and returns `ParquetError` on `Err`. This catches the `size_of::() * size > isize::MAX` capacity-overflow case (which previously panicked inside `alloc::raw_vec`) without needing the protocol to expose `remaining_bytes`. The `remaining_bytes` trait method and its `ThriftSliceInputProtocol` override are removed. Two regression tests: * `test_decode_metadata_huge_thrift_list_does_not_panic` runs the 10-byte fuzzer repro through the public `ParquetMetaDataReader::decode_metadata` entrypoint (panics with capacity overflow before this fix; returns `Err` cleanly after). * `test_read_list_begin_size_above_i32_max_returns_err` exercises the new protocol-layer rejection directly (without the fix the size wraps into `i32::MIN` and `read_list_begin` returns `Ok(... size = -2147483648)`, which is exactly the smuggled-negative case the fix now rejects). The earlier `test_read_thrift_vec_huge_size_does_not_panic` test from the first revision is dropped: per @etseidl, it returns `Err(EOF)` on unmodified main as well and so does not exercise the bug. Found via cargo-fuzz libFuzzer harness wrapping `ParquetMetaDataReader::parse_and_finish` and `ParquetRecordBatchReaderBuilder::try_new`. ~95 unique crashing inputs converged on this single root cause. Closes (in part) #9874. --- parquet/src/parquet_thrift.rs | 62 +++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index 6c82a0bf2c07..8e9be9628acd 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -36,6 +36,7 @@ use crate::{ write_thrift_field, }; use std::io::Error; +use std::num::TryFromIntError; use std::str::Utf8Error; #[derive(Debug)] @@ -46,6 +47,7 @@ pub(crate) enum ThriftProtocolError { InvalidElementType(u8), FieldDeltaOverflow { field_delta: u8, last_field_id: i16 }, InvalidBoolean(u8), + IntegerOverflow, Utf8Error, SkipDepth(FieldType), SkipUnsupportedType(FieldType), @@ -70,6 +72,9 @@ impl From for ParquetError { ThriftProtocolError::InvalidBoolean(value) => { general_err!("cannot convert {} into bool", value) } + ThriftProtocolError::IntegerOverflow => { + general_err!("integer overflow decoding thrift value") + } ThriftProtocolError::Utf8Error => general_err!("invalid utf8"), ThriftProtocolError::SkipDepth(field_type) => { general_err!("cannot parse past {:?}", field_type) @@ -94,6 +99,13 @@ impl From for ThriftProtocolError { } } +impl From for ThriftProtocolError { + fn from(_: TryFromIntError) -> Self { + // ignore error payload to reduce the size of ThriftProtocolError + Self::IntegerOverflow + } +} + pub type ThriftProtocolResult = Result; /// Wrapper for thrift `double` fields. This is used to provide @@ -317,7 +329,13 @@ pub(crate) trait ThriftCompactInputProtocol<'a> { // high bits set high if count and type encoded separately possible_element_count as i32 } else { - self.read_vlq()? as _ + // The list size on the wire is an unsigned varint, but we represent + // it as `i32` (matching Java's `int` and the Parquet Thrift schema). + // A varint that decodes above `i32::MAX` is malformed input — reject + // it here at the protocol layer rather than letting the cast wrap + // into a negative size that downstream allocation code has to + // re-validate. + i32::try_from(self.read_vlq()?)? }; Ok(ListIdentifier { @@ -686,8 +704,18 @@ where R: ThriftCompactInputProtocol<'a>, T: ReadThrift<'a, R>, { + // `read_list_begin` rejects sizes above `i32::MAX`; `try_reserve_exact` + // catches the remaining attacker-controlled allocation case so a + // malformed `size` cannot trigger a `Vec::with_capacity` panic. let list_ident = prot.read_list_begin()?; - let mut res = Vec::with_capacity(list_ident.size as usize); + let len = list_ident.size as usize; + let mut res: Vec = Vec::new(); + if res.try_reserve_exact(len).is_err() { + return Err(general_err!( + "cannot allocate Thrift list of {} elements", + len + )); + } for _ in 0..list_ident.size { let val = T::read_thrift(prot)?; res.push(val); @@ -1106,4 +1134,34 @@ pub(crate) mod tests { assert_eq!(header.size, 0); assert_eq!(header.element_type, ElementType::Byte); } + + /// Regression test for the 10-byte fuzzer repro that panicked inside + /// `alloc::raw_vec::handle_error` with "capacity overflow" before this + /// fix: the list header for `SchemaElement` decodes to a multi-billion + /// `size`, and `Vec::with_capacity(size)` then tripped the + /// `size_of::() * size` isize-overflow check. After the + /// fix the allocation goes through `try_reserve_exact` and returns a + /// clean `ParquetError`. + #[test] + fn test_decode_metadata_huge_thrift_list_does_not_panic() { + let bytes: [u8; 10] = [0x28, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0xfc, 0x51]; + let result = crate::file::metadata::ParquetMetaDataReader::decode_metadata(&bytes); + // The bytes are not valid Parquet metadata — we only require that + // `decode_metadata` returns an error rather than panicking. + assert!(result.is_err(), "expected error, got {result:?}"); + } + + /// A Thrift list header whose `size` varint decodes above `i32::MAX` + /// must be rejected at the protocol layer rather than wrapping into a + /// negative `i32` and being smuggled into downstream allocation code. + #[test] + fn test_read_list_begin_size_above_i32_max_returns_err() { + // List header: element_type=8 (Binary), 0xF=follow-up varint. + // Varint 80 80 80 80 08 decodes to 0x8000_0000 = i32::MAX + 1. + let mut data: Vec = vec![0xF8]; + data.extend_from_slice(&[0x80, 0x80, 0x80, 0x80, 0x08]); + let mut prot = ThriftSliceInputProtocol::new(&data); + let result = prot.read_list_begin(); + assert!(result.is_err(), "expected error, got {result:?}"); + } } From 252ebdf9306bb76f6cf245e9e2a359bd68aa8aa7 Mon Sep 17 00:00:00 2001 From: seidl Date: Mon, 4 May 2026 08:58:20 -0700 Subject: [PATCH 2/5] test a different approach --- parquet/src/errors.rs | 7 +++++++ parquet/src/parquet_thrift.rs | 7 +------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/parquet/src/errors.rs b/parquet/src/errors.rs index 0533d7662c5f..83ceb567fe7f 100644 --- a/parquet/src/errors.rs +++ b/parquet/src/errors.rs @@ -18,6 +18,7 @@ //! Common Parquet errors and macros. use core::num::TryFromIntError; +use std::collections::TryReserveError; use std::error::Error; use std::string::FromUtf8Error; use std::{cell, io, result, str}; @@ -132,6 +133,12 @@ impl From for ParquetError { } } +impl From for ParquetError { + fn from(e: TryReserveError) -> ParquetError { + ParquetError::External(Box::new(e)) + } +} + #[cfg(feature = "arrow")] impl From for ParquetError { fn from(e: ArrowError) -> ParquetError { diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index 8e9be9628acd..4df6c0cb0143 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -710,12 +710,7 @@ where let list_ident = prot.read_list_begin()?; let len = list_ident.size as usize; let mut res: Vec = Vec::new(); - if res.try_reserve_exact(len).is_err() { - return Err(general_err!( - "cannot allocate Thrift list of {} elements", - len - )); - } + res.try_reserve_exact(len)?; for _ in 0..list_ident.size { let val = T::read_thrift(prot)?; res.push(val); From cb1e15017501025524b57322ddd4c58526b63f45 Mon Sep 17 00:00:00 2001 From: seidl Date: Mon, 4 May 2026 10:17:49 -0700 Subject: [PATCH 3/5] try infallible conversion --- parquet/src/parquet_thrift.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index 4df6c0cb0143..4cba61c36a52 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -335,7 +335,8 @@ pub(crate) trait ThriftCompactInputProtocol<'a> { // it here at the protocol layer rather than letting the cast wrap // into a negative size that downstream allocation code has to // re-validate. - i32::try_from(self.read_vlq()?)? + //i32::try_from(self.read_vlq()?)? + self.read_vlq()? as i32 }; Ok(ListIdentifier { From 129cb374efa98cca9880181fc7e7b7c8273287cc Mon Sep 17 00:00:00 2001 From: seidl Date: Mon, 4 May 2026 10:29:45 -0700 Subject: [PATCH 4/5] revert vec sizing --- parquet/src/parquet_thrift.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index 4cba61c36a52..cf5c7bd56b44 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -709,9 +709,10 @@ where // catches the remaining attacker-controlled allocation case so a // malformed `size` cannot trigger a `Vec::with_capacity` panic. let list_ident = prot.read_list_begin()?; - let len = list_ident.size as usize; + /*let len = list_ident.size as usize; let mut res: Vec = Vec::new(); - res.try_reserve_exact(len)?; + res.try_reserve_exact(len)?;*/ + let mut res = Vec::with_capacity(list_ident.size as usize); for _ in 0..list_ident.size { let val = T::read_thrift(prot)?; res.push(val); From 462db19f51e8ae1050967b827c36992f99d070eb Mon Sep 17 00:00:00 2001 From: seidl Date: Mon, 4 May 2026 10:40:49 -0700 Subject: [PATCH 5/5] restore fallible conversion --- parquet/src/parquet_thrift.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/parquet/src/parquet_thrift.rs b/parquet/src/parquet_thrift.rs index cf5c7bd56b44..775ea846727c 100644 --- a/parquet/src/parquet_thrift.rs +++ b/parquet/src/parquet_thrift.rs @@ -335,8 +335,7 @@ pub(crate) trait ThriftCompactInputProtocol<'a> { // it here at the protocol layer rather than letting the cast wrap // into a negative size that downstream allocation code has to // re-validate. - //i32::try_from(self.read_vlq()?)? - self.read_vlq()? as i32 + i32::try_from(self.read_vlq()?)? }; Ok(ListIdentifier {