This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new f088fc728 fix(rust/ffi)!: importing partitions is unsafe (#4876)
f088fc728 is described below
commit f088fc72823be58986dc5c0d24c21b8bd15f039d
Author: David Li <[email protected]>
AuthorDate: Sun Oct 11 15:58:56 2026 -0700
fix(rust/ffi)!: importing partitions is unsafe (#4876)
We can't validate that the pointers and such are valid, so we shouldn't
make the conversion safe - instead it must be unsafe.
Assisted-by: GPT-6.1 Sol
---
rust/driver_manager/src/lib.rs | 4 +-
rust/ffi/src/lib.rs | 2 +-
rust/ffi/src/types.rs | 134 ++++++++++++++++++++++++++++++++++++-----
3 files changed, 122 insertions(+), 18 deletions(-)
diff --git a/rust/driver_manager/src/lib.rs b/rust/driver_manager/src/lib.rs
index f42bcdc58..ea7c071b7 100644
--- a/rust/driver_manager/src/lib.rs
+++ b/rust/driver_manager/src/lib.rs
@@ -1480,7 +1480,9 @@ impl Statement for ManagedStatement {
check_status(status, error)?;
let result = PartitionedResult {
- partitions: partitions.into(),
+ // SAFETY: The driver must return valid partition allocations and
a valid
+ // release callback according to the ADBC FFI contract.
+ partitions: unsafe { adbc_ffi::import_partitions(partitions) }?,
schema: (&schema).try_into()?,
rows_affected,
};
diff --git a/rust/ffi/src/lib.rs b/rust/ffi/src/lib.rs
index c171bc522..c6fcc7d22 100644
--- a/rust/ffi/src/lib.rs
+++ b/rust/ffi/src/lib.rs
@@ -48,5 +48,5 @@ pub mod methods;
pub(crate) mod types;
pub use types::{
FFI_AdbcConnection, FFI_AdbcDatabase, FFI_AdbcDriver,
FFI_AdbcDriverInitFunc, FFI_AdbcError,
- FFI_AdbcErrorDetail, FFI_AdbcPartitions, FFI_AdbcStatement, export_error,
+ FFI_AdbcErrorDetail, FFI_AdbcPartitions, FFI_AdbcStatement, export_error,
import_partitions,
};
diff --git a/rust/ffi/src/types.rs b/rust/ffi/src/types.rs
index 87bcbaec3..264ee7ba1 100644
--- a/rust/ffi/src/types.rs
+++ b/rust/ffi/src/types.rs
@@ -212,19 +212,49 @@ macro_rules! driver_method {
};
}
-impl From<FFI_AdbcPartitions> for Partitions {
- fn from(value: FFI_AdbcPartitions) -> Self {
- let mut partitions = Vec::with_capacity(value.num_partitions);
- for p in 0..value.num_partitions {
- let partition = unsafe {
- let ptr = *value.partitions.add(p);
- let len = *value.partition_lengths.add(p);
- std::slice::from_raw_parts(ptr, len)
- };
- partitions.push(partition.to_vec());
+/// Copy FFI partitions into owned Rust partitions, releasing the FFI value.
+///
+/// Null arrays are accepted only when there are no partitions. Null partition
+/// data pointers are accepted only for zero-length partitions.
+///
+/// # Safety
+///
+/// Non-null arrays must be aligned and contain `num_partitions` initialized
+/// entries. Each nonempty, non-null partition must point to an initialized,
+/// readable allocation of at least its corresponding length. Each array or
+/// partition must fit within a single allocation, with a byte size no greater
+/// than `isize::MAX`. This memory must remain valid and must not be mutated
+/// during the import. If present, the release callback must be safe to call
+/// with this value, including when import returns an error.
+pub unsafe fn import_partitions(value: FFI_AdbcPartitions) ->
Result<Partitions, Error> {
+ if value.num_partitions == 0 {
+ return Ok(Vec::new());
+ } else if value.partitions.is_null() || value.partition_lengths.is_null() {
+ return Err(Error::with_message_and_status(
+ "partition arrays must not be null when num_partitions is nonzero",
+ Status::Internal,
+ ));
+ }
+
+ let mut partitions = Vec::with_capacity(value.num_partitions);
+ for p in 0..value.num_partitions {
+ // SAFETY: The caller guarantees readable, initialized metadata arrays.
+ let (ptr, len) = unsafe { (*value.partitions.add(p),
*value.partition_lengths.add(p)) };
+ if len == 0 {
+ partitions.push(Vec::new());
+ continue;
}
- partitions
+ if ptr.is_null() {
+ return Err(Error::with_message_and_status(
+ format!("partition {p} data pointer must not be null when its
length is nonzero"),
+ Status::Internal,
+ ));
+ }
+ // SAFETY: The caller guarantees the non-null partition is readable
for len bytes.
+ let partition = unsafe { std::slice::from_raw_parts(ptr, len) };
+ partitions.push(partition.to_vec());
}
+ Ok(partitions)
}
// Taken from `Vec::into_raw_parts` which is currently nightly-only.
@@ -711,11 +741,83 @@ mod tests {
}
#[test]
- fn test_partitions_roundtrip() {
- let partitions_expected: Partitions = vec![b"A".into(), b"BB".into(),
b"CCC".into()];
- let partitions_ffi: FFI_AdbcPartitions =
partitions_expected.clone().into();
- let partitions_actual: Partitions = partitions_ffi.into();
- assert_eq!(partitions_expected, partitions_actual);
+ fn test_partitions_roundtrip() -> Result<(), Error> {
+ let inputs: [Partitions; 3] = [
+ vec![],
+ vec![vec![], Vec::with_capacity(8)],
+ vec![b"A".into(), vec![], b"BB".into(), b"CCC".into()],
+ ];
+ for partitions_expected in inputs {
+ let partitions_ffi: FFI_AdbcPartitions =
partitions_expected.clone().into();
+ // SAFETY: The metadata, allocations, and callback come from the
Rust exporter.
+ let partitions_actual = unsafe { import_partitions(partitions_ffi)
}?;
+ assert_eq!(partitions_expected, partitions_actual);
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_import_partitions_null_pointers_and_release() {
+ use std::sync::atomic::{AtomicUsize, Ordering};
+
+ unsafe extern "C" fn release(partitions: *mut FFI_AdbcPartitions) {
+ // SAFETY: Each test input points to a live local counter, and Drop
+ // passes a valid pointer to the consumed FFI value.
+ unsafe {
+ let partitions = &mut *partitions;
+ let counter = &*partitions.private_data.cast::<AtomicUsize>();
+ counter.fetch_add(1, Ordering::SeqCst);
+ partitions.release = None;
+ }
+ }
+
+ let counter = AtomicUsize::new(0);
+ let mut data = *b"A";
+ let mut pointers = [data.as_mut_ptr(), null_mut()];
+ let mut lengths = [1, 1];
+ let mut empty_lengths = [1, 0];
+ let pointers_ptr = pointers.as_mut_ptr();
+ let lengths_ptr = lengths.as_mut_ptr();
+ let empty_lengths_ptr = empty_lengths.as_mut_ptr();
+ let inputs = [
+ (0, null_mut(), null_mut(), Some(vec![])),
+ (1, null_mut(), null_mut(), None),
+ (1, null_mut(), lengths_ptr, None),
+ (1, pointers_ptr, null_mut(), None),
+ (1, pointers_ptr.wrapping_add(1), lengths_ptr, None),
+ (2, pointers_ptr, lengths_ptr, None),
+ (
+ 2,
+ pointers_ptr,
+ empty_lengths_ptr,
+ Some(vec![b"A".to_vec(), vec![]]),
+ ),
+ ];
+
+ for (num_partitions, partitions, partition_lengths, expected) in
inputs {
+ counter.store(0, Ordering::SeqCst);
+ let value = FFI_AdbcPartitions {
+ num_partitions,
+ partitions,
+ partition_lengths,
+ private_data: (&counter as *const
AtomicUsize).cast_mut().cast(),
+ release: Some(release),
+ };
+ // SAFETY: All non-null arrays and nonempty data refer to live
local
+ // allocations of sufficient size. The callback only accesses the
counter.
+ let result = unsafe { import_partitions(value) };
+ match (result, expected) {
+ (Ok(actual), Some(expected)) => assert_eq!(actual, expected),
+ (Err(error), None) => {
+ assert_eq!(error.status, Status::Internal);
+ assert!(error.message.contains("null"));
+ }
+ (actual, expected) => {
+ assert_eq!(actual.ok(), expected, "unexpected import
result");
+ }
+ }
+ assert_eq!(counter.load(Ordering::SeqCst), 1);
+ }
}
#[test]
