Handle::from_raw() and HandleAlloc::clone_handle()/consume_handle() a…

…re all unsafe. Fixes mozilla#2164
mhammond · Jul 22, 2024 · 4d51cbd · 4d51cbd
1 parent ca3f9d2
commit 4d51cbd
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 20 deletions.
diff --git a/uniffi_core/src/ffi/ffiserialize.rs b/uniffi_core/src/ffi/ffiserialize.rs
@@ -275,7 +275,7 @@ mod test {
             rust_buffer.len(),
             rust_buffer.capacity(),
         );
-        let handle = Handle::from_raw(101).unwrap();
+        let handle = unsafe { Handle::from_raw(101).unwrap() };
         let rust_call_status = RustCallStatus::default();
         let rust_call_status_error_buf = &rust_call_status.error_buf;
         let orig_rust_call_status_buffer_data = (

diff --git a/uniffi_core/src/ffi/handle.rs b/uniffi_core/src/ffi/handle.rs
@@ -28,7 +28,9 @@ impl Handle {
         self.0 as *const T
     }
 
-    pub fn from_raw(raw: u64) -> Option<Self> {
+    /// # Safety
+    /// The raw value must be a valid handle as described above.
+    pub unsafe fn from_raw(raw: u64) -> Option<Self> {
         if raw == 0 {
             None
         } else {

diff --git a/uniffi_core/src/ffi_converter_traits.rs b/uniffi_core/src/ffi_converter_traits.rs
@@ -449,18 +449,24 @@ pub unsafe trait HandleAlloc<UT>: Send + Sync {
     /// This creates a new handle from an existing one.
     /// It's used when the foreign code wants to pass back an owned handle and still keep a copy
     /// for themselves.
-    fn clone_handle(handle: Handle) -> Handle;
+    /// # Safety
+    /// The handle must be valid.
+    unsafe fn clone_handle(handle: Handle) -> Handle;
 
     /// Get a clone of the `Arc<>` using a "borrowed" handle.
     ///
-    /// Take care that the handle can not be destroyed between when it's passed and when
+    /// # Safety
+    /// The handle must be valid. Take care that the handle can
+    /// not be destroyed between when it's passed and when
     /// `get_arc()` is called.  #1797 is a cautionary tale.
-    fn get_arc(handle: Handle) -> Arc<Self> {
+    unsafe fn get_arc(handle: Handle) -> Arc<Self> {
         Self::consume_handle(Self::clone_handle(handle))
     }
 
     /// Consume a handle, getting back the initial `Arc<>`
-    fn consume_handle(handle: Handle) -> Arc<Self>;
+    /// # Safety
+    /// The handle must be valid.
+    unsafe fn consume_handle(handle: Handle) -> Arc<Self>;
 }
 
 /// Derive FFI traits
@@ -596,19 +602,15 @@ macro_rules! derive_ffi_traits {
                 $crate::Handle::from_pointer(::std::sync::Arc::into_raw(::std::sync::Arc::new(value)))
             }
 
-            fn clone_handle(handle: $crate::Handle) -> $crate::Handle {
-                unsafe {
-                    ::std::sync::Arc::<::std::sync::Arc<Self>>::increment_strong_count(handle.as_pointer::<::std::sync::Arc<Self>>());
-                }
+            unsafe fn clone_handle(handle: $crate::Handle) -> $crate::Handle {
+                ::std::sync::Arc::<::std::sync::Arc<Self>>::increment_strong_count(handle.as_pointer::<::std::sync::Arc<Self>>());
                 handle
             }
 
-            fn consume_handle(handle: $crate::Handle) -> ::std::sync::Arc<Self> {
-                unsafe {
-                    ::std::sync::Arc::<Self>::clone(
-                        &std::sync::Arc::<::std::sync::Arc::<Self>>::from_raw(handle.as_pointer::<::std::sync::Arc<Self>>())
-                    )
-                }
+            unsafe fn consume_handle(handle: $crate::Handle) -> ::std::sync::Arc<Self> {
+                ::std::sync::Arc::<Self>::clone(
+                    &std::sync::Arc::<::std::sync::Arc::<Self>>::from_raw(handle.as_pointer::<::std::sync::Arc<Self>>())
+                )
             }
         }
     };
@@ -626,12 +628,12 @@ unsafe impl<T: Send + Sync, UT> HandleAlloc<UT> for T {
         Handle::from_pointer(Arc::into_raw(value))
     }
 
-    fn clone_handle(handle: Handle) -> Handle {
-        unsafe { Arc::increment_strong_count(handle.as_pointer::<T>()) };
+    unsafe fn clone_handle(handle: Handle) -> Handle {
+        Arc::increment_strong_count(handle.as_pointer::<T>());
         handle
     }
 
-    fn consume_handle(handle: Handle) -> Arc<Self> {
-        unsafe { Arc::from_raw(handle.as_pointer()) }
+    unsafe fn consume_handle(handle: Handle) -> Arc<Self> {
+        Arc::from_raw(handle.as_pointer())
     }
 }