From d134340ef776745be25078a8b8037931cdb74bee Mon Sep 17 00:00:00 2001 From: Boaz Chen Date: Mon, 27 Apr 2026 17:15:57 +0300 Subject: [PATCH 1/2] Avoid panic on malformed thrift bool fields in parquet metadata Return a Parquet error when a compact thrift field is expected to be a bool but the field header contains a non-bool wire type. This replaces the remaining bool_val unwraps in generated thrift readers and the hand-expanded DataPageHeaderV2 reader. Add regression tests for malformed DictionaryPageHeader.is_sorted and DataPageHeaderV2.is_compressed headers to ensure corrupt input returns an error instead of panicking. --- parquet/src/file/metadata/thrift/mod.rs | 88 +++++++++++++++++++++++-- parquet/src/parquet_macros.rs | 5 +- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/parquet/src/file/metadata/thrift/mod.rs b/parquet/src/file/metadata/thrift/mod.rs index 88cb96f35555..e9ac1d9323fa 100644 --- a/parquet/src/file/metadata/thrift/mod.rs +++ b/parquet/src/file/metadata/thrift/mod.rs @@ -1128,7 +1128,12 @@ impl DataPageHeaderV2 { repetition_levels_byte_length = Some(val); } 7 => { - let val = field_ident.bool_val.unwrap(); + let val = field_ident.bool_val.ok_or_else(|| { + general_err!( + "Expected bool field but got thrift type {:?}", + field_ident.field_type + ) + })?; is_compressed = Some(val); } _ => { @@ -1721,13 +1726,17 @@ write_thrift_field!(RustBoundingBox, FieldType::Struct); #[cfg(test)] pub(crate) mod tests { - use crate::basic::Type as PhysicalType; + use crate::basic::{Encoding, PageType, Type as PhysicalType}; use crate::errors::Result; - use crate::file::metadata::thrift::{BoundingBox, SchemaElement, write_schema}; + use crate::file::metadata::thrift::{ + BoundingBox, DataPageHeaderV2, DictionaryPageHeader, PageHeader, SchemaElement, + write_schema, + }; use crate::file::metadata::{ColumnChunkMetaData, ParquetMetaDataOptions, RowGroupMetaData}; use crate::parquet_thrift::tests::test_roundtrip; use crate::parquet_thrift::{ - ElementType, ThriftCompactOutputProtocol, ThriftSliceInputProtocol, read_thrift_vec, + ElementType, ThriftCompactOutputProtocol, ThriftSliceInputProtocol, WriteThrift, + read_thrift_vec, }; use crate::schema::types::{ ColumnDescriptor, ColumnPath, SchemaDescriptor, TypePtr, num_nodes, @@ -1794,6 +1803,29 @@ pub(crate) mod tests { read_thrift_vec(&mut prot) } + fn thrift_bytes(value: &T) -> Vec { + let mut buf = Vec::new(); + let mut writer = ThriftCompactOutputProtocol::new(&mut buf); + value.write_thrift(&mut writer).unwrap(); + buf + } + + fn change_false_bool_field_to_i32(buf: &mut [u8]) { + let pos = buf + .iter() + .rposition(|byte| *byte == 0x12) + .expect("expected BOOL_FALSE field header byte"); + buf[pos] = 0x15; + } + + fn assert_malformed_bool_error(err: crate::errors::ParquetError) { + let msg = err.to_string(); + assert!( + msg.contains("Expected bool field"), + "unexpected error message: {msg}" + ); + } + #[test] fn test_bounding_box_roundtrip() { test_roundtrip(BoundingBox { @@ -1873,4 +1905,52 @@ pub(crate) mod tests { .unwrap(); assert_eq!(decoded_zero.null_count_opt(), Some(0)); } + + #[test] + fn malformed_bool_field_returns_error_not_panic() { + let page_header = PageHeader { + r#type: PageType::DICTIONARY_PAGE, + uncompressed_page_size: 1, + compressed_page_size: 1, + crc: None, + data_page_header: None, + index_page_header: None, + dictionary_page_header: Some(DictionaryPageHeader { + num_values: 1, + encoding: Encoding::PLAIN, + is_sorted: Some(false), + }), + data_page_header_v2: None, + }; + + let mut buf = thrift_bytes(&page_header); + change_false_bool_field_to_i32(&mut buf); + + let mut prot = ThriftSliceInputProtocol::new(&buf); + let err = PageHeader::read_thrift_without_stats(&mut prot) + .expect_err("malformed bool field should return an error"); + assert_malformed_bool_error(err); + } + + #[test] + fn malformed_data_page_v2_bool_field_returns_error_not_panic() { + let data_page_header_v2 = DataPageHeaderV2 { + num_values: 1, + num_nulls: 0, + num_rows: 1, + encoding: Encoding::PLAIN, + definition_levels_byte_length: 0, + repetition_levels_byte_length: 0, + is_compressed: Some(false), + statistics: None, + }; + + let mut buf = thrift_bytes(&data_page_header_v2); + change_false_bool_field_to_i32(&mut buf); + + let mut prot = ThriftSliceInputProtocol::new(&buf); + let err = DataPageHeaderV2::read_thrift_without_stats(&mut prot) + .expect_err("malformed bool field should return an error"); + assert_malformed_bool_error(err); + } } diff --git a/parquet/src/parquet_macros.rs b/parquet/src/parquet_macros.rs index 714015e10e32..13c476343164 100644 --- a/parquet/src/parquet_macros.rs +++ b/parquet/src/parquet_macros.rs @@ -464,7 +464,10 @@ macro_rules! __thrift_read_field { $crate::parquet_thrift::OrderedF64::read_thrift(&mut *$prot)? }; ($prot:tt, $field_ident:tt, bool) => { - $field_ident.bool_val.unwrap() + $field_ident.bool_val.ok_or_else(|| general_err!( + "Expected bool field but got thrift type {:?}", + $field_ident.field_type + ))? }; ($prot:tt, $field_ident:tt, $field_type:ident) => { $field_type::read_thrift(&mut *$prot)? From 22317bfec87a565fcaa67a99ec17587fb685bc58 Mon Sep 17 00:00:00 2001 From: Boaz Chen Date: Tue, 28 Apr 2026 06:15:21 +0300 Subject: [PATCH 2/2] Addressing a PR comment --- parquet/src/file/metadata/thrift/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/parquet/src/file/metadata/thrift/mod.rs b/parquet/src/file/metadata/thrift/mod.rs index e9ac1d9323fa..ad8f6312c215 100644 --- a/parquet/src/file/metadata/thrift/mod.rs +++ b/parquet/src/file/metadata/thrift/mod.rs @@ -1128,13 +1128,13 @@ impl DataPageHeaderV2 { repetition_levels_byte_length = Some(val); } 7 => { - let val = field_ident.bool_val.ok_or_else(|| { - general_err!( + if field_ident.bool_val.is_none() { + return Err(general_err!( "Expected bool field but got thrift type {:?}", field_ident.field_type - ) - })?; - is_compressed = Some(val); + )); + } + is_compressed = field_ident.bool_val; } _ => { prot.skip(field_ident.field_type)?;