RustCallStatus::error_buf is now ManuallyDrop and not MaybeUninit.

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 = <RustCallStatus as FfiSerialize>::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(),

uniffi_core/src/ffi/foreignfuture.rs

@@ -147,7 +147,7 @@ mod test {
                 *data,
                 ForeignFutureResult {
                     return_value: <String as Lower<crate::UniFfiTag>>::lower(value),
-                    call_status: RustCallStatus::new(),
+                    call_status: RustCallStatus::default(),
                 },
             );
         }

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<RustBuffer>,
-    // 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<RustBuffer>,
 }
 
-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<String>) -> Self {
         Self {
             code: RustCallStatusCode::UnexpectedError,
-            error_buf: MaybeUninit::new(<String as Lower<UniFfiTag>>::lower(message.into())),
-        }
-    }
-}
-
-impl Default for RustCallStatus {
-    fn default() -> Self {
-        Self {
-            code: RustCallStatusCode::Success,
-            error_buf: MaybeUninit::uninit(),
+            error_buf: ManuallyDrop::new(<String as Lower<UniFfiTag>>::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
                 <String as Lower<UniFfiTag>>::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<i8, TestError> {
         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, || {
             <Result<i8, TestError> as LowerReturn<UniFfiTag>>::lower_return(test_callback(0))
         });
@@ -244,23 +222,21 @@ mod test {
             <Result<i8, TestError> as LowerReturn<UniFfiTag>>::lower_return(test_callback(1))
         });
         assert_eq!(status.code, RustCallStatusCode::Error);
-        unsafe {
-            assert_eq!(
-                <TestError as Lift<UniFfiTag>>::try_lift(status.error_buf.assume_init()).unwrap(),
-                TestError("Error".to_owned())
-            );
-        }
+        assert_eq!(
+            <TestError as Lift<UniFfiTag>>::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, || {
             <Result<i8, TestError> as LowerReturn<UniFfiTag>>::lower_return(test_callback(2))
         });
         assert_eq!(status.code, RustCallStatusCode::UnexpectedError);
-        unsafe {
-            assert_eq!(
-                <String as Lift<UniFfiTag>>::try_lift(status.error_buf.assume_init()).unwrap(),
-                "Unexpected value: 2"
-            );
-        }
+        assert_eq!(
+            <String as Lift<UniFfiTag>>::try_lift(ManuallyDrop::into_inner(status.error_buf))
+                .unwrap(),
+            "Unexpected value: 2"
+        );
     }
 }

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!(
-            <TestError as Lift<crate::UniFfiTag>>::try_lift_from_rust_buffer(
-                call_status.error_buf.assume_init()
-            )
-            .unwrap(),
-            TestError::from("Something went wrong"),
-        )
-    }
+    assert_eq!(
+        <TestError as Lift<crate::UniFfiTag>>::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

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<UT>: 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 = <String as FfiConverter<crate::UniFfiTag>>::try_lift(unsafe {
-                    call_status.error_buf.assume_init()
-                })
+                let e = <String as FfiConverter<crate::UniFfiTag>>::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))
             }

uniffi_macros/src/export/callback_interface.rs

@@ -222,7 +222,7 @@ fn gen_method_impl(sig: &FnSignature, vtable_cell: &Ident) -> syn::Result<TokenS
         Ok(quote! {
             fn #ident(#self_param, #(#params),*) -> #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)