You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "PlenoraETL (via GitHub)" <gi...@apache.org> on 2026/09/08 08:23:03 UTC
[PR] fix(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
PlenoraETL opened a new pull request, #11020:
URL: https://github.com/apache/arrow-rs/pull/11020
# Which issue does this PR close?
Closes #11019.
# Rationale for this change
`DictionaryBatch.data` is optional in the flatbuffer grammar and required by the format. `get_dictionary_values` opened it with `unwrap`, so a file that omits it panicked instead of producing an `ArrowError`.
This is the same class of field, in the same file, that the footer's custom metadata was just fixed for:
```rust
let (Some(key), Some(value)) = (kv.key(), kv.value()) else {
return Err(ArrowError::ParseError(
"Custom metadata in the IPC footer is missing a key or a value".to_string(),
));
};
```
The patch uses that shape. The proposal is essentially "finish the round".
A panic matters more than usual on this path: under `libfuzzer-sys` a caught panic still becomes `abort()` before unwinding, so a fuzz target reading untrusted IPC goes into quarantine rather than reporting a rejected input.
# What changes are included in this PR?
One `let ... else` in `get_dictionary_values`, plus a regression test.
Measured before and after on `main`: the fixture panics without the change and returns `Parser error: Dictionary batch is missing its data` with it. `cargo test -p arrow-ipc` passes — 132, 7 and 11.
# Are these changes tested?
Yes. The fixture is the original 2169-byte input from the fuzzing corpus of [plenora-IO-tools](https://github.com/PlenoraETL/plenora-IO-tools), not a reduced one, added under `arrow-ipc/test/data/`.
# Are there any user-facing changes?
A file that used to panic now returns an `ArrowError::ParseError`. No API change.
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#discussion_r3957868568
##########
arrow-ipc/src/reader.rs:
##########
@@ -923,9 +923,18 @@ fn get_dictionary_values(
let value = value_type.as_ref().clone();
let schema = Schema::new(vec![Field::new("", value, true)]);
// Read a single column
+ // `DictionaryBatch.data` is optional in the flatbuffer grammar and
+ // required by the format, so a message that omits it panics here
+ // rather than producing an error. This mirrors the check the footer
+ // metadata already got: report it, don't unwrap it.
Review Comment:
```suggestion
```
these LLM comments are really not necessary
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not a
+ /// reduced one.
Review Comment:
```suggestion
```
same here
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not a
+ /// reduced one.
+ #[test]
+ fn dictionary_batch_without_data_is_an_error_not_a_panic() {
+ let bytes: &[u8] = include_bytes!("../test/data/dictionary_batch_without_data.arrow");
+ let err = crate::reader::FileReader::try_new(std::io::Cursor::new(bytes), None)
Review Comment:
please move this test down; it shouldnt be above the imports
##########
arrow-ipc/test/data/dictionary_batch_without_data.arrow:
##########
Review Comment:
could we refactor the test to somehow create this data in memory as part of the test? instead of commiting a separate file
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not a
+ /// reduced one.
+ #[test]
+ fn dictionary_batch_without_data_is_an_error_not_a_panic() {
Review Comment:
```suggestion
fn test_invalid_dictionary_batch_without_data() {
```
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "albertlockett (via GitHub)" <gi...@apache.org>.
albertlockett commented on code in PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#discussion_r3960310115
##########
arrow-ipc/src/reader.rs:
##########
@@ -2197,6 +2202,53 @@ mod tests {
);
}
+ #[test]
+ fn test_invalid_dictionary_batch_without_data() {
+ use crate::r#gen::Message::*;
+ use flatbuffers::FlatBufferBuilder;
+
+ #[allow(deprecated)]
+ let schema = Schema::new(vec![Field::new_dict(
+ "col",
+ DataType::Dictionary(Box::new(DataType::Int8), Box::new(DataType::Utf8)),
+ true,
+ 0,
+ false,
+ )]);
Review Comment:
Is using the deprecated `Field` constructor necessary here?
```suggestion
let schema = Schema::new(vec![Field::new(
"col",
DataType::Dictionary(Box::new(DataType::Int8), Box::new(DataType::Utf8)),
true,
)]);
```
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "PlenoraETL (via GitHub)" <gi...@apache.org>.
PlenoraETL commented on PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#issuecomment-5585739393
Thanks for the review — all five points addressed, branch force-pushed.
- Dropped the comment above the fix and the doc comment on the test.
- Renamed to `test_invalid_dictionary_batch_without_data` and moved it down, next to `test_missing_buffer_metadata_error`.
- Removed the committed `.arrow` file: the test now builds the malformed `DictionaryBatch` in memory with `FlatBufferBuilder`, following the shape of `test_missing_buffer_metadata_error`. It calls `read_dictionary` directly, so there's no file and no byte-patching involved.
`cargo test -p arrow-ipc` passes (132 / 7 / 11) and clippy is clean.
One note in case it matters to you: I had to `#[allow(deprecated)]` on `Field::new_dict`, since `read_dictionary` looks the field up by dict id and that's the constructor that sets one. Happy to build the schema differently if there's a preferred way now that dict ids are on their way out.
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#issuecomment-5593796069
thanks @PlenoraETL & @albertlockett
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "PlenoraETL (via GitHub)" <gi...@apache.org>.
PlenoraETL commented on PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#issuecomment-5594158988
thank you too @Jefffrey for your work
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020
--
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(arrow-ipc): return an error for a DictionaryBatch without its data [arrow-rs]
Posted by "PlenoraETL (via GitHub)" <gi...@apache.org>.
PlenoraETL commented on PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#issuecomment-5593105447
Good catch — no, it wasn't necessary. `Field::new` works here: `dict_id` defaults to 0, which is what `fields_with_dict_id(0)` looks for, so the deprecated constructor was only adding noise (and an `#[allow]`). Applied your suggestion and dropped the `#[allow(deprecated)]` with it.
`cargo test -p arrow-ipc` still passes (132 / 7 / 11) and clippy is clean.
--
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