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 432fe12bf7 fix: stop `try_` functions from panicking (#10730)
432fe12bf7 is described below
commit 432fe12bf7a9b7e34ede8168b6dc30dc69c5d8e8
Author: Emil Ernerfeldt <[email protected]>
AuthorDate: Fri Aug 21 21:35:21 2026 -0700
fix: stop `try_` functions from panicking (#10730)
# Which issue does this PR close?
* Part of https://github.com/apache/arrow-rs/issues/10553
* Follow-up to https://github.com/apache/arrow-rs/pull/10656
# Rationale for this change
A `try_` function that returns a `Result` but still panics is the worst
of both worlds: the caller writes error handling and gets a panic
anyway.
# What changes are included in this PR?
* `FFI_ArrowSchema::try_new`: an interior nul byte in `format` was an
`unwrap`, and the error path leaked the children.
* `MutableArrayData::try_extend`: validates the array index and the
`start..end` range, which used to underflow.
* `MutableArrayData::try_extend_nulls`: errors instead of panicking when
there is no null buffer.
* `VariantArray::try_value`: errors on an out of bounds index, and on
conflicting `value`/`typed_value`.
`MutableBuffer::try_from_trusted_len_iter` still panics on a bad
`size_hint`: that is part of its `unsafe` contract, and the docs now say
so.
# Are these changes tested?
Yes, a test per error path.
# Are there any user-facing changes?
Panics turn into errors. `MutableArrayData::extend`/`extend_nulls` still
panic as before.
---------
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
arrow-arith/src/arity.rs | 6 +-
arrow-buffer/src/buffer/mutable.rs | 7 ++
arrow-data/src/transform/mod.rs | 116 ++++++++++++++++++++++-----
arrow-schema/src/ffi.rs | 28 +++++--
arrow/tests/array_transform.rs | 9 ++-
parquet-variant-compute/src/variant_array.rs | 51 +++++++++---
6 files changed, 174 insertions(+), 43 deletions(-)
diff --git a/arrow-arith/src/arity.rs b/arrow-arith/src/arity.rs
index 42aa5b0889..b1256e0a70 100644
--- a/arrow-arith/src/arity.rs
+++ b/arrow-arith/src/arity.rs
@@ -247,10 +247,10 @@ where
///
/// Like [`try_unary`] the function is only evaluated for non-null indices
///
-/// # Error
+/// # Errors
///
-/// Return an error if the arrays have different lengths or
-/// the operation is under erroneous
+/// Returns an error if the arrays have different lengths,
+/// or if the operation returns one.
pub fn try_binary<A: ArrayAccessor, B: ArrayAccessor, F, O>(
a: A,
b: B,
diff --git a/arrow-buffer/src/buffer/mutable.rs
b/arrow-buffer/src/buffer/mutable.rs
index f0b45304f8..1dcfaf67f4 100644
--- a/arrow-buffer/src/buffer/mutable.rs
+++ b/arrow-buffer/src/buffer/mutable.rs
@@ -1105,8 +1105,15 @@ impl MutableBuffer {
/// if any of the items of the iterator is an error.
/// Prefer this to `collect` whenever possible, as it is faster ~60%
faster.
///
+ /// # Errors
+ ///
+ /// Returns the first error yielded by the iterator.
+ ///
/// # Panics
///
+ /// Note that unlike the [`Err`] cases, these panics are violations of the
safety contract
+ /// below, and are only checks that happen to be cheap enough to keep:
+ ///
/// Panics if the iterator does not report an upper bound via `size_hint`,
or if the
/// reported length does not match the number of items produced before an
error-free finish,
/// or if allocating the required buffer fails for the same reasons as
diff --git a/arrow-data/src/transform/mod.rs b/arrow-data/src/transform/mod.rs
index 6571e0b658..7558d1e887 100644
--- a/arrow-data/src/transform/mod.rs
+++ b/arrow-data/src/transform/mod.rs
@@ -136,9 +136,8 @@ fn build_extend_null_bits(array: &ArrayData, use_nulls:
bool) -> ExtendNullBits<
pub struct MutableArrayData<'a> {
/// Input arrays: the data being read FROM.
///
- /// Note this is "dead code" because all actual references to the arrays
are
- /// stored in closures for extending values and nulls.
- #[expect(dead_code)]
+ /// Note all actual reads of the arrays go through the closures for
extending
+ /// values and nulls; these references are only kept for bounds checking.
arrays: Vec<&'a ArrayData>,
/// In progress output array: The data being written TO
@@ -768,13 +767,23 @@ impl<'a> MutableArrayData<'a> {
/// * `end` - the end index of the chunk (exclusive)
///
/// # Errors
- /// Returns an error if offset arithmetic overflows the underlying integer
type.
- ///
- /// # Panics
- /// This function panics if there is an invalid index,
- /// i.e. `index` >= the number of source arrays
- /// or `end` > the length of the `index`th array
+ /// Returns an error if
+ /// * `index` >= the number of source arrays,
+ /// * `start..end` is not a valid range within the `index`th array, or
+ /// * offset arithmetic overflows the underlying integer type.
pub fn try_extend(&mut self, index: usize, start: usize, end: usize) ->
Result<(), ArrowError> {
+ let Some(array_len) = self.arrays.get(index).map(|array| array.len())
else {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Source array index {index} is out of bounds: there are {}
source arrays",
+ self.arrays.len()
+ )));
+ };
+ if end < start || array_len < end {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Invalid range {start}..{end} for source array {index} of
length {array_len}"
+ )));
+ }
+
let len = end - start;
(self.extend_null_bits[index])(&mut self.data, start, len);
// Snapshot buffer lengths before attempting the extend so we can roll
@@ -795,17 +804,16 @@ impl<'a> MutableArrayData<'a> {
/// Extends the in progress array with a region of the input arrays.
///
/// # Panics
- /// This function panics if there is an invalid index,
- /// i.e. `index` >= the number of source arrays,
- /// `end` > the length of the `index`th array,
- /// or the offset type overflows (e.g. more than 2 GiB in a `StringArray`).
+ /// This function panics if
+ /// * `index` >= the number of source arrays,
+ /// * `start..end` is not a valid range within the `index`th array, or
+ /// * the offset type overflows (e.g. more than 2 GiB in a `StringArray`).
#[deprecated(
since = "59.0.0",
note = "Use `try_extend` which returns an error on overflow instead of
panicking"
)]
pub fn extend(&mut self, index: usize, start: usize, end: usize) {
- self.try_extend(index, start, end)
- .expect("extend failed due to offset overflow")
+ self.try_extend(index, start, end).expect("extend failed")
}
/// Extends the in progress array with null elements, ignoring the input
arrays, returning an
@@ -814,10 +822,19 @@ impl<'a> MutableArrayData<'a> {
/// Prefer this over [`extend_nulls`](Self::extend_nulls) to handle cases
where the run-end
/// counter overflows (relevant for `RunEndEncoded` arrays).
///
- /// # Panics
+ /// # Errors
///
- /// Panics if [`MutableArrayData`] not created with `use_nulls` or
nullable source arrays
+ /// Returns an error if this [`MutableArrayData`] was not created with
`use_nulls` and none
+ /// of the source arrays are nullable, or if the run-end counter overflows.
pub fn try_extend_nulls(&mut self, len: usize) -> Result<(), ArrowError> {
+ if self.data.null_buffer.is_none() {
+ return Err(ArrowError::InvalidArgumentError(
+ "MutableArrayData cannot be extended with nulls: it was
created with `use_nulls` \
+ set to false and no source array is nullable"
+ .to_owned(),
+ ));
+ }
+
self.data.len += len;
let bit_len = bit_util::ceil(self.data.len, 8);
let nulls = self.data.null_buffer();
@@ -833,15 +850,14 @@ impl<'a> MutableArrayData<'a> {
///
/// # Panics
///
- /// Panics if [`MutableArrayData`] not created with `use_nulls` or
nullable source arrays,
- /// or if the run-end counter overflows for `RunEndEncoded` arrays.
+ /// Panics if this [`MutableArrayData`] was not created with `use_nulls`
and none of the
+ /// source arrays are nullable, or if the run-end counter overflows.
#[deprecated(
since = "59.0.0",
note = "Use `try_extend_nulls` which returns an error on overflow
instead of panicking"
)]
pub fn extend_nulls(&mut self, len: usize) {
- self.try_extend_nulls(len)
- .expect("extend_nulls failed due to overflow")
+ self.try_extend_nulls(len).expect("extend_nulls failed")
}
/// Returns the current length
@@ -937,6 +953,64 @@ mod test {
use arrow_schema::Field;
use std::sync::Arc;
+ fn int64_array_data(values: Vec<i64>) -> ArrayData {
+ let len = values.len();
+ ArrayData::try_new(
+ DataType::Int64,
+ len,
+ None,
+ 0,
+ vec![arrow_buffer::Buffer::from_slice_ref(&values)],
+ vec![],
+ )
+ .unwrap()
+ }
+
+ #[test]
+ fn test_try_extend_invalid_index_and_range() {
+ let array = int64_array_data(vec![1, 2, 3]);
+ let mut mutable = MutableArrayData::new(vec![&array], false, 3);
+
+ let err = mutable.try_extend(1, 0, 1).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Invalid argument error: Source array index 1 is out of bounds:
there are 1 source arrays"
+ );
+
+ let err = mutable.try_extend(0, 0, 4).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Invalid argument error: Invalid range 0..4 for source array 0 of
length 3"
+ );
+
+ // `end < start` used to underflow:
+ let err = mutable.try_extend(0, 2, 1).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Invalid argument error: Invalid range 2..1 for source array 0 of
length 3"
+ );
+
+ // The bounds are inclusive of the full array:
+ mutable.try_extend(0, 3, 3).unwrap();
+ mutable.try_extend(0, 0, 3).unwrap();
+ assert_eq!(mutable.len(), 3);
+ }
+
+ #[test]
+ fn test_try_extend_nulls_without_null_buffer() {
+ let array = int64_array_data(vec![1, 2, 3]);
+ let mut mutable = MutableArrayData::new(vec![&array], false, 3);
+ let err = mutable.try_extend_nulls(1).unwrap_err();
+ assert!(
+ err.to_string().contains("cannot be extended with nulls"),
+ "unexpected error: {err}"
+ );
+
+ let mut mutable = MutableArrayData::new(vec![&array], true, 3);
+ mutable.try_extend_nulls(1).unwrap();
+ assert_eq!(mutable.len(), 1);
+ }
+
#[test]
fn test_list_append_with_capacities() {
let array = ArrayData::new_empty(&DataType::List(Arc::new(Field::new(
diff --git a/arrow-schema/src/ffi.rs b/arrow-schema/src/ffi.rs
index 1885549cc2..3e1fe38082 100644
--- a/arrow-schema/src/ffi.rs
+++ b/arrow-schema/src/ffi.rs
@@ -132,17 +132,26 @@ unsafe extern "C" fn release_schema(schema: *mut
FFI_ArrowSchema) {
}
impl FFI_ArrowSchema {
- /// create a new [`FFI_ArrowSchema`]. This fails if the fields'
- /// [`DataType`] is not supported.
+ /// create a new [`FFI_ArrowSchema`].
///
- /// # Panics
+ /// # Errors
///
- /// Panics if `format` contains an interior nul byte
+ /// Errors if the fields' [`DataType`] is not supported,
+ /// or if `format` contains an interior nul byte.
pub fn try_new(
format: &str,
children: Vec<FFI_ArrowSchema>,
dictionary: Option<FFI_ArrowSchema>,
) -> Result<Self, ArrowError> {
+ // Convert the format before leaking any of the children,
+ // so that an error here does not leak memory.
+ let format = CString::new(format).map_err(|err| {
+ ArrowError::CDataInterface(format!(
+ "Null byte at position {} not allowed in format",
+ err.nul_position()
+ ))
+ })?;
+
let mut this = Self::empty();
let children_ptr = children
@@ -151,7 +160,7 @@ impl FFI_ArrowSchema {
.map(Box::into_raw)
.collect::<Box<_>>();
- this.format = CString::new(format).unwrap().into_raw();
+ this.format = format.into_raw();
this.release = Some(release_schema);
this.n_children = children_ptr.len() as i64;
@@ -932,6 +941,15 @@ mod tests {
assert_eq!(restored, schema);
}
+ #[test]
+ fn test_try_new_with_interior_nul_byte() {
+ let err = FFI_ArrowSchema::try_new("i\0nt", vec![], None).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "C Data interface error: Null byte at position 1 not allowed in
format"
+ );
+ }
+
#[test]
fn test_type() {
round_trip_type(DataType::Int64);
diff --git a/arrow/tests/array_transform.rs b/arrow/tests/array_transform.rs
index 23a5d85391..1f82ca4927 100644
--- a/arrow/tests/array_transform.rs
+++ b/arrow/tests/array_transform.rs
@@ -1029,11 +1029,14 @@ fn test_extend_nulls() {
}
#[test]
-#[should_panic(expected = "MutableArrayData not nullable")]
-fn test_extend_nulls_panic() {
+fn test_extend_nulls_not_nullable() {
let int = Int32Array::from(vec![1, 2, 3, 4]).into_data();
let mut mutable = MutableArrayData::new(vec![&int], false, 4);
- mutable.try_extend_nulls(2).unwrap();
+ let err = mutable.try_extend_nulls(2).unwrap_err();
+ assert!(
+ err.to_string().contains("cannot be extended with nulls"),
+ "unexpected error: {err}"
+ );
}
#[test]
diff --git a/parquet-variant-compute/src/variant_array.rs
b/parquet-variant-compute/src/variant_array.rs
index c6de730481..b8a0894719 100644
--- a/parquet-variant-compute/src/variant_array.rs
+++ b/parquet-variant-compute/src/variant_array.rs
@@ -409,11 +409,13 @@ impl VariantArray {
/// Use `try_value` if you need to handle conversion errors gracefully.
///
/// # Panics
- /// * if the index is out of bounds
- /// * if the array value is null
- /// * if `try_value` returns an error.
+ /// Panics if
+ /// * the index is out of bounds,
+ /// * the `metadata`/`value` bytes of the row are invalid, which includes
reading a null row, or
+ /// * both `value` and `typed_value` are non-null for a non-struct
`typed_value`.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
- self.try_value(index).unwrap()
+ self.try_value(index)
+ .unwrap_or_else(|err| panic!("VariantArray::value({index}) failed:
{err}"))
}
/// Return the [`Variant`] instance stored at the given row
@@ -421,16 +423,17 @@ impl VariantArray {
/// Note: This method does not check for nulls and the value is arbitrary
/// (but still well-defined) if [`is_null`](Self::is_null) returns true
for the index.
///
- /// # Panics
- ///
- /// Panics if
- /// * the index is out of bounds
- /// * the array value is null
- ///
/// # Errors
///
/// Errors if
+ /// - the index is out of bounds
/// - the data in `typed_value` cannot be interpreted as a valid `Variant`
+ /// - both `value` and `typed_value` are non-null for a non-struct
`typed_value`
+ ///
+ /// # Panics
+ ///
+ /// Panics if the unshredded `metadata`/`value` bytes fail basic
validation, since those are
+ /// read with [`Variant::new`]. This includes reading a row that is null.
///
/// If this is a shredded variant but has no value at the shredded
location, it
/// will return [`Variant::Null`].
@@ -444,13 +447,22 @@ impl VariantArray {
/// Note: Does not do deep validation of the [`Variant`], so it is up to
the
/// caller to ensure that the metadata and value were constructed
correctly.
pub fn try_value(&self, index: usize) -> Result<Variant<'_, '_>> {
+ if self.len() <= index {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Index {index} out of bounds for VariantArray of length {}",
+ self.len()
+ )));
+ }
+
let value = self.value_column();
match self.typed_value_column() {
// Always prefer typed_value, if available
Some(typed_value) if typed_value.is_valid(index) => {
if !matches!(typed_value.data_type(), DataType::Struct(_)) &&
value.is_valid(index) {
// Only a partially shredded struct is allowed to have
values for both columns
- panic!("Invalid variant, conflicting value and
typed_value");
+ return Err(ArrowError::InvalidArgumentError(
+ "Invalid variant, conflicting value and
typed_value".to_owned(),
+ ));
}
typed_value_to_variant(typed_value, index)
}
@@ -1595,6 +1607,23 @@ mod test {
}
}
+ #[test]
+ fn test_try_value_out_of_bounds() {
+ let mut b = VariantArrayBuilder::new(2);
+ b.append_variant(Variant::from(1_i8));
+ b.append_variant(Variant::Null);
+ let v = b.build();
+
+ assert_eq!(v.try_value(0).unwrap(), Variant::Int8(1));
+ assert_eq!(v.try_value(1).unwrap(), Variant::Null);
+
+ let err = v.try_value(2).unwrap_err();
+ assert_eq!(
+ err.to_string(),
+ "Invalid argument error: Index 2 out of bounds for VariantArray of
length 2"
+ );
+ }
+
#[test]
fn test_variant_array_iterable() {
let mut b = VariantArrayBuilder::new(6);
