You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2024/08/15 10:53:48 UTC
[I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
mapleFU opened a new issue, #6255:
URL: https://github.com/apache/arrow-rs/issues/6255
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
<!--
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
(This section helps Arrow developers understand the context and *why* for this feature, in addition to the *what*)
-->
See: https://github.com/apache/parquet-testing/pull/58#issuecomment-2290985490
When reading a corrupt file, currently, arrow-rs would have:
```rust
/// Returns the offset and length in bytes of the column chunk within the file
pub fn byte_range(&self) -> (u64, u64) {
let col_start = match self.dictionary_page_offset() {
Some(dictionary_page_offset) => dictionary_page_offset,
None => self.data_page_offset(),
};
let col_len = self.compressed_size();
assert!(
col_start >= 0 && col_len >= 0,
"column start and length should not be negative"
);
(col_start as u64, col_len as u64)
}
```
Would we better check the range here?
**Describe the solution you'd like**
<!--
A clear and concise description of what you want to happen.
-->
Checking the range when building the group reader or in "byte_range()"
**Describe alternatives you've considered**
<!--
A clear and concise description of any alternative solutions or features you've considered.
-->
```rust
/// Returns the offset and length in bytes of the column chunk within the file
pub fn byte_range(&self) -> Result<(u64, u64)> {
let col_start = match self.dictionary_page_offset() {
Some(dictionary_page_offset) => dictionary_page_offset,
None => self.data_page_offset(),
};
let col_len = self.compressed_size();
if col_len < 0 || col_len < 0{
return Err(ParquetError::General(
"column start and length should not be negative".to_string(),
));
}
(col_start as u64, col_len as u64)
}
```
**Additional context**
<!--
Add any other context or screenshots about the feature request here.
-->
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6255:
URL: https://github.com/apache/arrow-rs/issues/6255#issuecomment-2292276409
> @alamb I'm willing to try this but I'm not so familar with parquet-rs. Do you think this would better be checked in `ColumnChunkMetaData::byte_range` or `ColumnChunkMetaData::from_thrift`? Or this is already checked, we don't require this?
I am not sure -- I think the first thing we should do is get a reproducer. Let me see if I can whip up some tests
--
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: [I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on issue #6255:
URL: https://github.com/apache/arrow-rs/issues/6255#issuecomment-2291069590
@alamb I'm willing to try this but I'm not so familar with parquet-rs. Do you think this would better be checked in `ColumnChunkMetaData::byte_range` or `ColumnChunkMetaData::from_thrift`? Or this is already checked, we don't require this?
--
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: [I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6255:
URL: https://github.com/apache/arrow-rs/issues/6255#issuecomment-2292357527
> Describe alternatives you've considered
That looks good to me, FWIW
--
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: [I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6255:
URL: https://github.com/apache/arrow-rs/issues/6255#issuecomment-2292355902
See https://github.com/apache/arrow-rs/issues/6261 / https://github.com/apache/arrow-rs/pull/6262
--
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: [I] Check valid byte_range in parquet Column Chunk reading? [arrow-rs]
Posted by "elshanhsynv (via GitHub) via github" <gi...@arrow.apache.org>.
elshanhsynv commented on issue #6255:
URL: https://github.com/apache/arrow-rs/issues/6255#issuecomment-6726378424
This still panics on 60.1.0 (`file/metadata/mod.rs:1043`), and we hit it while fuzzing a Parquet viewer with libFuzzer, so it is reachable from untrusted files. In case a deterministic reproducer helps, this rewrites a valid file's footer so column 0 has a negative `data_page_offset`:
```toml
[dependencies]
parquet = { version = "60.1.0", features = ["arrow"] }
arrow-array = "60.1.0"
bytes = "1"
```
```rust
use std::sync::Arc;
use arrow_array::{Int32Array, RecordBatch};
use bytes::Bytes;
use parquet::{
arrow::{ArrowWriter, arrow_reader::ParquetRecordBatchReaderBuilder},
file::metadata::{ParquetMetaDataReader, ParquetMetaDataWriter},
};
fn main() {
let column = Int32Array::from(vec![1, 2, 3]);
let batch = RecordBatch::try_from_iter([("i", Arc::new(column) as _)]).unwrap();
let mut bytes = Vec::new();
let mut writer = ArrowWriter::try_new(&mut bytes, batch.schema(), None).unwrap();
writer.write(&batch).unwrap();
writer.close().unwrap();
let bytes = Bytes::from(bytes);
let metadata = ParquetMetaDataReader::new().parse_and_finish(&bytes).unwrap();
let mut builder = metadata.into_builder();
let mut row_groups = builder.take_row_groups();
let mut group = row_groups.remove(0).into_builder();
let mut columns = group.take_columns();
let first = columns
.remove(0)
.into_builder()
.set_dictionary_page_offset(None)
.set_data_page_offset(-100)
.build()
.unwrap();
columns.insert(0, first);
group = group.set_column_metadata(columns);
let metadata = builder.set_row_groups(vec![group.build().unwrap()]).build();
// Keep the data pages, replace the footer.
let footer_len =
u32::from_le_bytes(bytes[bytes.len() - 8..bytes.len() - 4].try_into().unwrap()) as usize;
let mut forged = bytes[..bytes.len() - 8 - footer_len].to_vec();
ParquetMetaDataWriter::new(&mut forged, &metadata).finish().unwrap();
let reader = ParquetRecordBatchReaderBuilder::try_new(Bytes::from(forged))
.unwrap()
.build()
.unwrap();
for batch in reader {
println!("{:?}", batch.map(|b| b.num_rows())); // expected: Err(...), actual: panic
}
}
```
```
thread 'main' panicked at parquet-60.1.0/src/file/metadata/mod.rs:1043:9:
column start and length should not be negative
```
Note that `ParquetMetaDataReader` accepts the negative offset and the panic only happens later, when the column chunk is read. Validating in `ColumnChunkMetaData::from_thrift` (or when building it) would turn this into an error at open time.
--
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