You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jaideeppyne (via GitHub)" <gi...@apache.org> on 2026/09/19 11:24:11 UTC
[PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
jaideeppyne opened a new pull request, #11136:
URL: https://github.com/apache/arrow-rs/pull/11136
Closes #11121.
BYTE_STREAM_SPLIT previously used the page header count as its decode bound while deriving stream stride from the encoded bytes. A malformed header could therefore drive `join_streams_const` past the end of the buffer.
This change derives the decoder value count from complete encoded values, rejects partial values and encoded counts larger than the declared page count, and leaves declared counts that include Parquet v1 nulls valid. Higher-level readers now return their existing insufficient-values error for truncated required pages instead of panicking.
Tests cover the reported declared-count mismatch, partial values, and impossible oversized buffers.
Validation:
- `cargo test -p parquet --lib`
- `cargo clippy -p parquet --lib --all-features -- -D warnings`
- `cargo +stable fmt --all -- --check`
- `git diff --check`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub) via github" <gi...@arrow.apache.org>.
jaideeppyne commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4475395308
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2190,6 +2190,35 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ decoder.set_data(Bytes::from(vec![0; 64]), 63).unwrap();
Review Comment:
Both BYTE_STREAM_SPLIT decoder variants now derive their count from the encoded bytes and retain the partial-value check in `set_data`, so level-count estimates can overstate the physical values. The column-reader workaround is removed. Tests cover oversized estimates, partial reads, skipping, and both variable-width join paths. The real malformed-Parquet regression also returns an ordinary insufficient-values error instead of panicking. Published in b280e76; [focused tests and Clippy passed](https://github.com/jaideeppyne/arrow-rs/actions/runs/41945965606).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "alamb (via GitHub) via github" <gi...@arrow.apache.org>.
alamb commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4517169221
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
Review Comment:
what extra coverage does this test add?
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -72,10 +72,36 @@ fn join_streams_variable(
}
}
+/// Returns the number of values physically present in a BYTE_STREAM_SPLIT page.
+///
+/// `num_values` is an upper bound rather than an exact count: for a V1 data page it is
+/// the number of levels, which includes nulls (and repetition levels), while only
+/// non-null values are encoded. The byte streams therefore must not hold more values
+/// than `num_values`, but they may legitimately hold fewer. The stride between the
+/// streams, and so the number of values that can be decoded, comes from the encoded
+/// length. If the page declares more values than are present, the column reader reports
+/// that it could not read enough values instead of indexing past the end of the buffer.
+fn check_encoded_value_count(encoded_values: usize, num_values: usize) -> Result<usize> {
+ if encoded_values > num_values {
+ return Err(general_err!(
+ "Invalid BYTE_STREAM_SPLIT data: {encoded_values} encoded values exceeds the {num_values} values expected"
+ ));
+ }
+ Ok(encoded_values)
+}
+
impl<T: DataType> Decoder<T> for ByteStreamSplitDecoder<T> {
fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ let type_size = T::get_type_size();
+ if !data.len().is_multiple_of(type_size) {
+ return Err(general_err!(
+ "Invalid BYTE_STREAM_SPLIT data length {} for values of size {type_size}",
+ data.len()
+ ));
+ }
+
+ self.total_num_values = check_encoded_value_count(data.len() / type_size, num_values)?;
Review Comment:
👍 this is looking really nice
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ for count in [8, 63, usize::MAX] {
+ decoder.set_data(Bytes::from(vec![0; 64]), count).unwrap();
+ assert_eq!(decoder.values_left(), 8);
+ let mut values = vec![0.; 63];
+ assert_eq!(decoder.get(&mut values[..3]).unwrap(), 3);
+ assert_eq!(decoder.skip(2).unwrap(), 2);
+ assert_eq!(decoder.get(&mut values).unwrap(), 3);
+ assert_eq!(decoder.values_left(), 0);
+ assert_eq!(decoder.get(&mut values).unwrap(), 0);
+ }
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_invalid_data_length() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ let err = decoder.set_data(Bytes::from(vec![0; 63]), 8).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data length 63 for values of size 8"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_more_values_than_expected() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+ let err = decoder.set_data(Bytes::from(vec![0; 64]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values exceeds the 7 values expected"
+ );
+
+ let mut decoder = VariableWidthByteStreamSplitDecoder::<FixedLenByteArrayType>::new(5);
+ let err = decoder.set_data(Bytes::from(vec![0; 40]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values exceeds the 7 values expected"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_flba_uses_encoded_value_count() {
Review Comment:
likewise what extra coverage does this chekc?
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -136,6 +136,19 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_arrow_rs_gh_11136_byte_stream_split_truncated_page() {
+ // Checked-in malformed page: declared value count exceeds BYTE_STREAM_SPLIT payload.
+ let file = Bytes::from_static(include_bytes!("ARROW-RS-GH-11136-bss-truncated.parquet"));
+ let reader = ArrowReaderBuilder::try_new(file).unwrap().build().unwrap();
+ let err = reader.collect::<Result<Vec<_>, _>>().unwrap_err();
+ assert!(
+ err.to_string()
+ .contains("insufficient values read from column"),
Review Comment:
Why isn't this the new error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#issuecomment-6483789622
Fork Actions are enabled, and the PR branch includes current `main` (`70d8190`) so it has the current fork CI workflow. The change remains scoped to the same three Parquet files.
**Fork verification:** [full CI run for head `0393d7a`](https://github.com/jaideeppyne/arrow-rs/actions/runs/40788512934). The completed jobs are passing, including Parquet tests and Clippy, MSRV, Windows, and macOS. The cross-language Archery integration job is still running; the full run is not yet complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#issuecomment-6444913911
@jaideeppyne could you enable the CI on your fork
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "alamb (via GitHub) via github" <gi...@arrow.apache.org>.
alamb commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4480354841
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -149,6 +155,56 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_byte_stream_split_truncated_page() {
Review Comment:
how about we just check in the 488 byte bad file rahter than trying t manually re-create it each time?
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -86,9 +86,18 @@ fn join_streams_variable(
}
impl<T: DataType> Decoder<T> for ByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
Why not check that num_values is is consistent with `data.len() / type_size;`? It seems like by ignorning it we may be allowing other inconsistencies?
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -164,7 +173,7 @@ impl<T: DataType> VariableWidthByteStreamSplitDecoder<T> {
}
impl<T: DataType> Decoder<T> for VariableWidthByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
same comment as above
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -684,8 +684,49 @@ impl RepetitionLevelDecoder for RepetitionLevelDecoderImpl {
#[cfg(test)]
mod tests {
use super::*;
+ use crate::data_type::DoubleType;
use crate::encodings::rle::RleEncoder;
+ use crate::schema::types::{ColumnDescriptor, ColumnPath, Type as SchemaType};
use rand::{prelude::*, rng};
+ use std::sync::Arc;
+
+ #[test]
+ fn test_byte_stream_split_v1_nulls_use_encoded_len() {
Review Comment:
What is the value of this test? Does it add significant coverage over an end to end reproducer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jhorstmann (via GitHub) via github" <gi...@arrow.apache.org>.
jhorstmann commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4429109207
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2190,6 +2190,35 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ decoder.set_data(Bytes::from(vec![0; 64]), 63).unwrap();
Review Comment:
I think the check for `data.len()` being a multiple of type_size can stay in `set_data`, but the `num_values` parameter there can overestimate, since it is really the number of levels. Other decoders already have to treat it as an estimate. We only know how many values are really read inside `get` and the missing check there probably looks something like
```rust
if self.encoded_bytes.len() / type_size < self.values_decoded + num_values {
return Err(general_err!("Invalid BYTE_STREAM_SPLIT data length for {} values: {}", self.values_decoded + num_values, self.encoded_bytes.len()));
}
```
Or in `set_data` the `num_values` parameter could be ignored and the total instead calculated from `data.len()`, that should then also fail later when trying to read more than the available values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub) via github" <gi...@arrow.apache.org>.
jaideeppyne commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4514998393
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -149,6 +155,56 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_byte_stream_split_truncated_page() {
Review Comment:
Done in 0e75fbd. The 487 byte file is now checked in as `parquet/tests/arrow_reader/ARROW-RS-GH-11136-bss-truncated.parquet` (byte for byte the reproducer linked in the description) and the test reads it with `include_bytes!`, the same way `bigdelta.parquet` is used. The writer and header patching code is gone.
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -684,8 +684,49 @@ impl RepetitionLevelDecoder for RepetitionLevelDecoderImpl {
#[cfg(test)]
mod tests {
use super::*;
+ use crate::data_type::DoubleType;
use crate::encodings::rle::RleEncoder;
+ use crate::schema::types::{ColumnDescriptor, ColumnPath, Type as SchemaType};
use rand::{prelude::*, rng};
+ use std::sync::Arc;
+
+ #[test]
+ fn test_byte_stream_split_v1_nulls_use_encoded_len() {
Review Comment:
Not enough to justify it, so I removed it in 0e75fbd.
The case it pinned is a V1 nullable page where the level count is larger than the number of encoded values. That is already covered end to end: when I temporarily made `set_data` require `num_values == data.len() / type_size`, `arrow::arrow_reader::tests::column_reader::test_primitive_single_column_reader_test` and `arrow::arrow_writer::tests::statistics_null_counts_only_nulls` both failed. The corrupt count case is covered by the checked-in file.
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -86,9 +86,18 @@ fn join_streams_variable(
}
impl<T: DataType> Decoder<T> for ByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
Good point, ignoring it entirely was too loose. In 0e75fbd both decoders now check it as an upper bound: `set_data` returns an error if the page holds more encoded values than `num_values`.
It can't be strict equality. `ColumnValueDecoderImpl::set_data` passes `num_values.unwrap_or(num_levels)`. For V2 pages that is the non-null count, but for V1 pages it is the level count, which includes nulls (and for repeated columns every level), while only non-null values are encoded. With an equality check, reading ordinary nullable V1 pages fails; the two existing tests mentioned in the other thread fail that way.
The other direction, fewer encoded values than `num_values`, is the #11121 case. Since `values_left()` now comes from the encoded length, `get` and `skip` stop at the end of the streams and the column reader returns `insufficient values read from column`, instead of `join_streams_const` indexing past the buffer. That is also why a separate length check in `get` (the other option @jhorstmann mentioned) isn't needed.
##########
parquet/src/encodings/decoding/byte_stream_split_decoder.rs:
##########
@@ -164,7 +173,7 @@ impl<T: DataType> VariableWidthByteStreamSplitDecoder<T> {
}
impl<T: DataType> Decoder<T> for VariableWidthByteStreamSplitDecoder<T> {
- fn set_data(&mut self, data: Bytes, num_values: usize) -> Result<()> {
+ fn set_data(&mut self, data: Bytes, _num_values: usize) -> Result<()> {
Review Comment:
Same change here: `VariableWidthByteStreamSplitDecoder::set_data` uses the same upper bound check through a shared `check_encoded_value_count` helper, and `test_byte_stream_split_rejects_more_values_than_expected` covers both decoders.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub) via github" <gi...@arrow.apache.org>.
jaideeppyne commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4533020286
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
Review Comment:
The file-level test checks the error returned by the reader. This test checks the fixed-width decoder's read and skip counts directly. With eight encoded values, reading three and skipping two must leave exactly three, even when the page count is larger. The next oversized read returns those three and subsequent reads return zero. Reusing the decoder in the loop also checks that setting a new page resets its position.
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2175,6 +2175,70 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ for count in [8, 63, usize::MAX] {
+ decoder.set_data(Bytes::from(vec![0; 64]), count).unwrap();
+ assert_eq!(decoder.values_left(), 8);
+ let mut values = vec![0.; 63];
+ assert_eq!(decoder.get(&mut values[..3]).unwrap(), 3);
+ assert_eq!(decoder.skip(2).unwrap(), 2);
+ assert_eq!(decoder.get(&mut values).unwrap(), 3);
+ assert_eq!(decoder.values_left(), 0);
+ assert_eq!(decoder.get(&mut values).unwrap(), 0);
+ }
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_invalid_data_length() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ let err = decoder.set_data(Bytes::from(vec![0; 63]), 8).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data length 63 for values of size 8"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_rejects_more_values_than_expected() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+ let err = decoder.set_data(Bytes::from(vec![0; 64]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values exceeds the 7 values expected"
+ );
+
+ let mut decoder = VariableWidthByteStreamSplitDecoder::<FixedLenByteArrayType>::new(5);
+ let err = decoder.set_data(Bytes::from(vec![0; 40]), 7).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Parquet error: Invalid BYTE_STREAM_SPLIT data: 8 encoded values exceeds the 7 values expected"
+ );
+ }
+
+ #[test]
+ fn test_byte_stream_split_flba_uses_encoded_value_count() {
Review Comment:
This covers the same read and skip behavior in the separate FIXED_LEN_BYTE_ARRAY decoder. The file-level regression reads doubles and never enters this implementation. Width 5 uses the generic byte join, and width 16 uses the specialized join. Both must stop at the physical value count even when the page count is larger.
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -136,6 +136,19 @@ fn test_arrow_gh_47662() {
);
}
+#[test]
+fn test_arrow_rs_gh_11136_byte_stream_split_truncated_page() {
+ // Checked-in malformed page: declared value count exceeds BYTE_STREAM_SPLIT payload.
+ let file = Bytes::from_static(include_bytes!("ARROW-RS-GH-11136-bss-truncated.parquet"));
+ let reader = ArrowReaderBuilder::try_new(file).unwrap().build().unwrap();
+ let err = reader.collect::<Result<Vec<_>, _>>().unwrap_err();
+ assert!(
+ err.to_string()
+ .contains("insufficient values read from column"),
Review Comment:
The new `set_data` count error is for the opposite mismatch, where the payload contains more complete values than the supplied upper bound. This fixture has fewer. That direction can be valid on a nullable V1 page because the bound includes null levels. The decoder stops at the number of values actually present, and the column reader returns the insufficient-values error when it cannot satisfy the read. `test_byte_stream_split_rejects_more_values_than_expected` checks the new error in both decoder variants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jhorstmann (via GitHub)" <gi...@apache.org>.
jhorstmann commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4149696642
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2190,6 +2190,35 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ decoder.set_data(Bytes::from(vec![0; 64]), 63).unwrap();
Review Comment:
> BYTE_STREAM_SPLIT stores only non-null values
That is true of all parquet encodings, so I would assume it is handled correctly by other encoders. Maybe the length checks could be done better in `get` instead of `set_data`, at that point the caller provides a buffer that should be sized correctly for the non-null values.
A similar check is probably also needed in `VariableWidthByteStreamSplitDecoder`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "etseidl (via GitHub) via github" <gi...@arrow.apache.org>.
etseidl commented on PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#issuecomment-6644532212
Going to mark this draft as it's waiting on submitter input
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub) via github" <gi...@arrow.apache.org>.
jaideeppyne commented on PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#issuecomment-6646816689
@alamb, the panic is at `sub_src[i + j * stride]` in [`join_streams_const`](https://github.com/apache/arrow-rs/blob/32902415b841ac824470d8ee9ffd4b785d8b2a9a/parquet/src/encodings/decoding/byte_stream_split_decoder.rs#L68), reached through the public `ParquetRecordBatchReaderBuilder` when a page's value count exceeds its encoded payload.
Here are actual files and a runnable public-API program:
- [Malformed Parquet, 487 bytes](https://raw.githubusercontent.com/jaideeppyne/arrow-rs/3f70bb88cb0c1029e84fcd297fe6a742dc13500b/malformed-bss.parquet)
- [Valid control](https://raw.githubusercontent.com/jaideeppyne/arrow-rs/3f70bb88cb0c1029e84fcd297fe6a742dc13500b/valid-bss.parquet)
- [Reproducer source](https://raw.githubusercontent.com/jaideeppyne/arrow-rs/3f70bb88cb0c1029e84fcd297fe6a742dc13500b/reproduce.rs) and [run instructions](https://github.com/jaideeppyne/arrow-rs/blob/3f70bb88cb0c1029e84fcd297fe6a742dc13500b/README.md)
The writer creates eight doubles and the control reads all eight back. Only zero-based byte 14 changes, from 16 to 126, declaring 63 values while the payload and footer stay in place. [Verification](https://github.com/jaideeppyne/arrow-rs/actions/runs/41945965606) observed `index out of bounds: the len is 56 but the index is 56` on unchanged upstream. With b280e76, the identical file returns an insufficient-values error.
The update also follows @jhorstmann's suggestion: both decoder variants use the encoded count, and the column-reader workaround is removed. The new integration test exercises this file-construction path directly. Focused tests, Clippy, and formatting passed; full fork CI is running. This follow-up and its reproducer were prepared with OpenAI Codex assistance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4139994399
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2190,6 +2190,35 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ decoder.set_data(Bytes::from(vec![0; 64]), 63).unwrap();
Review Comment:
The decoder now returns an error when `data.len()` is smaller than `num_values * T::get_type_size()`, instead of ignoring `num_values` and decoding the encoded count. Commit 87bdd58e18c4e6e94ba21cf6c6f09078dc9f0545.
A short buffer (64 bytes with a declared count of 63 f64 values) now errors. Exact-size input still decodes. Oversized buffers keep the existing "expected at most" error.
Data page v1 does not report a non-null count, and BYTE_STREAM_SPLIT stores only non-null values, so the column reader passes the number of complete values in the buffer in that case. An explicit count (v2) is still checked and errors when the buffer is short.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jhorstmann (via GitHub)" <gi...@apache.org>.
jhorstmann commented on code in PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#discussion_r4053210689
##########
parquet/src/encodings/decoding.rs:
##########
@@ -2190,6 +2190,35 @@ mod tests {
test_byte_stream_split_decode::<Int64Type>(data, -1);
}
+ #[test]
+ fn test_byte_stream_split_uses_encoded_value_count() {
+ let mut decoder = ByteStreamSplitDecoder::<DoubleType>::new();
+
+ decoder.set_data(Bytes::from(vec![0; 64]), 63).unwrap();
Review Comment:
The decoder seems to silently ignore the `num_values` parameter here. I would expect it to return an error if `data.len()` is smaller than `num_values * T::get_type_size()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): prevent byte stream split decoder panic [arrow-rs]
Posted by "jaideeppyne (via GitHub) via github" <gi...@arrow.apache.org>.
jaideeppyne commented on PR #11136:
URL: https://github.com/apache/arrow-rs/pull/11136#issuecomment-6745011195
@alamb thanks for the review. 0e75fbd addresses all four comments, with replies inline:
- The malformed 487 byte file is checked in and read directly.
- The column reader unit test is removed; existing end to end tests already cover nullable V1 pages.
- Both BYTE_STREAM_SPLIT decoders treat the page value count as an upper bound and reject a page with more encoded values than declared. Strict equality would break nullable V1 pages, where the count includes nulls.
The branch also merges current `main` (30c5c32), so CI runs the current workflow. Locally `cargo test -p parquet --lib` (1511 passed), `cargo test -p parquet --test arrow_reader` (147 passed), `cargo clippy -p parquet --all-targets --all-features -- -D warnings` and `cargo fmt` are clean. Prepared with AI assistance. Ready for another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org