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 732726e82 fix(rust/ffi): preserve caller's AdbcError private_data on
the 1.0.0 path (#4473)
732726e82 is described below
commit 732726e825047977ef9d355a4b9d530a66557c7b
Author: Fredrik Fornwall <[email protected]>
AuthorDate: Thu Jul 9 00:42:50 2026 +0200
fix(rust/ffi): preserve caller's AdbcError private_data on the 1.0.0 path
(#4473)
Fixes #4472.
---------
Signed-off-by: Fredrik Fornwall <[email protected]>
Co-authored-by: David Li <[email protected]>
---
rust/ffi/src/driver_exporter.rs | 50 ++++------
rust/ffi/src/lib.rs | 2 +-
rust/ffi/src/types.rs | 197 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 216 insertions(+), 33 deletions(-)
diff --git a/rust/ffi/src/driver_exporter.rs b/rust/ffi/src/driver_exporter.rs
index b1bb5e90a..ca483c5a4 100644
--- a/rust/ffi/src/driver_exporter.rs
+++ b/rust/ffi/src/driver_exporter.rs
@@ -238,12 +238,7 @@ macro_rules! check_err {
Err(error) => {
let error = adbc_core::error::Error::from(error);
let status: adbc_core::error::AdbcStatusCode =
error.status.into();
- if !$err_out.is_null() {
- let mut ffi_error =
-
$crate::FFI_AdbcError::try_from(error).unwrap_or_else(Into::into);
- ffi_error.private_driver = unsafe {
(*$err_out).private_driver };
- unsafe { std::ptr::write_unaligned($err_out, ffi_error) };
- }
+ unsafe { $crate::export_error($err_out, error) };
return status;
}
}
@@ -280,16 +275,11 @@ macro_rules! pointer_as_mut {
match unsafe { $ptr.as_mut() } {
Some(p) => p,
None => {
- if !$err_out.is_null() {
- let error =
adbc_core::error::Error::with_message_and_status(
- format!("Passed null pointer for argument {:?}",
stringify!($ptr)),
- adbc_core::error::Status::InvalidArguments,
- );
- let mut ffi_error =
-
$crate::FFI_AdbcError::try_from(error).unwrap_or_else(Into::into);
- ffi_error.private_driver = unsafe {
(*$err_out).private_driver };
- unsafe { std::ptr::write_unaligned($err_out, ffi_error) };
- }
+ let error = adbc_core::error::Error::with_message_and_status(
+ format!("Passed null pointer for argument {:?}",
stringify!($ptr)),
+ adbc_core::error::Status::InvalidArguments,
+ );
+ unsafe { $crate::export_error($err_out, error) };
return adbc_core::error::Status::InvalidArguments.into();
}
}
@@ -507,22 +497,18 @@ fn catch_panic<F: FnOnce() -> AdbcStatusCode +
std::panic::UnwindSafe>(
Err(cause) => {
POISON.store(true, std::sync::atomic::Ordering::Release);
- if !error.is_null() {
- let message = if let Some(s) = cause.downcast_ref::<&str>() {
- s.to_string()
- } else if let Some(s) = cause.downcast_ref::<String>() {
- s.clone()
- } else {
- "Unknown panic".to_string()
- };
- let err = Error::with_message_and_status(
- format!("Uncaught panic in driver: {message}"),
- Status::Internal,
- );
- let mut ffi_error =
FFI_AdbcError::try_from(err).unwrap_or_else(Into::into);
- ffi_error.private_driver = unsafe { (*error).private_driver };
- unsafe { std::ptr::write_unaligned(error, ffi_error) };
- }
+ let message = if let Some(s) = cause.downcast_ref::<&str>() {
+ s.to_string()
+ } else if let Some(s) = cause.downcast_ref::<String>() {
+ s.clone()
+ } else {
+ "Unknown panic".to_string()
+ };
+ let err = Error::with_message_and_status(
+ format!("Uncaught panic in driver: {message}"),
+ Status::Internal,
+ );
+ unsafe { crate::export_error(error, err) };
Status::Internal.into()
}
}
diff --git a/rust/ffi/src/lib.rs b/rust/ffi/src/lib.rs
index 0805d2f0e..c171bc522 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,
+ FFI_AdbcErrorDetail, FFI_AdbcPartitions, FFI_AdbcStatement, export_error,
};
diff --git a/rust/ffi/src/types.rs b/rust/ffi/src/types.rs
index 033eb7c21..01f27e0a2 100644
--- a/rust/ffi/src/types.rs
+++ b/rust/ffi/src/types.rs
@@ -45,6 +45,15 @@ pub struct FFI_AdbcError {
pub private_driver: *const FFI_AdbcDriver,
}
+#[repr(C)]
+#[derive(Debug)]
+pub struct FFI_AdbcErrorV100 {
+ pub message: *mut c_char,
+ pub vendor_code: i32,
+ pub sqlstate: [c_char; 5],
+ pub release: Option<unsafe extern "C" fn(*mut Self)>,
+}
+
#[repr(C)]
#[derive(Debug)]
pub struct FFI_AdbcErrorDetail {
@@ -555,6 +564,37 @@ impl TryFrom<Error> for FFI_AdbcError {
}
}
+impl From<NulError> for FFI_AdbcErrorV100 {
+ fn from(value: NulError) -> Self {
+ let message = CString::new(format!(
+ "Interior null byte was found at position {}",
+ value.nul_position()
+ ))
+ .unwrap();
+ FFI_AdbcErrorV100 {
+ message: message.into_raw(),
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ release: Some(release_ffi_error_v100),
+ }
+ }
+}
+
+impl TryFrom<Error> for FFI_AdbcErrorV100 {
+ type Error = NulError;
+
+ fn try_from(value: Error) -> Result<Self, Self::Error> {
+ // TODO: instead of dying on an interior NUL, just truncate or replace
+ let message = CString::new(value.message)?;
+ Ok(FFI_AdbcErrorV100 {
+ message: message.into_raw(),
+ release: Some(release_ffi_error_v100),
+ vendor_code: value.vendor_code,
+ sqlstate: value.sqlstate,
+ })
+ }
+}
+
unsafe extern "C" fn release_ffi_error(error: *mut FFI_AdbcError) {
match error.as_mut() {
None => (),
@@ -579,6 +619,44 @@ unsafe extern "C" fn release_ffi_error(error: *mut
FFI_AdbcError) {
}
}
+unsafe extern "C" fn release_ffi_error_v100(error: *mut FFI_AdbcErrorV100) {
+ match error.as_mut() {
+ None => (),
+ Some(error) => {
+ if !error.message.is_null() {
+ // SAFETY: `error.message` was necessarily obtained with
`CString::into_raw`.
+ drop(CString::from_raw(error.message));
+ error.message = null_mut();
+ }
+ error.release = None;
+ }
+ }
+}
+
+/// Export an error into an FFI error, accounting for the v1.0.0/v1.1.0 ABI
+/// extension.
+#[doc(hidden)]
+pub unsafe fn export_error(err_out: *mut FFI_AdbcError, error: Error) {
+ if err_out.is_null() {
+ return;
+ }
+
+ let is_v110 = (*err_out).vendor_code ==
constants::ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA;
+ if let Some(release) = (*err_out).release {
+ release(err_out);
+ }
+ if is_v110 {
+ let mut ffi_error =
FFI_AdbcError::try_from(error).unwrap_or_else(Into::into);
+ ffi_error.vendor_code = constants::ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA;
+ ffi_error.private_driver = (*err_out).private_driver;
+ std::ptr::write_unaligned(err_out, ffi_error);
+ } else {
+ let ffi_error =
FFI_AdbcErrorV100::try_from(error).unwrap_or_else(Into::into);
+ let out = &mut *(err_out as *mut FFI_AdbcErrorV100);
+ std::ptr::write_unaligned(out, ffi_error);
+ }
+}
+
impl Drop for FFI_AdbcError {
fn drop(&mut self) {
if let Some(release) = self.release {
@@ -690,4 +768,123 @@ mod tests {
assert!(ffi_error.release.is_none());
// Drop here is a no-op because release is None.
}
+
+ #[test]
+ fn test_set_error_out_preserves_caller_private_fields() {
+ // A caller that did not opt into the 1.1.0 layout leaves
`vendor_code` at
+ // something other than the sentinel. `export_error` must then leave
+ // `private_data` / `private_driver` untouched, even when the error
carries
+ // details that would otherwise be stashed in `private_data`. This
mirrors
+ // the C++ validation suite's `StatementTest.ErrorCompatibility`.
+ let mut canary_data = 0x1234;
+ let canary_ptr = (&raw mut canary_data) as *mut c_void;
+ let canary_driver: FFI_AdbcDriver = Default::default();
+ let mut out = FFI_AdbcError {
+ message: null_mut(),
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ release: None,
+ private_data: canary_ptr,
+ private_driver: &raw const canary_driver,
+ };
+
+ let error = Error {
+ message: "boom".into(),
+ status: Status::Internal,
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ details: Some(vec![("key".to_string(), b"value".to_vec())]),
+ };
+ unsafe { export_error(&mut out, error) };
+
+ // The extended fields are untouched.
+ assert_eq!(out.private_data, canary_ptr);
+ assert_eq!(out.private_driver, &raw const canary_driver);
+ // The message was written and a message-only release installed.
+ assert!(!out.message.is_null());
+ assert!(out.release.is_some());
+
+ // Releasing frees only the message and must not touch the canary.
+ unsafe { (out.release.unwrap())(&mut out) };
+ assert!(out.message.is_null());
+ assert!(out.release.is_none());
+ assert_eq!(out.private_data, canary_ptr);
+ assert_eq!(out.private_driver, &raw const canary_driver);
+ // Drop here is a no-op because release is None.
+ }
+
+ #[test]
+ fn test_set_error_out_extended_layout_overwrites() {
+ // A caller that opted in (vendor_code == sentinel) gets the full
struct,
+ // including details in `private_data`; `private_driver` is preserved
so the
+ // driver manager keeps its handle.
+ let canary_driver: FFI_AdbcDriver = Default::default();
+ let mut out = FFI_AdbcError {
+ message: null_mut(),
+ vendor_code: constants::ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA,
+ sqlstate: [0; 5],
+ release: None,
+ private_data: null_mut(),
+ private_driver: &raw const canary_driver,
+ };
+
+ let error = Error {
+ message: "boom".into(),
+ status: Status::Internal,
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ details: Some(vec![("key".to_string(), b"value".to_vec())]),
+ };
+ unsafe { export_error(&mut out, error) };
+
+ assert!(!out.message.is_null());
+ assert!(!out.private_data.is_null()); // details stashed here
+ assert_eq!(out.private_driver, &raw const canary_driver); // preserved
+ // The sentinel survives: consumers may only read private_data while
+ // vendor_code holds it (adbc.h), so the details above stay reachable.
+ assert_eq!(
+ out.vendor_code,
+ constants::ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA
+ );
+ assert!(out.release.is_some());
+
+ // The full release frees both the message and the details in
private_data.
+ unsafe { (out.release.unwrap())(&mut out) };
+ assert!(out.message.is_null());
+ assert!(out.private_data.is_null());
+ // Drop here is a no-op because release is None.
+ }
+
+ #[test]
+ fn test_set_error_out_releases_a_previous_error_on_reuse() {
+ // Writing into a struct that still holds an earlier error releases it
+ // first (as the C reference does), instead of leaking its message.
+ let mut out = FFI_AdbcError {
+ message: null_mut(),
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ release: None,
+ private_data: null_mut(),
+ private_driver: null(),
+ };
+ let error = |msg: &str| Error {
+ message: msg.into(),
+ status: Status::Internal,
+ vendor_code: 0,
+ sqlstate: [0; 5],
+ details: None,
+ };
+
+ unsafe { export_error(&mut out, error("first")) };
+ let first_message = out.message;
+ assert!(!first_message.is_null());
+
+ unsafe { export_error(&mut out, error("second")) };
+ assert!(!out.message.is_null());
+ let message = unsafe { CStr::from_ptr(out.message) };
+ assert_eq!(message.to_str().unwrap(), "second");
+
+ unsafe { (out.release.unwrap())(&mut out) };
+ assert!(out.message.is_null());
+ }
}
