From 824690991669f8410fa4592fd2e34c1096e126b6 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Sat, 22 Jun 2024 23:15:16 -0400 Subject: [PATCH] `RustCallStatus::error_buf` is now `ManuallyDrop` and not `MaybeUninit`. `error_buf` is owned by the foreign side. Because we assume it has been initialized we use `ManuallyDrop` instead of `MaybeUninit`, which clarifies ownership and avoids unsafe code. Fixes #2168 --- uniffi_core/src/ffi/ffiserialize.rs | 14 ++- uniffi_core/src/ffi/foreignfuture.rs | 2 +- uniffi_core/src/ffi/rustcalls.rs | 90 +++++++------------ uniffi_core/src/ffi/rustfuture/tests.rs | 17 ++-- uniffi_core/src/ffi_converter_traits.rs | 10 +-- .../src/export/callback_interface.rs | 2 +- 6 files changed, 54 insertions(+), 81 deletions(-) diff --git a/uniffi_core/src/ffi/ffiserialize.rs b/uniffi_core/src/ffi/ffiserialize.rs index 2ef91a8270..5af0df85b1 100644 --- a/uniffi_core/src/ffi/ffiserialize.rs +++ b/uniffi_core/src/ffi/ffiserialize.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use crate::{Handle, RustBuffer, RustCallStatus, RustCallStatusCode}; -use std::{mem::MaybeUninit, ptr::NonNull}; +use std::{mem::ManuallyDrop, ptr::NonNull}; /// FFIBuffer element /// @@ -228,15 +228,13 @@ impl FfiSerialize for RustCallStatus { let code = unsafe { buf[0].i8 }; Self { code: RustCallStatusCode::try_from(code).unwrap_or(RustCallStatusCode::UnexpectedError), - error_buf: MaybeUninit::new(RustBuffer::get(&buf[1..])), + error_buf: ManuallyDrop::new(RustBuffer::get(&buf[1..])), } } fn put(buf: &mut [FfiBufferElement], value: Self) { buf[0].i8 = value.code as i8; - // Safety: This is okay even if the error buf is not initialized. It just means we'll be - // copying the garbage data. - unsafe { RustBuffer::put(&mut buf[1..], value.error_buf.assume_init()) } + RustBuffer::put(&mut buf[1..], ManuallyDrop::into_inner(value.error_buf)) } } @@ -278,8 +276,8 @@ mod test { rust_buffer.capacity(), ); let handle = Handle::from_raw(101).unwrap(); - let rust_call_status = RustCallStatus::new(); - let rust_call_status_error_buf = unsafe { rust_call_status.error_buf.assume_init_ref() }; + let rust_call_status = RustCallStatus::default(); + let rust_call_status_error_buf = &rust_call_status.error_buf; let orig_rust_call_status_buffer_data = ( rust_call_status_error_buf.data_pointer(), rust_call_status_error_buf.len(), @@ -334,7 +332,7 @@ mod test { let rust_call_status2 = ::read(&mut buf_reader); assert_eq!(rust_call_status2.code, RustCallStatusCode::Success); - let rust_call_status2_error_buf = unsafe { rust_call_status2.error_buf.assume_init() }; + let rust_call_status2_error_buf = ManuallyDrop::into_inner(rust_call_status2.error_buf); assert_eq!( ( rust_call_status2_error_buf.data_pointer(), diff --git a/uniffi_core/src/ffi/foreignfuture.rs b/uniffi_core/src/ffi/foreignfuture.rs index 87d8b9a4e7..804beaef62 100644 --- a/uniffi_core/src/ffi/foreignfuture.rs +++ b/uniffi_core/src/ffi/foreignfuture.rs @@ -147,7 +147,7 @@ mod test { *data, ForeignFutureResult { return_value: >::lower(value), - call_status: RustCallStatus::new(), + call_status: RustCallStatus::default(), }, ); } diff --git a/uniffi_core/src/ffi/rustcalls.rs b/uniffi_core/src/ffi/rustcalls.rs index 91d3fe2472..cdc02b91d0 100644 --- a/uniffi_core/src/ffi/rustcalls.rs +++ b/uniffi_core/src/ffi/rustcalls.rs @@ -12,7 +12,7 @@ //! exception use crate::{FfiDefault, Lower, RustBuffer, UniFfiTag}; -use std::mem::MaybeUninit; +use std::mem::ManuallyDrop; use std::panic; /// Represents the success/error of a rust call @@ -42,52 +42,43 @@ use std::panic; #[repr(C)] pub struct RustCallStatus { pub code: RustCallStatusCode, - // code is signed because unsigned types are experimental in Kotlin - pub error_buf: MaybeUninit, - // error_buf is MaybeUninit to avoid dropping the value that the consumer code sends in: - // - Consumers should send in a zeroed out RustBuffer. In this case dropping is a no-op and - // avoiding the drop is a small optimization. - // - If consumers pass in invalid data, then we should avoid trying to drop it. In - // particular, we don't want to try to free any data the consumer has allocated. - // - // `MaybeUninit` requires unsafe code, since we are preventing rust from dropping the value. - // To use this safely we need to make sure that no code paths set this twice, since that will - // leak the first `RustBuffer`. + // error_buf is owned by the foreign side. + // - Whatever we are passed, we must never free. This however implies we must be passed + // an empty `RustBuffer` otherwise it would leak when we replace it with our own. + // - On error we will set it to a `RustBuffer` we expect the foreign side to free. + // We assume initialization, which means we can use `ManuallyDrop` instead of + // `MaybeUninit`, which avoids unsafe code and clarifies ownership. + // We must take care to not set this twice to avoid leaking the first `RustBuffer`. + pub error_buf: ManuallyDrop, } -impl RustCallStatus { - pub fn new() -> Self { +impl Default for RustCallStatus { + fn default() -> Self { Self { code: RustCallStatusCode::Success, - error_buf: MaybeUninit::new(RustBuffer::new()), + error_buf: Default::default(), } } +} +impl RustCallStatus { pub fn cancelled() -> Self { Self { code: RustCallStatusCode::Cancelled, - error_buf: MaybeUninit::new(RustBuffer::new()), + error_buf: Default::default(), } } pub fn error(message: impl Into) -> Self { Self { code: RustCallStatusCode::UnexpectedError, - error_buf: MaybeUninit::new(>::lower(message.into())), - } - } -} - -impl Default for RustCallStatus { - fn default() -> Self { - Self { - code: RustCallStatusCode::Success, - error_buf: MaybeUninit::uninit(), + error_buf: ManuallyDrop::new(>::lower(message.into())), } } } /// Result of a FFI call to a Rust function +/// Value is signed to avoid Kotlin's experimental unsigned types. #[repr(i8)] #[derive(Debug, PartialEq, Eq)] pub enum RustCallStatusCode { @@ -171,11 +162,7 @@ where // Callback returned an Err. Ok(Err(buf)) => { out_status.code = RustCallStatusCode::Error; - unsafe { - // Unsafe because we're setting the `MaybeUninit` value, see above for safety - // invariants. - out_status.error_buf.as_mut_ptr().write(buf); - } + *out_status.error_buf = buf; None } // Callback panicked @@ -196,11 +183,9 @@ where >::lower(message) })); if let Ok(buf) = message_result { - unsafe { - // Unsafe because we're setting the `MaybeUninit` value, see above for safety - // invariants. - out_status.error_buf.as_mut_ptr().write(buf); - } + // If this was ever set twice we'd leak the old value - but because this is the only + // place where it is set, and this is only called once, no leaks should exist in practice. + *out_status.error_buf = buf; } // Ignore the error case. We've done all that we can at this point. In the bindings // code, we handle this by checking if `error_buf` still has an empty `RustBuffer` and @@ -215,13 +200,6 @@ mod test { use super::*; use crate::{test_util::TestError, Lift, LowerReturn}; - fn create_call_status() -> RustCallStatus { - RustCallStatus { - code: RustCallStatusCode::Success, - error_buf: MaybeUninit::new(RustBuffer::new()), - } - } - fn test_callback(a: u8) -> Result { match a { 0 => Ok(100), @@ -232,7 +210,7 @@ mod test { #[test] fn test_rust_call() { - let mut status = create_call_status(); + let mut status = RustCallStatus::default(); let return_value = rust_call(&mut status, || { as LowerReturn>::lower_return(test_callback(0)) }); @@ -244,23 +222,21 @@ mod test { as LowerReturn>::lower_return(test_callback(1)) }); assert_eq!(status.code, RustCallStatusCode::Error); - unsafe { - assert_eq!( - >::try_lift(status.error_buf.assume_init()).unwrap(), - TestError("Error".to_owned()) - ); - } + assert_eq!( + >::try_lift(ManuallyDrop::into_inner(status.error_buf)) + .unwrap(), + TestError("Error".to_owned()) + ); - let mut status = create_call_status(); + let mut status = RustCallStatus::default(); rust_call(&mut status, || { as LowerReturn>::lower_return(test_callback(2)) }); assert_eq!(status.code, RustCallStatusCode::UnexpectedError); - unsafe { - assert_eq!( - >::try_lift(status.error_buf.assume_init()).unwrap(), - "Unexpected value: 2" - ); - } + assert_eq!( + >::try_lift(ManuallyDrop::into_inner(status.error_buf)) + .unwrap(), + "Unexpected value: 2" + ); } } diff --git a/uniffi_core/src/ffi/rustfuture/tests.rs b/uniffi_core/src/ffi/rustfuture/tests.rs index 369ef2eabc..67dd47525b 100644 --- a/uniffi_core/src/ffi/rustfuture/tests.rs +++ b/uniffi_core/src/ffi/rustfuture/tests.rs @@ -1,6 +1,7 @@ use once_cell::sync::OnceCell; use std::{ future::Future, + mem::ManuallyDrop, panic, pin::Pin, sync::{Arc, Mutex}, @@ -126,15 +127,13 @@ fn test_error() { let (_, call_status) = complete(rust_future); assert_eq!(call_status.code, RustCallStatusCode::Error); - unsafe { - assert_eq!( - >::try_lift_from_rust_buffer( - call_status.error_buf.assume_init() - ) - .unwrap(), - TestError::from("Something went wrong"), - ) - } + assert_eq!( + >::try_lift_from_rust_buffer(ManuallyDrop::into_inner( + call_status.error_buf + )) + .unwrap(), + TestError::from("Something went wrong"), + ) } // Once `complete` is called, the inner future should be released, even if wakers still hold a diff --git a/uniffi_core/src/ffi_converter_traits.rs b/uniffi_core/src/ffi_converter_traits.rs index 1b4fdb7333..1a7f2b2aac 100644 --- a/uniffi_core/src/ffi_converter_traits.rs +++ b/uniffi_core/src/ffi_converter_traits.rs @@ -50,7 +50,7 @@ //! These traits should not be used directly, only in generated code, and the generated code should //! have fixture tests to test that everything works correctly together. -use std::{borrow::Borrow, sync::Arc}; +use std::{borrow::Borrow, mem::ManuallyDrop, sync::Arc}; use anyhow::bail; use bytes::Buf; @@ -339,12 +339,12 @@ pub unsafe trait LiftReturn: Sized { Self::handle_callback_unexpected_error(UnexpectedUniFFICallbackError::new(e)) }), RustCallStatusCode::Error => { - Self::lift_error(unsafe { call_status.error_buf.assume_init() }) + Self::lift_error(ManuallyDrop::into_inner(call_status.error_buf)) } _ => { - let e = >::try_lift(unsafe { - call_status.error_buf.assume_init() - }) + let e = >::try_lift( + ManuallyDrop::into_inner(call_status.error_buf), + ) .unwrap_or_else(|e| format!("(Error lifting message: {e}")); Self::handle_callback_unexpected_error(UnexpectedUniFFICallbackError::new(e)) } diff --git a/uniffi_macros/src/export/callback_interface.rs b/uniffi_macros/src/export/callback_interface.rs index e9621913b7..fe60b85ae3 100644 --- a/uniffi_macros/src/export/callback_interface.rs +++ b/uniffi_macros/src/export/callback_interface.rs @@ -222,7 +222,7 @@ fn gen_method_impl(sig: &FnSignature, vtable_cell: &Ident) -> syn::Result #return_ty { let vtable = #vtable_cell.get(); - let mut uniffi_call_status = ::uniffi::RustCallStatus::new(); + let mut uniffi_call_status = ::uniffi::RustCallStatus::default(); let mut uniffi_return_value: #lift_return_type = ::uniffi::FfiDefault::ffi_default(); (vtable.#ident)(self.handle, #(#lower_exprs,)* &mut uniffi_return_value, &mut uniffi_call_status); #lift_foreign_return(uniffi_return_value, uniffi_call_status)