Skip to content

Commit fe3c0c9

Browse files
authored
fix(parquet): Avoid panic on malformed thrift bool fields in parquet metadata (#9840)
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. # Which issue does this PR close? - Closes #9839. # Rationale for this change See #9839 # What changes are included in this PR? Proper error handling instead of using `unwrap`. # Are these changes tested? Yes. Added 2 UTs. # Are there any user-facing changes? An error string that can find its way all the way to the end user.
1 parent fc3f778 commit fe3c0c9

2 files changed

Lines changed: 89 additions & 6 deletions

File tree

parquet/src/file/metadata/thrift/mod.rs

Lines changed: 85 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,8 +1128,13 @@ impl DataPageHeaderV2 {
11281128
repetition_levels_byte_length = Some(val);
11291129
}
11301130
7 => {
1131-
let val = field_ident.bool_val.unwrap();
1132-
is_compressed = Some(val);
1131+
if field_ident.bool_val.is_none() {
1132+
return Err(general_err!(
1133+
"Expected bool field but got thrift type {:?}",
1134+
field_ident.field_type
1135+
));
1136+
}
1137+
is_compressed = field_ident.bool_val;
11331138
}
11341139
_ => {
11351140
prot.skip(field_ident.field_type)?;
@@ -1721,13 +1726,17 @@ write_thrift_field!(RustBoundingBox, FieldType::Struct);
17211726

17221727
#[cfg(test)]
17231728
pub(crate) mod tests {
1724-
use crate::basic::Type as PhysicalType;
1729+
use crate::basic::{Encoding, PageType, Type as PhysicalType};
17251730
use crate::errors::Result;
1726-
use crate::file::metadata::thrift::{BoundingBox, SchemaElement, write_schema};
1731+
use crate::file::metadata::thrift::{
1732+
BoundingBox, DataPageHeaderV2, DictionaryPageHeader, PageHeader, SchemaElement,
1733+
write_schema,
1734+
};
17271735
use crate::file::metadata::{ColumnChunkMetaData, ParquetMetaDataOptions, RowGroupMetaData};
17281736
use crate::parquet_thrift::tests::test_roundtrip;
17291737
use crate::parquet_thrift::{
1730-
ElementType, ThriftCompactOutputProtocol, ThriftSliceInputProtocol, read_thrift_vec,
1738+
ElementType, ThriftCompactOutputProtocol, ThriftSliceInputProtocol, WriteThrift,
1739+
read_thrift_vec,
17311740
};
17321741
use crate::schema::types::{
17331742
ColumnDescriptor, ColumnPath, SchemaDescriptor, TypePtr, num_nodes,
@@ -1794,6 +1803,29 @@ pub(crate) mod tests {
17941803
read_thrift_vec(&mut prot)
17951804
}
17961805

1806+
fn thrift_bytes<T: WriteThrift>(value: &T) -> Vec<u8> {
1807+
let mut buf = Vec::new();
1808+
let mut writer = ThriftCompactOutputProtocol::new(&mut buf);
1809+
value.write_thrift(&mut writer).unwrap();
1810+
buf
1811+
}
1812+
1813+
fn change_false_bool_field_to_i32(buf: &mut [u8]) {
1814+
let pos = buf
1815+
.iter()
1816+
.rposition(|byte| *byte == 0x12)
1817+
.expect("expected BOOL_FALSE field header byte");
1818+
buf[pos] = 0x15;
1819+
}
1820+
1821+
fn assert_malformed_bool_error(err: crate::errors::ParquetError) {
1822+
let msg = err.to_string();
1823+
assert!(
1824+
msg.contains("Expected bool field"),
1825+
"unexpected error message: {msg}"
1826+
);
1827+
}
1828+
17971829
#[test]
17981830
fn test_bounding_box_roundtrip() {
17991831
test_roundtrip(BoundingBox {
@@ -1873,4 +1905,52 @@ pub(crate) mod tests {
18731905
.unwrap();
18741906
assert_eq!(decoded_zero.null_count_opt(), Some(0));
18751907
}
1908+
1909+
#[test]
1910+
fn malformed_bool_field_returns_error_not_panic() {
1911+
let page_header = PageHeader {
1912+
r#type: PageType::DICTIONARY_PAGE,
1913+
uncompressed_page_size: 1,
1914+
compressed_page_size: 1,
1915+
crc: None,
1916+
data_page_header: None,
1917+
index_page_header: None,
1918+
dictionary_page_header: Some(DictionaryPageHeader {
1919+
num_values: 1,
1920+
encoding: Encoding::PLAIN,
1921+
is_sorted: Some(false),
1922+
}),
1923+
data_page_header_v2: None,
1924+
};
1925+
1926+
let mut buf = thrift_bytes(&page_header);
1927+
change_false_bool_field_to_i32(&mut buf);
1928+
1929+
let mut prot = ThriftSliceInputProtocol::new(&buf);
1930+
let err = PageHeader::read_thrift_without_stats(&mut prot)
1931+
.expect_err("malformed bool field should return an error");
1932+
assert_malformed_bool_error(err);
1933+
}
1934+
1935+
#[test]
1936+
fn malformed_data_page_v2_bool_field_returns_error_not_panic() {
1937+
let data_page_header_v2 = DataPageHeaderV2 {
1938+
num_values: 1,
1939+
num_nulls: 0,
1940+
num_rows: 1,
1941+
encoding: Encoding::PLAIN,
1942+
definition_levels_byte_length: 0,
1943+
repetition_levels_byte_length: 0,
1944+
is_compressed: Some(false),
1945+
statistics: None,
1946+
};
1947+
1948+
let mut buf = thrift_bytes(&data_page_header_v2);
1949+
change_false_bool_field_to_i32(&mut buf);
1950+
1951+
let mut prot = ThriftSliceInputProtocol::new(&buf);
1952+
let err = DataPageHeaderV2::read_thrift_without_stats(&mut prot)
1953+
.expect_err("malformed bool field should return an error");
1954+
assert_malformed_bool_error(err);
1955+
}
18761956
}

parquet/src/parquet_macros.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,10 @@ macro_rules! __thrift_read_field {
464464
$crate::parquet_thrift::OrderedF64::read_thrift(&mut *$prot)?
465465
};
466466
($prot:tt, $field_ident:tt, bool) => {
467-
$field_ident.bool_val.unwrap()
467+
$field_ident.bool_val.ok_or_else(|| general_err!(
468+
"Expected bool field but got thrift type {:?}",
469+
$field_ident.field_type
470+
))?
468471
};
469472
($prot:tt, $field_ident:tt, $field_type:ident) => {
470473
$field_type::read_thrift(&mut *$prot)?

0 commit comments

Comments
 (0)