Skip to content

Commit ba7dada

Browse files
authored
fix(parquet): avoid panic on ColumnIndex length mismatch (#9833)
# Which issue does this PR close? - Closes #9832 . # Rationale for this change In `parquet/src/file/page_index/column_index.rs`, `ColumnIndex` decoding assumes that page-aligned arrays (`null_pages`, `min_values`, `max_values`, and optional arrays) have matching lengths, but this is not validated. As a result, malformed metadata can trigger an out-of-bounds panic during decoding instead of returning a `ParquetError`. Since parquet files are external input, this should be handled safely. # What changes are included in this PR? * Added validation in: * `PrimitiveColumnIndex::try_new` * `ByteArrayColumnIndex::try_new` * Ensures: * `min_values.len() == null_pages.len()` * `max_values.len() == null_pages.len()` * optional arrays (`null_counts`, histograms) are consistent with page count * Returns `ParquetError` on mismatch instead of panicking # Are these changes tested? Yes. Added a unit test: * `test_column_index_rejects_mismatched_min_max_lengths` This constructs a `ColumnIndex` with mismatched lengths and verifies that decoding returns an error instead of panicking. # Are there any user-facing changes? No.
1 parent fe3c0c9 commit ba7dada

1 file changed

Lines changed: 80 additions & 0 deletions

File tree

parquet/src/file/page_index/column_index.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,36 @@ impl<T: ParquetValueType> PrimitiveColumnIndex<T> {
106106
) -> Result<Self> {
107107
let len = null_pages.len();
108108

109+
if min_bytes.len() != len || max_bytes.len() != len {
110+
return Err(ParquetError::General(format!(
111+
"ColumnIndex min/max length mismatch: expected {len}, got min={} max={}",
112+
min_bytes.len(),
113+
max_bytes.len()
114+
)));
115+
}
116+
if let Some(ref nc) = null_counts {
117+
if nc.len() != len {
118+
return Err(ParquetError::General(format!(
119+
"ColumnIndex null_counts length mismatch: expected {len}, got {}",
120+
nc.len()
121+
)));
122+
}
123+
}
124+
if let Some(ref rep) = repetition_level_histograms {
125+
if len != 0 && rep.len() % len != 0 {
126+
return Err(ParquetError::General(
127+
"Invalid repetition_level_histograms length".to_string(),
128+
));
129+
}
130+
}
131+
if let Some(ref def) = definition_level_histograms {
132+
if len != 0 && def.len() % len != 0 {
133+
return Err(ParquetError::General(
134+
"Invalid definition_level_histograms length".to_string(),
135+
));
136+
}
137+
}
138+
109139
let mut min_values = Vec::with_capacity(len);
110140
let mut max_values = Vec::with_capacity(len);
111141

@@ -295,6 +325,36 @@ impl ByteArrayColumnIndex {
295325
) -> Result<Self> {
296326
let len = null_pages.len();
297327

328+
if min_values.len() != len || max_values.len() != len {
329+
return Err(ParquetError::General(format!(
330+
"ColumnIndex min/max length mismatch: expected {len}, got min={} max={}",
331+
min_values.len(),
332+
max_values.len()
333+
)));
334+
}
335+
if let Some(ref nc) = null_counts {
336+
if nc.len() != len {
337+
return Err(ParquetError::General(format!(
338+
"ColumnIndex null_counts length mismatch: expected {len}, got {}",
339+
nc.len()
340+
)));
341+
}
342+
}
343+
if let Some(ref rep) = repetition_level_histograms {
344+
if len != 0 && rep.len() % len != 0 {
345+
return Err(ParquetError::General(
346+
"Invalid repetition_level_histograms length".to_string(),
347+
));
348+
}
349+
}
350+
if let Some(ref def) = definition_level_histograms {
351+
if len != 0 && def.len() % len != 0 {
352+
return Err(ParquetError::General(
353+
"Invalid definition_level_histograms length".to_string(),
354+
));
355+
}
356+
}
357+
298358
let min_len = min_values.iter().map(|&v| v.len()).sum();
299359
let max_len = max_values.iter().map(|&v| v.len()).sum();
300360
let mut min_bytes = vec![0u8; min_len];
@@ -738,4 +798,24 @@ mod tests {
738798
"Parquet error: error converting value, expected 4 bytes got 0"
739799
);
740800
}
801+
802+
#[test]
803+
fn test_column_index_rejects_mismatched_min_max_lengths() {
804+
// Two pages, but only one min/max entry. The entry itself is valid i32 bytes,
805+
// so this specifically checks that lengths must match the number of pages.
806+
let column_index = ThriftColumnIndex {
807+
null_pages: vec![false, false],
808+
min_values: vec![&[1u8, 0, 0, 0]],
809+
max_values: vec![&[10u8, 0, 0, 0]],
810+
null_counts: None,
811+
repetition_level_histograms: None,
812+
definition_level_histograms: None,
813+
boundary_order: BoundaryOrder::UNORDERED,
814+
};
815+
816+
// ColumnIndex arrays must align with the number of pages (null_pages.len()).
817+
let err = PrimitiveColumnIndex::<i32>::try_from_thrift(column_index).unwrap_err();
818+
// Should fail because min/max lengths don’t match null_pages
819+
assert!(err.to_string().contains("length mismatch"));
820+
}
741821
}

0 commit comments

Comments
 (0)