Skip to content

Prevent FixedSizeBinaryArray i32 offset overflows (try 2) #9872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 5 commits into from
May 4, 2026
Merged

Prevent FixedSizeBinaryArray i32 offset overflows (try 2) #9872

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
701436e
Prevent FixedSizeBinaryArray offset overflows
alamb May 1, 2026
1152dbc
Check other paths, add errors
alamb May 1, 2026
b4ea0e8
Adjust bounds
alamb May 1, 2026
e47512a
A few more potential overflows
alamb May 1, 2026
4d0c5e0
[arrow-array] revert naming-only size changes
alamb May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 107 additions & 13 deletions arrow-array/src/array/fixed_size_binary_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ impl FixedSizeBinaryArray {
/// Create a new [`Scalar`] from `value`
pub fn new_scalar(value: impl AsRef<[u8]>) -> Scalar<Self> {
let v = value.as_ref();
Scalar::new(Self::new(v.len() as _, Buffer::from(v), None))
let size = i32::try_from(v.len()).expect("FixedSizeBinaryArray value length exceeds i32");
Scalar::new(Self::new(size, Buffer::from(v), None))
}

/// Create a new [`FixedSizeBinaryArray`] from the provided parts, returning an error on failure
Expand All @@ -84,6 +85,7 @@ impl FixedSizeBinaryArray {
/// * `size < 0`
/// * `values.len() / size != nulls.len()`
/// * `size == 0 && values.len() != 0`
/// * `len * size > i32::MAX`
pub fn try_new(
size: i32,
values: Buffer,
Expand Down Expand Up @@ -120,6 +122,8 @@ impl FixedSizeBinaryArray {
}
};

Self::validate_lengths(s, len)?;

@alamb alamb May 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the key check -- it prevents creating arrays that are susceptible to this overflow

I added it on each constructor path


Ok(Self {
data_type,
value_data: values,
Expand All @@ -129,6 +133,33 @@ impl FixedSizeBinaryArray {
})
}

/// Some calculations below use i32 arithmetic which can overflow when
/// valid offsets are past i32::MAX. Until that is solved for real do not
/// permit constructing any FixedSizeBinaryArray that has a valid offset
/// past i32::MAX
fn validate_lengths(value_size: usize, len: usize) -> Result<(), ArrowError> {
// the offset is also calculated for the next element (i + 1) so
// check `len` (not last element index) to ensure that all offsets are valid
let max_offset = value_size.checked_mul(len).ok_or_else(|| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray error: value size {value_size} * len {len} exceeds maximum valid offset"
))
})?;

let max_valid_offset: usize = i32::MAX.try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray error: maximum valid offset exceeds i32::MAX, got {max_offset}"
))
})?;

if max_offset > max_valid_offset {
return Err(ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray error: value size {value_size} * length {len} exceeds maximum valid offset of {max_valid_offset}"
)));
};
Ok(())
}

/// Create a new [`FixedSizeBinaryArray`] of length `len` where all values are null
///
/// # Panics
Expand All @@ -137,12 +168,17 @@ impl FixedSizeBinaryArray {
///
/// * `size < 0`
/// * `size * len` would overflow `usize`
/// * `size * len > i32::MAX`
/// * `size * len * 8` would overflow `usize`
pub fn new_null(size: i32, len: usize) -> Self {
const BITS_IN_A_BYTE: usize = 8;
let capacity_in_bytes = size.to_usize().unwrap().checked_mul(len).unwrap();
let size_usize = size.to_usize().unwrap();
Self::validate_lengths(size_usize, len).unwrap();
let capacity_in_bytes = size_usize.checked_mul(len).unwrap();
let capacity_in_bits = capacity_in_bytes.checked_mul(BITS_IN_A_BYTE).unwrap();
Self {
data_type: DataType::FixedSizeBinary(size),
value_data: MutableBuffer::new_null(capacity_in_bytes * BITS_IN_A_BYTE).into(),
value_data: MutableBuffer::new_null(capacity_in_bits).into(),
nulls: Some(NullBuffer::new_null(len)),
value_length: size,
len,
Expand Down Expand Up @@ -310,8 +346,16 @@ impl FixedSizeBinaryArray {
// Now that we know how large each element is we can reserve
// sufficient capacity in the underlying mutable buffer for
// the data.
buffer.reserve(iter_size_hint * len);
buffer.extend_zeros(slice.len() * prepend);
if let Some(capacity) = iter_size_hint.checked_mul(len) {

@alamb alamb May 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated some of the other arithmetic to used checked multiplication

buffer.reserve(capacity);
}
let prepend_zeros = slice.len().checked_mul(prepend).ok_or_else(|| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray error: value size {} * prepend {prepend} exceeds usize",
slice.len()
))
})?;
buffer.extend_zeros(prepend_zeros);
}
bit_util::set_bit(null_buf.as_slice_mut(), len);
buffer.extend_from_slice(slice);
Expand All @@ -334,7 +378,13 @@ impl FixedSizeBinaryArray {

let nulls = NullBuffer::from_unsliced_buffer(null_buf, len);

let size = size.unwrap_or(0) as i32;
let size = size.unwrap_or(0);
Self::validate_lengths(size, len)?;
let size = size.try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray value length exceeds i32, got {size}"
))
})?;
Ok(Self {
data_type: DataType::FixedSizeBinary(size),
value_data: buffer.into(),
Expand Down Expand Up @@ -373,12 +423,20 @@ impl FixedSizeBinaryArray {
T: Iterator<Item = Option<U>>,
U: AsRef<[u8]>,
{
let size_usize = size.to_usize().ok_or_else(|| {
ArrowError::InvalidArgumentError(format!("Size cannot be negative, got {size}"))
})?;
let mut len = 0;
let mut byte = 0;

let iter_size_hint = iter.size_hint().0;
let mut null_buf = MutableBuffer::new(bit_util::ceil(iter_size_hint, 8));
let mut buffer = MutableBuffer::new(iter_size_hint * (size as usize));
let capacity = iter_size_hint.checked_mul(size_usize).ok_or_else(|| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray error: value size {size_usize} * len hint {iter_size_hint} exceeds usize"
))
})?;
let mut buffer = MutableBuffer::new(capacity);

iter.try_for_each(|item| -> Result<(), ArrowError> {
// extend null bitmask by one byte per each 8 items
Expand All @@ -390,7 +448,7 @@ impl FixedSizeBinaryArray {

if let Some(slice) = item {
let slice = slice.as_ref();
if size as usize != slice.len() {
if size_usize != slice.len() {
return Err(ArrowError::InvalidArgumentError(format!(
"Nested array size mismatch: one is {}, and the other is {}",
size,
Expand All @@ -401,7 +459,7 @@ impl FixedSizeBinaryArray {
bit_util::set_bit(null_buf.as_slice_mut(), len);
buffer.extend_from_slice(slice);
} else {
buffer.extend_zeros(size as usize);
buffer.extend_zeros(size_usize);
}

len += 1;
Expand All @@ -410,6 +468,7 @@ impl FixedSizeBinaryArray {
})?;

let nulls = NullBuffer::from_unsliced_buffer(null_buf, len);
Self::validate_lengths(size_usize, len)?;

Ok(Self {
data_type: DataType::FixedSizeBinary(size),
Expand Down Expand Up @@ -460,7 +519,9 @@ impl FixedSizeBinaryArray {
} else {
let len = slice.len();
size = Some(len);
buffer.reserve(iter_size_hint * len);
if let Some(capacity) = iter_size_hint.checked_mul(len) {
buffer.reserve(capacity);
}
}

buffer.extend_from_slice(slice);
Expand All @@ -476,7 +537,13 @@ impl FixedSizeBinaryArray {
));
}

let size = size.unwrap_or(0).try_into().unwrap();
let size = size.unwrap_or(0);
Self::validate_lengths(size, len)?;
let size = size.try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!(
"FixedSizeBinaryArray value length exceeds i32, got {size}"
))
})?;
Ok(Self {
data_type: DataType::FixedSizeBinary(size),
value_data: buffer.into(),
Expand Down Expand Up @@ -511,8 +578,15 @@ impl From<ArrayData> for FixedSizeBinaryArray {
_ => panic!("Expected data type to be FixedSizeBinary"),
};

let size = value_length as usize;

@alamb alamb May 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is another unchecked conversion from value_lengh to usize

let value_data = buffers[0].slice_with_length(offset * size, len * size);
let size = value_length
.to_usize()
.expect("FixedSizeBinaryArray value length must be non-negative");
Self::validate_lengths(size, len)
.expect("FixedSizeBinaryArray offsets must fit within i32");
let value_data = buffers[0].slice_with_length(
offset.checked_mul(size).expect("offset overflow"),
len.checked_mul(size).expect("length overflow"),
);

Self {
data_type,
Expand Down Expand Up @@ -1003,6 +1077,26 @@ mod tests {
array.value(4);
}

#[test]
fn test_validate_lengths_allows_empty_array() {
FixedSizeBinaryArray::validate_lengths(1024, 0).unwrap();
}

#[test]
fn test_validate_lengths_allows_i32_max_offset() {
FixedSizeBinaryArray::validate_lengths(1, i32::MAX as usize).unwrap();
FixedSizeBinaryArray::validate_lengths(262_176, 8191).unwrap();
}

#[test]
fn test_validate_lengths_rejects_offset_past_i32_max() {
let err = FixedSizeBinaryArray::validate_lengths(262_177, 8192).unwrap_err();
assert_eq!(
err.to_string(),
"Invalid argument error: FixedSizeBinaryArray error: value size 262177 * length 8192 exceeds maximum valid offset of 2147483647",
);
}

#[test]
fn test_constructors() {
let buffer = Buffer::from_vec(vec![0_u8; 10]);
Expand Down
Loading