diff --git a/arrow-arith/src/arity.rs b/arrow-arith/src/arity.rs index f15ea955eaaa..2f3109f087ee 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: A, b: B, diff --git a/arrow-buffer/src/buffer/mutable.rs b/arrow-buffer/src/buffer/mutable.rs index f0b45304f804..1dcfaf67f4c0 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 7145463ac45b..46ec2be089a8 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 @@ -736,13 +735,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 @@ -763,17 +772,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 @@ -782,10 +790,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(); @@ -801,15 +818,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 @@ -905,6 +921,64 @@ mod test { use arrow_schema::Field; use std::sync::Arc; + fn int64_array_data(values: Vec) -> 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 1885549cc28d..3e1fe380828d 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, dictionary: Option, ) -> Result { + // 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::>(); - 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 23a5d8539188..1f82ca492753 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 c6de730481c0..b8a089471916 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> { + 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);