diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c4b3631ef..54e4a3fec8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ [All changes in [[UnreleasedUniFFIVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.25.0...HEAD). +### What's Fixed +- Fixed potential use-after-free error when lowering object handles (#1797) + ### What's changed? - The object handle FFI has changed. External bindings generators will need to update their code to diff --git a/fixtures/coverall/tests/bindings/test_handlerace.kts b/fixtures/coverall/tests/bindings/test_handlerace.kts deleted file mode 100644 index 580099e6f1..0000000000 --- a/fixtures/coverall/tests/bindings/test_handlerace.kts +++ /dev/null @@ -1,52 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -import java.util.concurrent.* -import uniffi.coverall.* - -// This tests the thread-safety of operations on object handles. -// The idea is to have a method call running on one thread while -// we try to destroy the object on another thread. This should -// be handled correctly at the Kotlin level, rather than triggering -// any fail-safe checks (or worse, memory unsafety!) in the underlying -// Rust code. -// -// This test was able to reproduce the concurrency issue in -// https://github.com/mozilla/uniffi-rs/issues/457, and is kept -// to prevent regressions. - -// Give ourselves enough opportunity to trigger concurrency issues. -for (i in 1..1000) { - // So we can operate concurrently. - val executor = Executors.newFixedThreadPool(2) - try { - // On the main thread here, we're going to create and then destroy a `Coveralls` instance. - val concurrentMethodCall: Future = Coveralls("test_handlerace").use { coveralls -> - // Make a method call on a separate thread thread. - val concurrentMethodCall = executor.submit(Callable { - coveralls.getName() - }) - // Sleep a little, to give the background thread a chance to start operating. - Thread.sleep(1) - concurrentMethodCall - } - // At this point the object has been destroyed. - // The concurrent method call should either have failed to run at all (if the object was destroyed - // before it started) or should have completely succeeded (if it started before the object was - // destroyed). It should not fail in the Rust code (which currently fails cleanly on handlemap - // safety checks, but in future might be a use-after-free). - try { - concurrentMethodCall.get() - } catch (e: ExecutionException) { - if (e.cause is IllegalStateException && e.message!!.endsWith("has already been destroyed")) { - // This is fine, we rejected the call after the object had been destroyed. - } else { - // Any other behaiour signals a possible problem. - throw e - } - } - } finally { - executor.shutdown() - } -} diff --git a/fixtures/coverall/tests/test_generated_bindings.rs b/fixtures/coverall/tests/test_generated_bindings.rs index 962f767964..ee5d69dfb2 100644 --- a/fixtures/coverall/tests/test_generated_bindings.rs +++ b/fixtures/coverall/tests/test_generated_bindings.rs @@ -3,5 +3,4 @@ uniffi::build_foreign_language_testcases!( "tests/bindings/test_coverall.kts", "tests/bindings/test_coverall.rb", "tests/bindings/test_coverall.swift", - "tests/bindings/test_handlerace.kts", ); diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt index b9ede7b453..25261913ff 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt @@ -34,102 +34,16 @@ inline fun T.use(block: (T) -> R) = // // This class provides core operations for working with the Rust handle to the live Rust struct on // the other side of the FFI. -// -// There's some subtlety here, because we have to be careful not to operate on a Rust -// struct after it has been dropped, and because we must expose a public API for freeing -// the Kotlin wrapper object in lieu of reliable finalizers. The core requirements are: -// -// * Each `FFIObject` instance holds an opaque handle to the underlying Rust struct. -// Method calls need to read this handle from the object's state and pass it in to -// the Rust FFI. -// -// * When an `FFIObject` is no longer needed, its handle should be passed to a -// special destructor function provided by the Rust FFI, which will drop the -// underlying Rust struct. -// -// * Given an `FFIObject` instance, calling code is expected to call the special -// `destroy` method in order to free it after use, either by calling it explicitly -// or by using a higher-level helper like the `use` method. Failing to do so will -// leak the underlying Rust struct. -// -// * We can't assume that calling code will do the right thing, and must be prepared -// to handle Kotlin method calls executing concurrently with or even after a call to -// `destroy`, and to handle multiple (possibly concurrent!) calls to `destroy`. -// -// * We must never allow Rust code to operate on the underlying Rust struct after -// the destructor has been called, and must never call the destructor more than once. -// Doing so may trigger memory unsafety. -// -// If we try to implement this with mutual exclusion on access to the handle, there is the -// possibility of a race between a method call and a concurrent call to `destroy`: -// -// * Thread A starts a method call, reads the value of the handle, but is interrupted -// before it can pass the handle over the FFI to Rust. -// * Thread B calls `destroy` and frees the underlying Rust struct. -// * Thread A resumes, passing the already-read handle value to Rust and triggering -// a use-after-free. -// -// One possible solution would be to use a `ReadWriteLock`, with each method call taking -// a read lock (and thus allowed to run concurrently) and the special `destroy` method -// taking a write lock (and thus blocking on live method calls). However, we aim not to -// generate methods with any hidden blocking semantics, and a `destroy` method that might -// block if called incorrectly seems to meet that bar. -// -// So, we achieve our goals by giving each `FFIObject` an associated `AtomicLong` counter to track -// the number of in-flight method calls, and an `AtomicBoolean` flag to indicate whether `destroy` -// has been called. These are updated according to the following rules: -// -// * The initial value of the counter is 1, indicating a live object with no in-flight calls. -// The initial value for the flag is false. -// -// * At the start of each method call, we atomically check the counter. -// If it is 0 then the underlying Rust struct has already been destroyed and the call is aborted. -// If it is nonzero them we atomically increment it by 1 and proceed with the method call. -// -// * At the end of each method call, we atomically decrement and check the counter. -// If it has reached zero then we destroy the underlying Rust struct. -// -// * When `destroy` is called, we atomically flip the flag from false to true. -// If the flag was already true we silently fail. -// Otherwise we atomically decrement and check the counter. -// If it has reached zero then we destroy the underlying Rust struct. -// -// Astute readers may observe that this all sounds very similar to the way that Rust's `Arc` works, -// and indeed it is, with the addition of a flag to guard against multiple calls to `destroy`. -// -// The overall effect is that the underlying Rust struct is destroyed only when `destroy` has been -// called *and* all in-flight method calls have completed, avoiding violating any of the expectations -// of the underlying Rust code. -// -// In the future we may be able to replace some of this with automatic finalization logic, such as using -// the new "Cleaner" functionaility in Java 9. The above scheme has been designed to work even if `destroy` is -// invoked by garbage-collection machinery rather than by calling code (which by the way, it's apparently also -// possible for the JVM to finalize an object while there is an in-flight call to one of its methods [1], -// so there would still be some complexity here). -// -// Sigh...all of this for want of a robust finalization mechanism. -// -// [1] https://stackoverflow.com/questions/24376768/can-java-finalize-an-object-when-it-is-still-in-scope/24380219 -// -abstract class FFIObject( - internal val handle: UniffiHandle -): Disposable, AutoCloseable { - +abstract class FFIObject(): Disposable, AutoCloseable { private val wasDestroyed = AtomicBoolean(false) - private val callCounter = AtomicLong(1) open protected fun freeRustArcPtr() { // To be overridden in subclasses. } override fun destroy() { - // Only allow a single call to this method. - // TODO: maybe we should log a warning if called more than once? if (this.wasDestroyed.compareAndSet(false, true)) { - // This decrement always matches the initial count of 1 given at creation time. - if (this.callCounter.decrementAndGet() == 0L) { - this.freeRustArcPtr() - } + this.freeRustArcPtr() } } @@ -137,27 +51,4 @@ abstract class FFIObject( override fun close() { this.destroy() } - - internal inline fun callWithHandle(block: (handle: UniffiHandle) -> R): R { - // Check and increment the call counter, to keep the object alive. - // This needs a compare-and-set retry loop in case of concurrent updates. - do { - val c = this.callCounter.get() - if (c == 0L) { - throw IllegalStateException("${this.javaClass.simpleName} object has already been destroyed") - } - if (c == Long.MAX_VALUE) { - throw IllegalStateException("${this.javaClass.simpleName} call counter would overflow") - } - } while (! this.callCounter.compareAndSet(c, c + 1L)) - // Now we can safely do the method call without the handle being freed concurrently. - try { - return block(this.handle) - } finally { - // This decrement always matches the increment we performed above. - if (this.callCounter.decrementAndGet() == 0L) { - this.freeRustArcPtr() - } - } - } } diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt index ed18c93451..99c9c5ecaa 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt @@ -6,8 +6,8 @@ {% include "Interface.kt" %} class {{ impl_class_name }}( - handle: UniffiHandle -) : FFIObject(handle), {{ interface_name }}{ + internal val handle: UniffiHandle +) : FFIObject(), {{ interface_name }}{ {%- match obj.primary_constructor() %} {%- when Some with (cons) %} @@ -30,6 +30,13 @@ class {{ impl_class_name }}( } } + internal fun uniffiCloneHandle(): UniffiHandle { + rustCall() { status -> + _UniFFILib.INSTANCE.{{ obj.ffi_object_inc_ref().name() }}(handle, status) + } + return handle + } + {% for meth in obj.methods() -%} {%- match meth.throws_type() -%} {%- when Some with (throwable) %} @@ -40,12 +47,10 @@ class {{ impl_class_name }}( @Suppress("ASSIGNED_BUT_NEVER_ACCESSED_VARIABLE") override suspend fun {{ meth.name()|fn_name }}({%- call kt::arg_list_decl(meth) -%}){% match meth.return_type() %}{% when Some with (return_type) %} : {{ return_type|type_name }}{% when None %}{%- endmatch %} { return uniffiRustCallAsync( - callWithHandle { uniffiHandle -> - _UniFFILib.INSTANCE.{{ meth.ffi_func().name() }}( - uniffiHandle, - {% call kt::arg_list_lowered(meth) %} - ) - }, + _UniFFILib.INSTANCE.{{ meth.ffi_func().name() }}( + uniffiCloneHandle(), + {% call kt::arg_list_lowered(meth) %} + ), { future, continuation -> _UniFFILib.INSTANCE.{{ meth.ffi_rust_future_poll(ci) }}(future, continuation) }, { future, status -> _UniFFILib.INSTANCE.{{ meth.ffi_rust_future_complete(ci) }}(future, status) }, { future -> _UniFFILib.INSTANCE.{{ meth.ffi_rust_future_free(ci) }}(future) }, @@ -69,17 +74,13 @@ class {{ impl_class_name }}( {%- match meth.return_type() -%} {%- when Some with (return_type) -%} override fun {{ meth.name()|fn_name }}({% call kt::arg_list_protocol(meth) %}): {{ return_type|type_name }} = - callWithHandle { - {%- call kt::to_ffi_call_with_prefix("it", meth) %} - }.let { + {%- call kt::to_ffi_call_with_prefix("uniffiCloneHandle()", meth) %}.let { {{ return_type|lift_fn }}(it) } {%- when None -%} override fun {{ meth.name()|fn_name }}({% call kt::arg_list_protocol(meth) %}) = - callWithHandle { - {%- call kt::to_ffi_call_with_prefix("it", meth) %} - } + {%- call kt::to_ffi_call_with_prefix("uniffiCloneHandle()", meth) %} {% endmatch %} {% endif %} {% endfor %} @@ -111,7 +112,7 @@ public object {{ obj|ffi_converter_name }}: FfiConverter<{{ type_name }}, Uniffi override fun lower(value: {{ type_name }}): UniffiHandle { {%- match obj.imp() %} {%- when ObjectImpl::Struct %} - return value.handle + return value.uniffiCloneHandle() {%- when ObjectImpl::Trait %} return UniffiHandle(handleMap.insert(value)) {%- endmatch %} diff --git a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py index 8cce1500db..aabe5552ad 100644 --- a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py @@ -21,6 +21,10 @@ def __del__(self): if handle is not None: _rust_call(_UniffiLib.{{ obj.ffi_object_free().name() }}, handle) + def _uniffi_clone_handle(self): + _rust_call(_UniffiLib.{{ obj.ffi_object_inc_ref().name() }}, self._uniffi_handle) + return self._uniffi_handle + # Used by alternative constructors or any methods which return this type. @classmethod def _make_instance_(cls, handle): @@ -89,7 +93,7 @@ def lower(value: {{ protocol_name }}): {%- when ObjectImpl::Struct %} if not isinstance(value, {{ impl_name }}): raise TypeError("Expected {{ impl_name }} instance, {} found".format(type(value).__name__)) - return value._uniffi_handle + return value._uniffi_clone_handle() {%- when ObjectImpl::Trait %} return {{ ffi_converter_name }}._handle_map.insert(value) {%- endmatch %} diff --git a/uniffi_bindgen/src/bindings/python/templates/macros.py b/uniffi_bindgen/src/bindings/python/templates/macros.py index e6cafa9f9f..99d48d343b 100644 --- a/uniffi_bindgen/src/bindings/python/templates/macros.py +++ b/uniffi_bindgen/src/bindings/python/templates/macros.py @@ -104,7 +104,7 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}): {%- call setup_args_extra_indent(meth) %} return _uniffi_rust_call_async( _UniffiLib.{{ meth.ffi_func().name() }}( - self._uniffi_handle, {% call arg_list_lowered(meth) %} + self._uniffi_clone_handle(), {% call arg_list_lowered(meth) %} ), _UniffiLib.{{ meth.ffi_rust_future_poll(ci) }}, _UniffiLib.{{ meth.ffi_rust_future_complete(ci) }}, @@ -133,14 +133,14 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}): def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}) -> "{{ return_type|type_name }}": {%- call setup_args_extra_indent(meth) %} return {{ return_type|lift_fn }}( - {% call to_ffi_call_with_prefix("self._uniffi_handle", meth) %} + {% call to_ffi_call_with_prefix("self._uniffi_clone_handle()", meth) %} ) {%- when None %} def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}): {%- call setup_args_extra_indent(meth) %} - {% call to_ffi_call_with_prefix("self._uniffi_handle", meth) %} + {% call to_ffi_call_with_prefix("self._uniffi_clone_handle()", meth) %} {% endmatch %} {% endif %} diff --git a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb index 21ce0167fc..4482ccfe88 100644 --- a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb +++ b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb @@ -21,6 +21,14 @@ def self._uniffi_define_finalizer_by_handle(handle, object_id) end end + def _uniffi_clone_handle() + {{ ci.namespace()|class_name_rb }}.rust_call( + :{{ obj.ffi_object_inc_ref().name() }}, + @handle + ) + return @handle + end + # A private helper for lowering instances into a handle. # This does an explicit typecheck, because accidentally lowering a different type of # object in a place where this type is expected, could lead to memory unsafety. @@ -28,7 +36,7 @@ def self._uniffi_lower(inst) if not inst.is_a? self raise TypeError.new "Expected a {{ obj.name()|class_name_rb }} instance, got #{inst}" end - return inst.instance_variable_get :@handle + return inst._uniffi_clone_handle() end {%- match obj.primary_constructor() %} @@ -58,14 +66,14 @@ def self.{{ cons.name()|fn_name_rb }}({% call rb::arg_list_decl(cons) %}) {%- when Some with (return_type) -%} def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %}) {%- call rb::coerce_args_extra_indent(meth) %} - result = {% call rb::to_ffi_call_with_prefix("@handle", meth) %} + result = {% call rb::to_ffi_call_with_prefix("_uniffi_clone_handle()", meth) %} return {{ "result"|lift_rb(return_type) }} end {%- when None -%} def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %}) {%- call rb::coerce_args_extra_indent(meth) %} - {% call rb::to_ffi_call_with_prefix("@handle", meth) %} + {% call rb::to_ffi_call_with_prefix("_uniffi_clone_handle()", meth) %} end {% endmatch %} {% endfor %} diff --git a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift index 0ab73c2550..6fc3e155d5 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift @@ -26,6 +26,11 @@ public class {{ impl_class_name }}: {{ protocol_name }} { try! rustCall { {{ obj.ffi_object_free().name() }}(handle, $0) } } + internal func uniffiCloneHandle() -> Int64 { + try! rustCall { {{ obj.ffi_object_inc_ref().name() }}(handle, $0) } + return handle + } + {% for cons in obj.alternate_constructors() %} public static func {{ cons.name()|fn_name }}({% call swift::arg_list_decl(cons) %}) {% call swift::throws(cons) %} -> {{ impl_class_name }} { @@ -42,7 +47,7 @@ public class {{ impl_class_name }}: {{ protocol_name }} { return {% call swift::try(meth) %} await uniffiRustCallAsync( rustFutureFunc: { {{ meth.ffi_func().name() }}( - self.handle + self.uniffiCloneHandle() {%- for arg in meth.arguments() -%} , {{ arg|lower_fn }}({{ arg.name()|var_name }}) @@ -75,14 +80,14 @@ public class {{ impl_class_name }}: {{ protocol_name }} { public func {{ meth.name()|fn_name }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} -> {{ return_type|type_name }} { return {% call swift::try(meth) %} {{ return_type|lift_fn }}( - {% call swift::to_ffi_call_with_prefix("self.handle", meth) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiCloneHandle()", meth) %} ) } {%- when None %} public func {{ meth.name()|fn_name }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} { - {% call swift::to_ffi_call_with_prefix("self.handle", meth) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiCloneHandle()", meth) %} } {%- endmatch -%} @@ -112,7 +117,8 @@ public struct {{ ffi_converter_name }}: FfiConverter { public static func lower(_ value: {{ type_name }}) -> Int64 { {%- match obj.imp() %} {%- when ObjectImpl::Struct %} - return value.handle + // inc-ref the current handle, then return the new reference. + return value.uniffiCloneHandle() {%- when ObjectImpl::Trait %} guard let ptr = UnsafeMutableRawPointer(bitPattern: UInt(truncatingIfNeeded: handleMap.insert(obj: value))) else { fatalError("Cast to UnsafeMutableRawPointer failed") diff --git a/uniffi_bindgen/src/interface/ffi.rs b/uniffi_bindgen/src/interface/ffi.rs index bd0fafae80..dddcfbc963 100644 --- a/uniffi_bindgen/src/interface/ffi.rs +++ b/uniffi_bindgen/src/interface/ffi.rs @@ -143,6 +143,24 @@ pub struct FfiFunction { } impl FfiFunction { + pub fn ffi_inc_ref(name: String) -> Self { + Self { + name, + arguments: vec![FfiArgument { + name: "handle".to_owned(), + type_: FfiType::Handle, + }], + ..Default::default() + } + } + + pub fn ffi_free(name: String) -> Self { + Self { + name, + ..Default::default() + } + } + pub fn callback_init(module_path: &str, trait_name: &str) -> Self { Self { name: uniffi_meta::init_callback_fn_symbol_name(module_path, trait_name), diff --git a/uniffi_bindgen/src/interface/object.rs b/uniffi_bindgen/src/interface/object.rs index f38ea8cfbc..52a1719de1 100644 --- a/uniffi_bindgen/src/interface/object.rs +++ b/uniffi_bindgen/src/interface/object.rs @@ -57,8 +57,6 @@ //! # Ok::<(), anyhow::Error>(()) //! ``` -use std::iter; - use anyhow::Result; use uniffi_meta::Checksum; @@ -100,6 +98,9 @@ pub struct Object { // avoids a weird circular dependency in the calculation. #[checksum_ignore] pub(super) ffi_func_free: FfiFunction, + // Increment the reference count for this object. Call this before lowering a handle. + #[checksum_ignore] + pub(super) ffi_func_inc_ref: FfiFunction, // Ffi function to initialize the foreign callback for trait interfaces #[checksum_ignore] pub(super) ffi_init_callback: Option, @@ -162,6 +163,10 @@ impl Object { &self.ffi_func_free } + pub fn ffi_object_inc_ref(&self) -> &FfiFunction { + &self.ffi_func_inc_ref + } + pub fn ffi_init_callback(&self) -> &FfiFunction { self.ffi_init_callback .as_ref() @@ -169,7 +174,8 @@ impl Object { } pub fn iter_ffi_function_definitions(&self) -> impl Iterator { - iter::once(&self.ffi_func_free) + [&self.ffi_func_free, &self.ffi_func_inc_ref] + .into_iter() .chain(&self.ffi_init_callback) .chain(self.constructors.iter().map(|f| &f.ffi_func)) .chain(self.methods.iter().map(|f| &f.ffi_func)) @@ -236,7 +242,8 @@ impl AsType for Object { impl From for Object { fn from(meta: uniffi_meta::ObjectMetadata) -> Self { - let ffi_free_name = meta.free_ffi_symbol_name(); + let ffi_func_inc_ref = FfiFunction::ffi_inc_ref(meta.inc_ref_ffi_symbol_name()); + let ffi_func_free = FfiFunction::ffi_free(meta.free_ffi_symbol_name()); Object { module_path: meta.module_path, name: meta.name, @@ -244,10 +251,8 @@ impl From for Object { constructors: Default::default(), methods: Default::default(), uniffi_traits: Default::default(), - ffi_func_free: FfiFunction { - name: ffi_free_name, - ..Default::default() - }, + ffi_func_inc_ref, + ffi_func_free, ffi_init_callback: None, } } diff --git a/uniffi_core/src/ffi/slab.rs b/uniffi_core/src/ffi/slab.rs index 61186c005c..6abc6c39ad 100644 --- a/uniffi_core/src/ffi/slab.rs +++ b/uniffi_core/src/ffi/slab.rs @@ -575,6 +575,10 @@ impl Slab { self.insert(value).unwrap_or_else(|e| panic!("{e}")) } + pub fn inc_ref_or_panic(&self, handle: Handle) { + self.inc_ref(handle).unwrap_or_else(|e| panic!("{e}")) + } + pub fn get_clone_or_panic(&self, handle: Handle) -> T { self.get_clone(handle).unwrap_or_else(|e| panic!("{e}")) } diff --git a/uniffi_core/src/ffi_converter_traits.rs b/uniffi_core/src/ffi_converter_traits.rs index f9101011f1..ed1fadbbb3 100644 --- a/uniffi_core/src/ffi_converter_traits.rs +++ b/uniffi_core/src/ffi_converter_traits.rs @@ -366,6 +366,7 @@ pub trait ConvertError: Sized { /// `&T` using the Arc. pub unsafe trait SlabAlloc { fn insert(value: Arc) -> Handle; + fn inc_ref(handle: Handle); fn get_clone(handle: Handle) -> Arc; fn remove(handle: Handle) -> Arc; } @@ -494,6 +495,10 @@ macro_rules! derive_ffi_traits { fn insert(value: ::std::sync::Arc) -> $crate::Handle { SLAB.insert_or_panic(value) } + fn inc_ref(handle: $crate::Handle) { + SLAB.inc_ref_or_panic(handle) + } + fn get_clone(handle: $crate::Handle) -> ::std::sync::Arc { SLAB.get_clone_or_panic(handle) } diff --git a/uniffi_macros/src/export/scaffolding.rs b/uniffi_macros/src/export/scaffolding.rs index 8f5f1cb480..5997a37435 100644 --- a/uniffi_macros/src/export/scaffolding.rs +++ b/uniffi_macros/src/export/scaffolding.rs @@ -143,7 +143,7 @@ impl ScaffoldingBits { // pointer. quote! { { - Ok(>::get_clone(uniffi_self_lowered)) + Ok(>::remove(uniffi_self_lowered)) } } } else { diff --git a/uniffi_macros/src/object.rs b/uniffi_macros/src/object.rs index 4355be88f3..71c73f6fa3 100644 --- a/uniffi_macros/src/object.rs +++ b/uniffi_macros/src/object.rs @@ -1,7 +1,6 @@ use proc_macro2::{Ident, Span, TokenStream}; use quote::quote; use syn::DeriveInput; -use uniffi_meta::free_fn_symbol_name; use crate::util::{ create_metadata_items, derive_ffi_traits, ident_to_string, mod_path, tagged_impl_header, @@ -11,7 +10,14 @@ pub fn expand_object(input: DeriveInput, udl_mode: bool) -> syn::Result syn::Result>::inc_ref(handle); + Ok(()) + }); + } + #[doc(hidden)] #[no_mangle] pub extern "C" fn #free_fn_ident( @@ -84,7 +102,7 @@ pub(crate) fn interface_impl(ident: &Ident, udl_mode: bool) -> TokenStream { /// Safety: the provided value must be a pointer previously obtained by calling /// the `lower()` or `write()` method of this impl. fn try_lift(v: Self::FfiType) -> ::uniffi::Result<::std::sync::Arc> { - Ok(<#ident as ::uniffi::SlabAlloc>::get_clone(v)) + Ok(<#ident as ::uniffi::SlabAlloc>::remove(v)) } /// When writing as a field of a complex structure, make a clone and transfer ownership diff --git a/uniffi_meta/src/ffi_names.rs b/uniffi_meta/src/ffi_names.rs index 44a5bc3e63..1e2e423d6c 100644 --- a/uniffi_meta/src/ffi_names.rs +++ b/uniffi_meta/src/ffi_names.rs @@ -39,6 +39,12 @@ pub fn free_fn_symbol_name(namespace: &str, object_name: &str) -> String { format!("uniffi_{namespace}_fn_free_{object_name}") } +/// FFI symbol name for the `inc_ref` function for an object. +pub fn inc_ref_fn_symbol_name(namespace: &str, object_name: &str) -> String { + let object_name = object_name.to_ascii_lowercase(); + format!("uniffi_{namespace}_fn_inc_ref_{object_name}") +} + /// FFI symbol name for the `init_callback` function for a callback interface pub fn init_callback_fn_symbol_name(namespace: &str, callback_interface_name: &str) -> String { let callback_interface_name = callback_interface_name.to_ascii_lowercase(); diff --git a/uniffi_meta/src/lib.rs b/uniffi_meta/src/lib.rs index 1c8a66801c..60cbf230b6 100644 --- a/uniffi_meta/src/lib.rs +++ b/uniffi_meta/src/lib.rs @@ -319,6 +319,14 @@ pub struct CallbackInterfaceMetadata { } impl ObjectMetadata { + /// FFI symbol name for the `inc_ref` function for this object. + /// + /// This function is used to increment the reference count before lowering an object to pass + /// back to Rust. + pub fn inc_ref_ffi_symbol_name(&self) -> String { + inc_ref_fn_symbol_name(&self.module_path, &self.name) + } + /// FFI symbol name for the `free` function for this object. /// /// This function is used to free the memory used by this object.