This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new f74d9bf054 Return errors for undersized ArrayData validity buffers
(#11044)
f74d9bf054 is described below
commit f74d9bf05409ba56ee43314489688f0ec53e7b80
Author: Yifan Chen <[email protected]>
AuthorDate: Thu Sep 17 21:02:40 2026 -0700
Return errors for undersized ArrayData validity buffers (#11044)
# Which issue does this PR close?
Closes #7124.
# Rationale for this change
Validated IPC decoding uses `ArrayDataBuilder`, which constructs a
`BooleanBuffer` before checking whether the validity buffer is large
enough. Malformed input can therefore panic instead of returning an
`ArrowError`. `ArrayData::try_new` already performs the required check,
but its direct builder callers do not benefit from it.
# What changes are included in this PR?
Move the existing bounds check into the builder's validated path, before
`BooleanBuffer` construction. Keep the error wording, checked
length-plus-offset arithmetic, and unsafe skip-validation behavior.
Builder tests cover a short validity buffer with and without an offset,
two valid boundary controls, and length-plus-offset overflow. The public
`StreamReader` regression uses the original #7124 reproduction,
serializing a deliberately malformed array. Review follow-ups remove the
hand-encoded IPC fixture and null-count/validity permutations, and place
the stream regression in the existing reader unit tests. Production
logic is unchanged.
# Are these changes tested?
The short-buffer, overflow and IPC regressions were verified to panic
without the production fix and pass with it. After moving the unchanged
stream regression into `arrow-ipc/src/reader.rs`, validation on Rust
1.98.0 / macOS aarch64:
- `cargo test --locked -p arrow-ipc --lib reader::tests --all-features`:
53 passed, including the moved regression.
- `cargo clippy --locked -p arrow-ipc --lib --tests --all-features -- -D
warnings`, workspace formatting, touched-file Typos and diff whitespace
checks pass.
- The preceding test-simplification revision passed all 60 arrow-data
library tests and 4 focused all-feature builder tests. Builder
source/tests are unchanged by this move; those commands were not
repeated.
Explicitly forcing `arrow-data/force_validate` and
`arrow-array/force_validate` into the IPC test rejects its deliberately
invalid array during fixture construction, before decoding. That
combination cannot run this original-issue reproduction. The earlier
hand-encoded fixture did support it; the limitation is recorded rather
than silently skipping the test.
The broader workspace, release-mode and separate all-feature crate
suites passed before this test-only simplification; those full suites
were not rerun for the review edit. Shared-host benchmark measurements
were inconclusive, so no performance conclusion is claimed.
# Are there any user-facing changes?
An undersized validity buffer now produces a recoverable error through
the validated builder/IPC path instead of a panic. No public API
signature changes.
Implementation and regression tests generated with OpenAI Codex.
---
arrow-data/src/data.rs | 86 +++++++++++++++++++++++++++++++++++++++----------
arrow-ipc/src/reader.rs | 24 ++++++++++++++
2 files changed, 93 insertions(+), 17 deletions(-)
diff --git a/arrow-data/src/data.rs b/arrow-data/src/data.rs
index b911416506..e4e69ef2e4 100644
--- a/arrow-data/src/data.rs
+++ b/arrow-data/src/data.rs
@@ -333,21 +333,6 @@ impl ArrayData {
buffers: Vec<Buffer>,
child_data: Vec<ArrayData>,
) -> Result<Self, ArrowError> {
- // we must check the length of `null_bit_buffer` first
- // because we use this buffer to calculate `null_count`
- // in `ArrayDataBuilder::build`.
- if let Some(null_bit_buffer) = null_bit_buffer.as_ref() {
- let len_plus_offset = checked_len_plus_offset(&data_type, len,
offset)?;
- let needed_len = bit_util::ceil(len_plus_offset, 8);
- if null_bit_buffer.len() < needed_len {
- return Err(ArrowError::InvalidArgumentError(format!(
- "null_bit_buffer size too small. got {} needed {}",
- null_bit_buffer.len(),
- needed_len
- )));
- }
- }
-
let builder = Self::inner_new_builder(
data_type,
len,
@@ -2331,6 +2316,21 @@ impl ArrayDataBuilder {
skip_validation,
} = self;
+ // SAFETY: `skip_validation` is only set to true using `unsafe` APIs.
+ let validate = !skip_validation.get() || cfg!(feature =
"force_validate");
+ if validate && let Some(buffer) = null_bit_buffer.as_ref() {
+ // Check before constructing the BooleanBuffer, which would
otherwise panic.
+ let len_plus_offset = checked_len_plus_offset(&data_type, len,
offset)?;
+ let needed_len = bit_util::ceil(len_plus_offset, 8);
+ if buffer.len() < needed_len {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "null_bit_buffer size too small. got {} needed {}",
+ buffer.len(),
+ needed_len
+ )));
+ }
+ }
+
let nulls = nulls
.or_else(|| {
let buffer = null_bit_buffer?;
@@ -2358,8 +2358,7 @@ impl ArrayDataBuilder {
data.align_buffers();
}
- // SAFETY: `skip_validation` is only set to true using `unsafe` APIs
- if !skip_validation.get() || cfg!(feature = "force_validate") {
+ if validate {
data.validate_data()?;
}
Ok(data)
@@ -2954,6 +2953,59 @@ mod tests {
);
}
+ #[test]
+ fn test_builder_rejects_short_null_bit_buffer() {
+ for (len, offset) in [(8000, 0), (8, 1)] {
+ let err = ArrayData::builder(DataType::Int32)
+ .len(len)
+ .offset(offset)
+ .add_buffer(make_i32_buffer(len + offset))
+ .null_bit_buffer(Some(Buffer::from([0_u8])))
+ .build()
+ .unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ format!(
+ "Invalid argument error: null_bit_buffer size too small.
got 1 needed {}",
+ bit_util::ceil(len + offset, 8)
+ )
+ );
+ }
+ }
+
+ #[test]
+ fn test_builder_null_bit_buffer_length_overflow() {
+ let err = ArrayData::builder(DataType::Int32)
+ .len(usize::MAX)
+ .offset(1)
+ .null_bit_buffer(Some(Buffer::default()))
+ .build()
+ .unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ format!(
+ "Invalid argument error: Length {} with offset 1 overflows
usize for Int32",
+ usize::MAX
+ )
+ );
+ }
+
+ #[test]
+ fn test_builder_accepts_valid_null_bit_buffer() {
+ for (len, offset) in [(8, 0), (7, 1)] {
+ let data = ArrayData::builder(DataType::Int32)
+ .len(len)
+ .offset(offset)
+ .add_buffer(make_i32_buffer(len + offset))
+ .null_bit_buffer(Some(Buffer::from([0_u8])))
+ .build()
+ .unwrap();
+ assert_eq!(data.len(), len);
+ assert_eq!(data.offset(), offset);
+ assert_eq!(data.null_count(), len);
+ }
+ }
+
#[test]
fn test_count_nulls() {
let buffer = Buffer::from([0b00010110, 0b10011111]);
diff --git a/arrow-ipc/src/reader.rs b/arrow-ipc/src/reader.rs
index 52a026863b..c340649123 100644
--- a/arrow-ipc/src/reader.rs
+++ b/arrow-ipc/src/reader.rs
@@ -3586,6 +3586,30 @@ mod tests {
assert_eq!(batch, roundtrip_batch);
}
+ #[test]
+ fn test_stream_reader_rejects_short_validity_buffer() {
+ // Reproduce #7124: serialize an Int32Array with too few validity bits.
+ let data = ArrayDataBuilder::new(DataType::Int32)
+ .len(8000)
+ .add_buffer(ScalarBuffer::<i32>::from_iter(0..8000).into())
+ .nulls(Some(NullBuffer::from(&[true, false, true, false])));
+ let array: ArrayRef = unsafe {
Arc::new(Int32Array::from(data.build_unchecked())) };
+ let batch = RecordBatch::try_from_iter([("a", array)]).unwrap();
+
+ let mut stream = Vec::new();
+ let mut writer =
+ crate::writer::StreamWriter::try_new(&mut stream,
&batch.schema()).unwrap();
+ writer.write(&batch).unwrap();
+ writer.finish().unwrap();
+
+ let mut reader = StreamReader::try_new(Cursor::new(stream),
None).unwrap();
+ let err = reader.next().unwrap().unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Invalid argument error: null_bit_buffer size too small. got 1
needed 1000"
+ );
+ }
+
#[test]
fn test_invalid_struct_array_ipc_read_errors() {
let a_field = Field::new("a", DataType::Int32, false);
