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 c87638f4a9 fix(ffi): make FFI struct fields private to close Drop
soundness hole (#10431)
c87638f4a9 is described below
commit c87638f4a977c289395725bb329cf33fb4e8fc44
Author: Aditya Mishra <[email protected]>
AuthorDate: Thu Aug 13 18:53:00 2026 +0530
fix(ffi): make FFI struct fields private to close Drop soundness hole
(#10431)
# Which issue does this PR close?
- Closes #10429.
- Closes #10253.
# Rationale for this change
`FFI_ArrowArray`, `FFI_ArrowSchema`, and `FFI_ArrowArrayStream` had all
of their fields `pub`. Several of those fields carry invariants that the
`Drop` impl and the import path rely on, so safe code could set them to
values that trigger undefined behavior with no `unsafe` block:
- Setting `FFI_ArrowSchema::format` or `name` to a pointer that did not
come from `CString::into_raw` causes UB when the release callback frees
it with `CString::from_raw`.
- Setting `FFI_ArrowArray::buffers` to an invalid pointer causes UB when
it is dereferenced with `ptr::read_unaligned` on import.
- Setting the `release` fn pointer (or the stream's `get_schema` /
`get_next` / `get_last_error` fn pointers) to a bogus function causes UB
when it is invoked on drop or import.
An earlier revision of this PR made only `release` and `private_data`
private and left the data fields `pub`, on the assumption that the data
fields were harmless. That was wrong: as noted in review, the
pointer-carrying data fields are UB vectors too. Any write to these
fields has to be treated as unsafe.
# What changes are included in this PR?
- All fields on `FFI_ArrowArray`, `FFI_ArrowSchema`, and
`FFI_ArrowArrayStream` are now private.
- Reads are unchanged: every field already has a typed getter, so
consumers keep full read access.
- The wrap-release use case from #9771 keeps working through the
existing `unsafe` setters `set_release` and `set_private_data`, which
swap in a new callback and private data and return the old values so the
caller can chain into the original release. No other field needs a write
path (the only fields any external consumer writes are `release` and
`private_data`).
- `#[repr(C)]` is unchanged, so the C Data Interface layout and ABI are
identical.
A separate soundness issue with `FFI_ArrowSchema::with_metadata` /
`with_name` on foreign schemas was raised in review. It has a different
root cause and changes the safe/unsafe surface of those methods, so it
will be handled in its own issue and PR rather than bundled here.
# Are these changes tested?
Yes. Each struct has a `test_wrap_release_callback` test covering the
swap-and-restore path, and all three pass under Miri with no leak,
use-after-free, or double-free. The existing FFI roundtrip tests also
pass under Miri. The full workspace builds and tests with `--features
ffi`.
# Are there any user-facing changes?
Yes, this is a breaking change. Code that read these fields directly
must switch to the getters. Code that constructed these structs with a
struct literal, or wrote fields directly, must use the safe constructors
(`try_from`, `new`, `empty`) or, for `release` / `private_data`, the
`unsafe` setters. The `#[repr(C)]` layout is unchanged.
---------
Co-authored-by: Jeffrey Vo <[email protected]>
---
arrow-array/src/ffi_stream.rs | 98 +++++++++++++++++++++++++++++++++++++---
arrow-data/src/ffi.rs | 63 +++++++++++++++++++++-----
arrow-schema/src/ffi.rs | 101 +++++++++++++++++++++++++++++++++++++-----
3 files changed, 233 insertions(+), 29 deletions(-)
diff --git a/arrow-array/src/ffi_stream.rs b/arrow-array/src/ffi_stream.rs
index aeb7ca0918..ac79e10eb2 100644
--- a/arrow-array/src/ffi_stream.rs
+++ b/arrow-array/src/ffi_stream.rs
@@ -96,17 +96,18 @@ const ENOSYS: i32 = 38;
#[repr(C)]
#[derive(Debug)]
pub struct FFI_ArrowArrayStream {
+ // Fields are intentionally private so safety guarantees can be upheld via
+ // explicit unsafe functions.
/// C function to get schema from the stream
- pub get_schema:
- Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowSchema) -> c_int>,
+ get_schema: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowSchema) -> c_int>,
/// C function to get next array from the stream
- pub get_next: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowArray) -> c_int>,
+ get_next: Option<unsafe extern "C" fn(arg1: *mut Self, out: *mut
FFI_ArrowArray) -> c_int>,
/// C function to get the error from last operation on the stream
- pub get_last_error: Option<unsafe extern "C" fn(arg1: *mut Self) -> *const
c_char>,
+ get_last_error: Option<unsafe extern "C" fn(arg1: *mut Self) -> *const
c_char>,
/// C function to release the stream
- pub release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
- /// Private data used by the stream
- pub private_data: *mut c_void,
+ release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
+ /// Private data used by the stream, owned by the release callback.
+ private_data: *mut c_void,
}
unsafe impl Send for FFI_ArrowArrayStream {}
@@ -212,6 +213,45 @@ impl FFI_ArrowArrayStream {
private_data: std::ptr::null_mut(),
}
}
+
+ /// Returns the producer-provided release callback, if any.
+ pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut Self)> {
+ self.release
+ }
+
+ /// Returns the opaque producer-provided private data pointer.
+ pub fn private_data(&self) -> *mut c_void {
+ self.private_data
+ }
+
+ /// Replaces the release callback, returning the previous one.
+ ///
+ /// Lets a consumer wrap release: save the old callback, install its own,
and
+ /// chain back on drop. See
<https://github.com/apache/arrow-rs/issues/9771>.
+ ///
+ /// # Safety
+ ///
+ /// [`Drop`] calls this callback with a pointer to `self`. The new callback
+ /// must correctly release this stream (usually by chaining to the returned
+ /// one) and must match the [`FFI_ArrowArrayStream::private_data`] it
reads.
+ /// A wrong callback is undefined behavior on drop.
+ pub unsafe fn set_release(
+ &mut self,
+ release: Option<unsafe extern "C" fn(arg1: *mut Self)>,
+ ) -> Option<unsafe extern "C" fn(arg1: *mut Self)> {
+ std::mem::replace(&mut self.release, release)
+ }
+
+ /// Replaces the private data pointer, returning the previous one.
+ ///
+ /// # Safety
+ ///
+ /// The old pointer is returned without being freed; the caller owns it
from
+ /// here. The new pointer must match what the current
+ /// [`FFI_ArrowArrayStream::release`] callback expects.
+ pub unsafe fn set_private_data(&mut self, private_data: *mut c_void) ->
*mut c_void {
+ std::mem::replace(&mut self.private_data, private_data)
+ }
}
struct ExportedArrayStream {
@@ -564,4 +604,48 @@ mod tests {
Ok(())
}
+
+ // A consumer wraps the release callback with its own, then chains back to
+ // the original on drop. This is the same wrap-release pattern the
+ // release/private_data accessors exist for (#9771).
+ static STREAM_WRAPPER_RAN: std::sync::atomic::AtomicBool =
+ std::sync::atomic::AtomicBool::new(false);
+
+ struct StreamWrapperData {
+ original_release: Option<unsafe extern "C" fn(*mut
FFI_ArrowArrayStream)>,
+ original_private_data: *mut c_void,
+ }
+
+ unsafe extern "C" fn wrapping_release(stream: *mut FFI_ArrowArrayStream) {
+ use std::sync::atomic::Ordering;
+ let stream = unsafe { &mut *stream };
+ let data = unsafe { Box::from_raw(stream.private_data() as *mut
StreamWrapperData) };
+ STREAM_WRAPPER_RAN.store(true, Ordering::SeqCst);
+ unsafe { stream.set_release(data.original_release) };
+ unsafe { stream.set_private_data(data.original_private_data) };
+ if let Some(release) = stream.release() {
+ unsafe { release(stream) };
+ }
+ }
+
+ #[test]
+ fn test_wrap_release_callback() {
+ use std::sync::atomic::Ordering;
+
+ let batch_reader = Box::new(TestRecordBatchReader::new(
+ Arc::new(Schema::new(vec![Field::new("a", DataType::Int32,
true)])),
+ Box::new(std::iter::empty()),
+ ));
+ let mut stream = FFI_ArrowArrayStream::new(batch_reader);
+
+ let data = Box::new(StreamWrapperData {
+ original_release: stream.release(),
+ original_private_data: stream.private_data(),
+ });
+ unsafe { stream.set_release(Some(wrapping_release)) };
+ unsafe { stream.set_private_data(Box::into_raw(data) as *mut c_void) };
+
+ drop(stream); // runs wrapping_release, which chains to the original
+ assert!(STREAM_WRAPPER_RAN.load(Ordering::SeqCst));
+ }
}
diff --git a/arrow-data/src/ffi.rs b/arrow-data/src/ffi.rs
index e00b6eea49..8506a8844d 100644
--- a/arrow-data/src/ffi.rs
+++ b/arrow-data/src/ffi.rs
@@ -37,31 +37,33 @@ use std::ffi::c_void;
#[repr(C)]
#[derive(Debug)]
pub struct FFI_ArrowArray {
+ // Fields are intentionally private so safety guarantees can be upheld via
+ // explicit unsafe functions.
/// Logical length of the array
- pub length: i64,
+ length: i64,
/// Number of null items in the array
- pub null_count: i64,
+ null_count: i64,
/// logical offset inside the array
- pub offset: i64,
+ offset: i64,
/// Number of physical buffers backing this array
- pub n_buffers: i64,
+ n_buffers: i64,
/// Number of children this array has
- pub n_children: i64,
+ n_children: i64,
/// C array of pointers to the start of each physical buffer backing this
array
- pub buffers: *mut *const c_void,
+ buffers: *mut *const c_void,
/// C array of pointers to each child array of this array
- pub children: *mut *mut FFI_ArrowArray,
+ children: *mut *mut FFI_ArrowArray,
/// Pointer to the underlying array of dictionary values
- pub dictionary: *mut FFI_ArrowArray,
- /// Pointer to a producer-provided release callback
- pub release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowArray)>,
+ dictionary: *mut FFI_ArrowArray,
+ /// Producer-provided release callback.
+ release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowArray)>,
/// Opaque pointer to producer-provided private data
/// When exported, this MUST contain everything that is owned by this
array.
/// For example, any buffer pointed to in `buffers` must be here, as well
/// as the `buffers` pointer itself.
/// In other words, everything in [FFI_ArrowArray] must be owned by
/// `private_data` and can assume that they do not outlive `private_data`.
- pub private_data: *mut c_void,
+ private_data: *mut c_void,
}
impl Drop for FFI_ArrowArray {
@@ -251,6 +253,45 @@ impl FFI_ArrowArray {
}
}
+ /// Returns the producer-provided release callback, if any.
+ pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut
FFI_ArrowArray)> {
+ self.release
+ }
+
+ /// Returns the opaque producer-provided private data pointer.
+ pub fn private_data(&self) -> *mut c_void {
+ self.private_data
+ }
+
+ /// Replaces the release callback, returning the previous one.
+ ///
+ /// Lets a consumer wrap release: save the old callback, install its own,
and
+ /// chain back on drop. See
<https://github.com/apache/arrow-rs/issues/9771>.
+ ///
+ /// # Safety
+ ///
+ /// [`Drop`] calls this callback with a pointer to `self`. The new callback
+ /// must correctly release this array (usually by chaining to the returned
+ /// one) and must match the [`FFI_ArrowArray::private_data`] it reads. A
+ /// wrong callback is undefined behavior on drop.
+ pub unsafe fn set_release(
+ &mut self,
+ release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowArray)>,
+ ) -> Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowArray)> {
+ std::mem::replace(&mut self.release, release)
+ }
+
+ /// Replaces the private data pointer, returning the previous one.
+ ///
+ /// # Safety
+ ///
+ /// The old pointer is returned without being freed; the caller owns it
from
+ /// here. The new pointer must match what the current
+ /// [`FFI_ArrowArray::release`] callback expects.
+ pub unsafe fn set_private_data(&mut self, private_data: *mut c_void) ->
*mut c_void {
+ std::mem::replace(&mut self.private_data, private_data)
+ }
+
/// the length of the array
#[inline]
pub fn len(&self) -> usize {
diff --git a/arrow-schema/src/ffi.rs b/arrow-schema/src/ffi.rs
index 6bff0c85df..557313ab85 100644
--- a/arrow-schema/src/ffi.rs
+++ b/arrow-schema/src/ffi.rs
@@ -74,25 +74,27 @@ bitflags! {
#[repr(C)]
#[derive(Debug)]
pub struct FFI_ArrowSchema {
+ // Fields are intentionally private so safety guarantees can be upheld via
+ // explicit unsafe functions.
/// Null-terminated, UTF8-encoded string describing the data type
- pub format: *const c_char,
+ format: *const c_char,
/// Null-terminated, UTF8-encoded string of the field or array name
- pub name: *const c_char,
+ name: *const c_char,
/// Binary string describing the type’s metadata
- pub metadata: *const c_char,
+ metadata: *const c_char,
/// A bitfield of flags enriching the type description
/// Refer to [Arrow
Flags](https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.flags)
- pub flags: i64,
+ flags: i64,
/// The number of children this type has
- pub n_children: i64,
+ n_children: i64,
/// C array of pointers to each child type of this type
- pub children: *mut *mut FFI_ArrowSchema,
+ children: *mut *mut FFI_ArrowSchema,
/// Pointer to the type of dictionary values
- pub dictionary: *mut FFI_ArrowSchema,
- /// Pointer to a producer-provided release callback
- pub release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowSchema)>,
- /// Opaque pointer to producer-provided private data
- pub private_data: *mut c_void,
+ dictionary: *mut FFI_ArrowSchema,
+ /// Producer-provided release callback.
+ release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowSchema)>,
+ /// Opaque producer-provided private data, owned by the release callback.
+ private_data: *mut c_void,
}
struct SchemaPrivateData {
@@ -279,6 +281,45 @@ impl FFI_ArrowSchema {
}
}
+ /// Returns the producer-provided release callback, if any.
+ pub fn release(&self) -> Option<unsafe extern "C" fn(arg1: *mut
FFI_ArrowSchema)> {
+ self.release
+ }
+
+ /// Returns the opaque producer-provided private data pointer.
+ pub fn private_data(&self) -> *mut c_void {
+ self.private_data
+ }
+
+ /// Replaces the release callback, returning the previous one.
+ ///
+ /// Lets a consumer wrap release: save the old callback, install its own,
and
+ /// chain back on drop. See
<https://github.com/apache/arrow-rs/issues/9771>.
+ ///
+ /// # Safety
+ ///
+ /// [`Drop`] calls this callback with a pointer to `self`. The new callback
+ /// must correctly release this schema (usually by chaining to the returned
+ /// one) and must match the [`FFI_ArrowSchema::private_data`] it reads. A
+ /// wrong callback is undefined behavior on drop.
+ pub unsafe fn set_release(
+ &mut self,
+ release: Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowSchema)>,
+ ) -> Option<unsafe extern "C" fn(arg1: *mut FFI_ArrowSchema)> {
+ std::mem::replace(&mut self.release, release)
+ }
+
+ /// Replaces the private data pointer, returning the previous one.
+ ///
+ /// # Safety
+ ///
+ /// The old pointer is returned without being freed; the caller owns it
from
+ /// here. The new pointer must match what the current
+ /// [`FFI_ArrowSchema::release`] callback expects.
+ pub unsafe fn set_private_data(&mut self, private_data: *mut c_void) ->
*mut c_void {
+ std::mem::replace(&mut self.private_data, private_data)
+ }
+
/// Returns the format of this schema.
///
/// # Panics
@@ -870,6 +911,7 @@ impl TryFrom<Schema> for FFI_ArrowSchema {
mod tests {
use super::*;
use crate::Fields;
+ use std::sync::atomic::{AtomicBool, Ordering};
fn round_trip_type(dtype: DataType) {
let c_schema = FFI_ArrowSchema::try_from(&dtype).unwrap();
@@ -1042,4 +1084,41 @@ mod tests {
let field = Field::try_from(&c_schema).unwrap();
assert_eq!(field.name(), "");
}
+
+ // A consumer wraps the release callback with its own, then chains back to
+ // the original on drop. This is the cross-thread use case from #9771 and
is
+ // what the release/private_data accessors exist for.
+ static WRAPPER_RAN: AtomicBool = AtomicBool::new(false);
+
+ struct WrapperData {
+ original_release: Option<unsafe extern "C" fn(*mut FFI_ArrowSchema)>,
+ original_private_data: *mut c_void,
+ }
+
+ unsafe extern "C" fn wrapping_release(schema: *mut FFI_ArrowSchema) {
+ let schema = unsafe { &mut *schema };
+ let data = unsafe { Box::from_raw(schema.private_data() as *mut
WrapperData) };
+ WRAPPER_RAN.store(true, Ordering::SeqCst);
+ // restore the originals, then let the original callback free
everything
+ unsafe { schema.set_release(data.original_release) };
+ unsafe { schema.set_private_data(data.original_private_data) };
+ if let Some(release) = schema.release() {
+ unsafe { release(schema) };
+ }
+ }
+
+ #[test]
+ fn test_wrap_release_callback() {
+ let mut schema = FFI_ArrowSchema::try_from(&DataType::Int32).unwrap();
+
+ let data = Box::new(WrapperData {
+ original_release: schema.release(),
+ original_private_data: schema.private_data(),
+ });
+ unsafe { schema.set_release(Some(wrapping_release)) };
+ unsafe { schema.set_private_data(Box::into_raw(data) as *mut c_void) };
+
+ drop(schema); // runs wrapping_release, which chains to the original
+ assert!(WRAPPER_RAN.load(Ordering::SeqCst));
+ }
}
