diff --git a/Cargo.lock b/Cargo.lock index b96207cad3..6228d794de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -819,16 +819,6 @@ version = "0.2.147" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4668fb0ea861c1df094127ac5f1da3409a82116a4ba74fca2e58ef927159bb3" -[[package]] -name = "libloading" -version = "0.7.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b67380fd3b2fbe7527a606e18729d21c6f3951633d0500574c4dc22d2d638b9f" -dependencies = [ - "cfg-if", - "winapi", -] - [[package]] name = "linux-raw-sys" version = "0.3.8" @@ -1868,18 +1858,6 @@ dependencies = [ "uniffi", ] -[[package]] -name = "uniffi-fixture-reexport-scaffolding-macro" -version = "0.22.0" -dependencies = [ - "cargo_metadata", - "libloading", - "uniffi", - "uniffi-fixture-callbacks", - "uniffi-fixture-coverall", - "uniffi_bindgen", -] - [[package]] name = "uniffi-fixture-regression-callbacks-omit-labels" version = "0.22.0" diff --git a/Cargo.toml b/Cargo.toml index 177898ee00..ccc77fe675 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,6 @@ members = [ "fixtures/keywords/swift", "fixtures/metadata", "fixtures/proc-macro", - "fixtures/reexport-scaffolding-macro", "fixtures/regressions/enum-without-i32-helpers", "fixtures/regressions/fully-qualified-types", "fixtures/regressions/kotlin-experimental-unsigned-types", diff --git a/fixtures/coverall/src/coverall.udl b/fixtures/coverall/src/coverall.udl index 1c0a6d48ab..578cfd6b82 100644 --- a/fixtures/coverall/src/coverall.udl +++ b/fixtures/coverall/src/coverall.udl @@ -27,6 +27,9 @@ namespace coverall { ReturnOnlyEnum output_return_only_enum(); void try_input_return_only_dict(ReturnOnlyDict d); + + Getters test_round_trip_through_rust(Getters getters); + void test_round_trip_through_foreign(Getters getters); }; dictionary SimpleDict { @@ -229,6 +232,7 @@ interface Getters { string? get_option(string v, boolean arg2); sequence get_list(sequence v, boolean arg2); void get_nothing(string v); + Coveralls round_trip_object(Coveralls coveralls); }; // Test trait #2 diff --git a/fixtures/coverall/src/lib.rs b/fixtures/coverall/src/lib.rs index ae43a7361c..301513de04 100644 --- a/fixtures/coverall/src/lib.rs +++ b/fixtures/coverall/src/lib.rs @@ -10,7 +10,10 @@ use std::time::SystemTime; use once_cell::sync::Lazy; mod traits; -pub use traits::{ancestor_names, get_traits, make_rust_getters, test_getters, Getters, NodeTrait}; +pub use traits::{ + ancestor_names, get_traits, make_rust_getters, test_getters, test_round_trip_through_foreign, + test_round_trip_through_rust, Getters, NodeTrait, +}; static NUM_ALIVE: Lazy> = Lazy::new(|| RwLock::new(0)); diff --git a/fixtures/coverall/src/traits.rs b/fixtures/coverall/src/traits.rs index 15785ef0c6..5ba65694ec 100644 --- a/fixtures/coverall/src/traits.rs +++ b/fixtures/coverall/src/traits.rs @@ -2,7 +2,7 @@ * 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/. */ -use super::{ComplexError, CoverallError}; +use super::{ComplexError, CoverallError, Coveralls}; use std::sync::{Arc, Mutex}; // namespace functions. @@ -41,6 +41,16 @@ pub trait Getters: Send + Sync { fn get_option(&self, v: String, arg2: bool) -> Result, ComplexError>; fn get_list(&self, v: Vec, arg2: bool) -> Vec; fn get_nothing(&self, v: String); + fn round_trip_object(&self, coveralls: Arc) -> Arc; +} + +pub fn test_round_trip_through_rust(getters: Arc) -> Arc { + getters +} + +pub fn test_round_trip_through_foreign(getters: Arc) { + let coveralls = getters.round_trip_object(Arc::new(Coveralls::new("round-trip".to_owned()))); + assert_eq!(coveralls.get_name(), "round-trip"); } struct RustGetters; @@ -90,6 +100,10 @@ impl Getters for RustGetters { } fn get_nothing(&self, _v: String) {} + + fn round_trip_object(&self, coveralls: Arc) -> Arc { + coveralls + } } pub fn make_rust_getters() -> Arc { diff --git a/fixtures/coverall/tests/bindings/test_coverall.kts b/fixtures/coverall/tests/bindings/test_coverall.kts index f091591601..eff626bad9 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.kts +++ b/fixtures/coverall/tests/bindings/test_coverall.kts @@ -280,6 +280,10 @@ class KotlinGetters : Getters { @Suppress("UNUSED_PARAMETER") override fun getNothing(v: String) = Unit + + override fun roundTripObject(coveralls: Coveralls): Coveralls { + return coveralls + } } // Test traits implemented in Rust @@ -395,6 +399,13 @@ getTraits().let { traits -> // not possible through the `NodeTrait` interface (see #1787). } +makeRustGetters().let { rustGetters -> + // Check that these don't cause use-after-free bugs + testRoundTripThroughRust(rustGetters) + + testRoundTripThroughForeign(KotlinGetters()) +} + // This tests that the UniFFI-generated scaffolding doesn't introduce any unexpected locking. // We have one thread busy-wait for a some period of time, while a second thread repeatedly // increments the counter and then checks if the object is still busy. The second thread should diff --git a/fixtures/coverall/tests/bindings/test_coverall.py b/fixtures/coverall/tests/bindings/test_coverall.py index 5f795c4f64..5b14c02467 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.py +++ b/fixtures/coverall/tests/bindings/test_coverall.py @@ -333,6 +333,9 @@ def get_list(self, v, arg2): def get_nothing(self, _v): return None + def round_trip_object(self, coveralls): + return coveralls + class PyNode: def __init__(self): self.parent = None @@ -432,5 +435,13 @@ def test_path(self): py_node.set_parent(None) traits[0].set_parent(None) + def test_round_tripping(self): + rust_getters = make_rust_getters(); + coveralls = Coveralls("test_round_tripping") + # Check that these don't cause use-after-free bugs + test_round_trip_through_rust(rust_getters) + + test_round_trip_through_foreign(PyGetters()) + if __name__=='__main__': unittest.main() diff --git a/fixtures/coverall/tests/bindings/test_coverall.swift b/fixtures/coverall/tests/bindings/test_coverall.swift index 6bf99be1fc..fb1e32b102 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.swift +++ b/fixtures/coverall/tests/bindings/test_coverall.swift @@ -315,6 +315,10 @@ class SwiftGetters: Getters { func getList(v: [Int32], arg2: Bool) -> [Int32] { arg2 ? v : [] } func getNothing(v: String) -> () { } + + func roundTripObject(coveralls: Coveralls) -> Coveralls { + return coveralls + } } @@ -444,3 +448,12 @@ do { swiftNode.setParent(parent: nil) traits[0].setParent(parent: nil) } + +// Test round tripping +do { + let rustGetters = makeRustGetters() + // Check that these don't cause use-after-free bugs + let _ = testRoundTripThroughRust(getters: rustGetters) + + testRoundTripThroughForeign(getters: SwiftGetters()) +} diff --git a/fixtures/reexport-scaffolding-macro/Cargo.toml b/fixtures/reexport-scaffolding-macro/Cargo.toml deleted file mode 100644 index d494000c26..0000000000 --- a/fixtures/reexport-scaffolding-macro/Cargo.toml +++ /dev/null @@ -1,24 +0,0 @@ -[package] -name = "uniffi-fixture-reexport-scaffolding-macro" -version = "0.22.0" -authors = ["Firefox Sync Team "] -edition = "2021" -license = "MPL-2.0" -publish = false - -# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html - -[lib] -name = "reexport_scaffolding_macro" -crate-type = ["lib", "cdylib"] - -[dependencies] -uniffi-fixture-callbacks = { path = "../callbacks" } -uniffi-fixture-coverall = { path = "../coverall" } -uniffi = { path = "../../uniffi", version = "0.25" } -uniffi_bindgen = { path = "../../uniffi_bindgen" } - -[dev-dependencies] -cargo_metadata = "0.15" -libloading = "0.7" -uniffi = { path = "../../uniffi", version = "0.25" } diff --git a/fixtures/reexport-scaffolding-macro/src/lib.rs b/fixtures/reexport-scaffolding-macro/src/lib.rs deleted file mode 100644 index 6bd04f2ccd..0000000000 --- a/fixtures/reexport-scaffolding-macro/src/lib.rs +++ /dev/null @@ -1,197 +0,0 @@ -uniffi_fixture_callbacks::uniffi_reexport_scaffolding!(); -uniffi_coverall::uniffi_reexport_scaffolding!(); - -#[cfg(test)] -mod tests { - use cargo_metadata::Message; - use libloading::{Library, Symbol}; - use std::ffi::CString; - use std::os::raw::c_void; - use std::process::{Command, Stdio}; - use uniffi::{FfiConverter, ForeignCallback, RustBuffer, RustCallStatus, RustCallStatusCode}; - use uniffi_bindgen::ComponentInterface; - - struct UniFfiTag; - - // Load the dynamic library that was built for this crate. The external functions from - // `uniffi_callbacks' and `uniffi_coverall` should be present. - pub fn load_library() -> Library { - let mut cmd = Command::new("cargo"); - cmd.arg("build").arg("--message-format=json").arg("--lib"); - cmd.stdout(Stdio::piped()); - let mut child = cmd.spawn().unwrap(); - let output = std::io::BufReader::new(child.stdout.take().unwrap()); - let artifacts = Message::parse_stream(output) - .filter_map(|message| match message { - Err(e) => panic!("{e}"), - Ok(Message::CompilerArtifact(artifact)) => { - if artifact.target.name == "reexport_scaffolding_macro" - && artifact.target.kind.iter().any(|item| item == "cdylib") - { - Some(artifact) - } else { - None - } - } - _ => None, - }) - .collect::>(); - if !child.wait().unwrap().success() { - panic!("Failed to execute `cargo build`"); - } - let artifact = match artifacts.len() { - 1 => &artifacts[0], - n => panic!("Found {n} artfiacts from cargo build"), - }; - let cdylib_files: Vec<_> = artifact - .filenames - .iter() - .filter(|nm| matches!(nm.extension(), Some(std::env::consts::DLL_EXTENSION))) - .collect(); - let library_path = match cdylib_files.len() { - 1 => cdylib_files[0].to_string(), - _ => panic!("Failed to build exactly one cdylib file"), - }; - unsafe { Library::new(library_path).unwrap() } - } - - pub fn has_symbol(library: &Library, name: &str) -> bool { - unsafe { - library - .get::(CString::new(name).unwrap().as_bytes_with_nul()) - .is_ok() - } - } - - pub fn get_symbol<'lib, T>(library: &'lib Library, name: &str) -> Symbol<'lib, T> { - unsafe { - library - .get::(CString::new(name).unwrap().as_bytes_with_nul()) - .unwrap() - } - } - - #[test] - fn test_symbols_present() { - let library = load_library(); - let coveralls_ci = ComponentInterface::from_webidl( - include_str!("../../coverall/src/coverall.udl"), - "uniffi_coverall", - ) - .unwrap(); - let callbacks_ci = ComponentInterface::from_webidl( - include_str!("../../callbacks/src/callbacks.udl"), - "uniffi_fixture_callbacks", - ) - .unwrap(); - - // UniFFI internal function - assert!(has_symbol::< - unsafe extern "C" fn(i32, &mut RustCallStatus) -> RustBuffer, - >( - &library, coveralls_ci.ffi_rustbuffer_alloc().name() - )); - - // Top-level function - assert!( - has_symbol:: u64>( - &library, - coveralls_ci - .get_function_definition("get_num_alive") - .unwrap() - .ffi_func() - .name() - ) - ); - - // Object method - assert!( - has_symbol:: u64>( - &library, - coveralls_ci - .get_object_definition("Coveralls") - .unwrap() - .get_method("get_name") - .ffi_func() - .name() - ) - ); - - // Callback init func - assert!(has_symbol::< - unsafe extern "C" fn(ForeignCallback, &mut RustCallStatus) -> (), - >( - &library, - callbacks_ci - .get_callback_interface_definition("ForeignGetters") - .unwrap() - .ffi_init_callback() - .name() - )); - } - - #[test] - fn test_calls() { - let mut call_status = RustCallStatus::default(); - let library = load_library(); - let coveralls_ci = ComponentInterface::from_webidl( - include_str!("../../coverall/src/coverall.udl"), - "uniffi_coverall", - ) - .unwrap(); - let object_def = coveralls_ci.get_object_definition("Coveralls").unwrap(); - - let get_num_alive: Symbol u64> = get_symbol( - &library, - coveralls_ci - .get_function_definition("get_num_alive") - .unwrap() - .ffi_func() - .name(), - ); - let coveralls_new: Symbol< - unsafe extern "C" fn(RustBuffer, &mut RustCallStatus) -> *const c_void, - > = get_symbol( - &library, - object_def.primary_constructor().unwrap().ffi_func().name(), - ); - let coveralls_get_name: Symbol< - unsafe extern "C" fn(*const c_void, &mut RustCallStatus) -> RustBuffer, - > = get_symbol( - &library, - object_def.get_method("get_name").ffi_func().name(), - ); - let coveralls_free: Symbol ()> = - get_symbol(&library, object_def.ffi_object_free().name()); - - let num_alive = unsafe { get_num_alive(&mut call_status) }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - assert_eq!(num_alive, 0); - - let obj_id = unsafe { - coveralls_new( - >::lower("TestName".into()), - &mut call_status, - ) - }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - - let name_buf = unsafe { coveralls_get_name(obj_id, &mut call_status) }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - assert_eq!( - >::try_lift(name_buf).unwrap(), - "TestName" - ); - - let num_alive = unsafe { get_num_alive(&mut call_status) }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - assert_eq!(num_alive, 1); - - unsafe { coveralls_free(obj_id, &mut call_status) }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - - let num_alive = unsafe { get_num_alive(&mut call_status) }; - assert_eq!(call_status.code, RustCallStatusCode::Success); - assert_eq!(num_alive, 0); - } -} diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt index 216aeaf035..fa22b6ad2b 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt @@ -106,7 +106,14 @@ abstract class FFIObject: Disposable, AutoCloseable { private val callCounter = AtomicLong(1) open protected fun freeRustArcPtr() { - // To be overridden in subclasses. + // Overridden by generated subclasses, the default method exists to allow users to manually + // implement the interface + } + + open fun uniffiClonePointer(): Pointer { + // Overridden by generated subclasses, the default method exists to allow users to manually + // implement the interface + throw RuntimeException("uniffiClonePointer not implemented") } override fun destroy() { @@ -139,7 +146,7 @@ abstract class FFIObject: Disposable, AutoCloseable { } while (! this.callCounter.compareAndSet(c, c + 1L)) // Now we can safely do the method call without the pointer being freed concurrently. try { - return block(this.pointer!!) + return block(this.uniffiClonePointer()) } finally { // This decrement always matches the increment we performed above. if (this.callCounter.decrementAndGet() == 0L) { @@ -150,4 +157,4 @@ abstract class FFIObject: Disposable, AutoCloseable { } /** Used to instantiate a [FFIObject] without an actual pointer, for fakes in tests, mostly. */ -object NoPointer \ No newline at end of file +object NoPointer diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt index 50d18824bf..797d595f11 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt @@ -29,6 +29,12 @@ open class {{ impl_class_name }} : FFIObject, {{ interface_name }} { {%- when None %} {%- endmatch %} + override fun uniffiClonePointer(): Pointer { + return rustCall() { status -> + _UniFFILib.INSTANCE.{{ obj.ffi_object_clone().name() }}(pointer!!, status) + } + } + /** * Disconnect the object from the underlying Rust object. * @@ -165,7 +171,7 @@ public object {{ obj|ffi_converter_name }}: FfiConverter<{{ type_name }}, Pointe override fun lower(value: {{ type_name }}): Pointer { {%- match obj.imp() %} {%- when ObjectImpl::Struct %} - return value.callWithPointer { it } + return value.uniffiClonePointer() {%- when ObjectImpl::Trait %} return Pointer(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 aa6fade40f..c68808d01f 100644 --- a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py @@ -25,6 +25,9 @@ def __del__(self): if pointer is not None: _rust_call(_UniffiLib.{{ obj.ffi_object_free().name() }}, pointer) + def _uniffi_clone_pointer(self): + return _rust_call(_UniffiLib.{{ obj.ffi_object_clone().name() }}, self._pointer) + # Used by alternative constructors or any methods which return this type. @classmethod def _make_instance_(cls, pointer): @@ -60,13 +63,13 @@ def __eq__(self, other: object) -> {{ eq.return_type().unwrap()|type_name }}: if not isinstance(other, {{ type_name }}): return NotImplemented - return {{ eq.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self._pointer", eq) %}) + return {{ eq.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self._uniffi_clone_pointer()", eq) %}) def __ne__(self, other: object) -> {{ ne.return_type().unwrap()|type_name }}: if not isinstance(other, {{ type_name }}): return NotImplemented - return {{ ne.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self._pointer", ne) %}) + return {{ ne.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self._uniffi_clone_pointer()", ne) %}) {%- when UniffiTrait::Hash { hash } %} {%- call py::method_decl("__hash__", hash) %} {% endmatch %} @@ -103,7 +106,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._pointer + return value._uniffi_clone_pointer() {%- 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 109257cfae..80f6bf749f 100644 --- a/uniffi_bindgen/src/bindings/python/templates/macros.py +++ b/uniffi_bindgen/src/bindings/python/templates/macros.py @@ -120,7 +120,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._pointer, {% call arg_list_lowered(meth) %} + self._uniffi_clone_pointer(), {% call arg_list_lowered(meth) %} ), _UniffiLib.{{ meth.ffi_rust_future_poll(ci) }}, _UniffiLib.{{ meth.ffi_rust_future_complete(ci) }}, @@ -150,7 +150,7 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}) -> "{{ return_typ {%- call docstring(meth, 8) %} {%- call setup_args_extra_indent(meth) %} return {{ return_type|lift_fn }}( - {% call to_ffi_call_with_prefix("self._pointer", meth) %} + {% call to_ffi_call_with_prefix("self._uniffi_clone_pointer()", meth) %} ) {%- when None %} @@ -158,7 +158,7 @@ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}) -> "{{ return_typ def {{ py_method_name }}(self, {% call arg_list_decl(meth) %}): {%- call docstring(meth, 8) %} {%- call setup_args_extra_indent(meth) %} - {% call to_ffi_call_with_prefix("self._pointer", meth) %} + {% call to_ffi_call_with_prefix("self._uniffi_clone_pointer()", meth) %} {% endmatch %} {% endif %} diff --git a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb index 2c085a2dec..e2e5fd951a 100644 --- a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb +++ b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb @@ -31,8 +31,15 @@ def self._uniffi_check_lower(inst) end end + def _uniffi_clone_pointer() + return {{ ci.namespace()|class_name_rb }}.rust_call( + :{{ obj.ffi_object_clone().name() }}, + @pointer + ) + end + def self._uniffi_lower(inst) - return inst.instance_variable_get :@pointer + return inst._uniffi_clone_pointer() end {%- match obj.primary_constructor() %} @@ -62,14 +69,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::setup_args_extra_indent(meth) %} - result = {% call rb::to_ffi_call_with_prefix("@pointer", meth) %} + result = {% call rb::to_ffi_call_with_prefix("_uniffi_clone_pointer()", meth) %} return {{ "result"|lift_rb(return_type) }} end {%- when None -%} def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %}) {%- call rb::setup_args_extra_indent(meth) %} - {% call rb::to_ffi_call_with_prefix("@pointer", meth) %} + {% call rb::to_ffi_call_with_prefix("_uniffi_clone_pointer()", 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 5eca690e5a..173a98b8de 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift @@ -30,6 +30,10 @@ public class {{ impl_class_name }}: self.pointer = pointer } + public func uniffiClonePointer() -> UnsafeMutableRawPointer { + return try! rustCall { {{ obj.ffi_object_clone().name() }}(self.pointer, $0) } + } + {%- match obj.primary_constructor() %} {%- when Some with (cons) %} {%- call swift::docstring(cons, 4) %} @@ -59,7 +63,7 @@ public class {{ impl_class_name }}: return {% call swift::try(meth) %} await uniffiRustCallAsync( rustFutureFunc: { {{ meth.ffi_func().name() }}( - self.pointer + self.uniffiClonePointer() {%- for arg in meth.arguments() -%} , {{ arg|lower_fn }}({{ arg.name()|var_name }}) @@ -92,14 +96,14 @@ public class {{ impl_class_name }}: {%- call swift::docstring(meth, 4) %} 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.pointer", meth) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiClonePointer()", meth) %} ) } {%- when None %} {%- call swift::docstring(meth, 4) %} public func {{ meth.name()|fn_name }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} { - {% call swift::to_ffi_call_with_prefix("self.pointer", meth) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiClonePointer()", meth) %} } {%- endmatch -%} @@ -111,25 +115,25 @@ public class {{ impl_class_name }}: {%- when UniffiTrait::Display { fmt } %} public var description: String { return {% call swift::try(fmt) %} {{ fmt.return_type().unwrap()|lift_fn }}( - {% call swift::to_ffi_call_with_prefix("self.pointer", fmt) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiClonePointer()", fmt) %} ) } {%- when UniffiTrait::Debug { fmt } %} public var debugDescription: String { return {% call swift::try(fmt) %} {{ fmt.return_type().unwrap()|lift_fn }}( - {% call swift::to_ffi_call_with_prefix("self.pointer", fmt) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiClonePointer()", fmt) %} ) } {%- when UniffiTrait::Eq { eq, ne } %} public static func == (lhs: {{ impl_class_name }}, other: {{ impl_class_name }}) -> Bool { return {% call swift::try(eq) %} {{ eq.return_type().unwrap()|lift_fn }}( - {% call swift::to_ffi_call_with_prefix("lhs.pointer", eq) %} + {% call swift::to_ffi_call_with_prefix("lhs.uniffiClonePointer()", eq) %} ) } {%- when UniffiTrait::Hash { hash } %} public func hash(into hasher: inout Hasher) { let val = {% call swift::try(hash) %} {{ hash.return_type().unwrap()|lift_fn }}( - {% call swift::to_ffi_call_with_prefix("self.pointer", hash) %} + {% call swift::to_ffi_call_with_prefix("self.uniffiClonePointer()", hash) %} ) hasher.combine(val) } @@ -161,7 +165,7 @@ public struct {{ ffi_converter_name }}: FfiConverter { public static func lower(_ value: {{ type_name }}) -> UnsafeMutableRawPointer { {%- match obj.imp() %} {%- when ObjectImpl::Struct %} - return value.pointer + return value.uniffiClonePointer() {%- 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/object.rs b/uniffi_bindgen/src/interface/object.rs index 3b4c3838d6..06e32b3a37 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; @@ -98,6 +96,11 @@ pub struct Object { // - its `name` property includes a checksum derived from the very // hash value we're trying to calculate here, so excluding it // avoids a weird circular dependency in the calculation. + + // FFI function to clone a pointer for this object + #[checksum_ignore] + pub(super) ffi_func_clone: FfiFunction, + // FFI function to free a pointer for this object #[checksum_ignore] pub(super) ffi_func_free: FfiFunction, // Ffi function to initialize the foreign callback for trait interfaces @@ -160,6 +163,10 @@ impl Object { self.uniffi_traits.iter().collect() } + pub fn ffi_object_clone(&self) -> &FfiFunction { + &self.ffi_func_clone + } + pub fn ffi_object_free(&self) -> &FfiFunction { &self.ffi_func_free } @@ -175,7 +182,8 @@ impl Object { } pub fn iter_ffi_function_definitions(&self) -> impl Iterator { - iter::once(&self.ffi_func_free) + [&self.ffi_func_clone, &self.ffi_func_free] + .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)) @@ -193,7 +201,13 @@ impl Object { } pub fn derive_ffi_funcs(&mut self) -> Result<()> { + assert!(!self.ffi_func_clone.name().is_empty()); assert!(!self.ffi_func_free.name().is_empty()); + self.ffi_func_clone.arguments = vec![FfiArgument { + name: "ptr".to_string(), + type_: FfiType::RustArcPtr(self.name.to_string()), + }]; + self.ffi_func_clone.return_type = Some(FfiType::RustArcPtr(self.name.to_string())); self.ffi_func_free.arguments = vec![FfiArgument { name: "ptr".to_string(), type_: FfiType::RustArcPtr(self.name.to_string()), @@ -242,6 +256,7 @@ impl AsType for Object { impl From for Object { fn from(meta: uniffi_meta::ObjectMetadata) -> Self { + let ffi_clone_name = meta.clone_ffi_symbol_name(); let ffi_free_name = meta.free_ffi_symbol_name(); Object { module_path: meta.module_path, @@ -250,6 +265,10 @@ impl From for Object { constructors: Default::default(), methods: Default::default(), uniffi_traits: Default::default(), + ffi_func_clone: FfiFunction { + name: ffi_clone_name, + ..Default::default() + }, ffi_func_free: FfiFunction { name: ffi_free_name, ..Default::default() diff --git a/uniffi_macros/src/export/scaffolding.rs b/uniffi_macros/src/export/scaffolding.rs index d00d8403bd..98843a3cde 100644 --- a/uniffi_macros/src/export/scaffolding.rs +++ b/uniffi_macros/src/export/scaffolding.rs @@ -143,9 +143,9 @@ impl ScaffoldingBits { // pointer. quote! { { - let foreign_arc = ::std::boxed::Box::leak(unsafe { Box::from_raw(uniffi_self_lowered as *mut ::std::sync::Arc) }); + let boxed_foreign_arc = unsafe { Box::from_raw(uniffi_self_lowered as *mut ::std::sync::Arc) }; // Take a clone for our own use. - Ok(::std::sync::Arc::clone(foreign_arc)) + Ok(*boxed_foreign_arc) } } } else { diff --git a/uniffi_macros/src/export/trait_interface.rs b/uniffi_macros/src/export/trait_interface.rs index ccd284329a..3b7f3cfa08 100644 --- a/uniffi_macros/src/export/trait_interface.rs +++ b/uniffi_macros/src/export/trait_interface.rs @@ -13,7 +13,6 @@ use crate::{ object::interface_meta_static_var, util::{ident_to_string, tagged_impl_header}, }; -use uniffi_meta::free_fn_symbol_name; pub(super) fn gen_trait_scaffolding( mod_path: &str, @@ -30,15 +29,43 @@ pub(super) fn gen_trait_scaffolding( let trait_impl = callback_interface::trait_impl(mod_path, &self_ident, &items) .unwrap_or_else(|e| e.into_compile_error()); + let clone_fn_ident = Ident::new( + &uniffi_meta::clone_fn_symbol_name(mod_path, &trait_name), + Span::call_site(), + ); let free_fn_ident = Ident::new( - &free_fn_symbol_name(mod_path, &trait_name), + &uniffi_meta::free_fn_symbol_name(mod_path, &trait_name), Span::call_site(), ); - let free_tokens = quote! { + let helper_fn_tokens = quote! { + #[doc(hidden)] + #[no_mangle] + /// Clone a pointer to this object type + /// + /// Safety: Only pass pointers returned by a UniFFI call. Do not pass pointers that were + /// passed to the free function. + pub unsafe extern "C" fn #clone_fn_ident( + ptr: *const ::std::ffi::c_void, + call_status: &mut ::uniffi::RustCallStatus + ) -> *const ::std::ffi::c_void { + uniffi::rust_call(call_status, || { + let ptr = ptr as *mut std::sync::Arc; + let arc = unsafe { ::std::sync::Arc::clone(&*ptr) }; + Ok(::std::boxed::Box::into_raw(::std::boxed::Box::new(arc)) as *const ::std::ffi::c_void) + }) + } + #[doc(hidden)] #[no_mangle] - pub extern "C" fn #free_fn_ident( + /// Free a pointer to this object type + /// + /// Safety: Only pass pointers returned by a UniFFI call. Do not pass pointers that were + /// passed to the free function. + /// + /// Note: clippy doesn't complain about this being unsafe, but it definitely is since it + /// calls `Box::from_raw`. + pub unsafe extern "C" fn #free_fn_ident( ptr: *const ::std::ffi::c_void, call_status: &mut ::uniffi::RustCallStatus ) { @@ -74,7 +101,7 @@ pub(super) fn gen_trait_scaffolding( Ok(quote_spanned! { self_ident.span() => #meta_static_var - #free_tokens + #helper_fn_tokens #trait_impl #impl_tokens #ffi_converter_tokens diff --git a/uniffi_macros/src/object.rs b/uniffi_macros/src/object.rs index 6f8c0ac48f..1fb90a49af 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, extract_docstring, ident_to_string, mod_path, tagged_impl_header, @@ -12,7 +11,14 @@ pub fn expand_object(input: DeriveInput, udl_mode: bool) -> syn::Result syn::Result *const ::std::ffi::c_void { + uniffi::rust_call(call_status, || { + unsafe { ::std::sync::Arc::increment_strong_count(ptr) }; + Ok(ptr) + }) + } + + #[doc(hidden)] + #[no_mangle] + pub unsafe extern "C" fn #free_fn_ident( ptr: *const ::std::ffi::c_void, call_status: &mut ::uniffi::RustCallStatus ) { @@ -81,17 +99,10 @@ pub(crate) fn interface_impl(ident: &Ident, udl_mode: bool) -> TokenStream { ::std::sync::Arc::into_raw(obj) as Self::FfiType } - /// When lifting, we receive a "borrow" of the `Arc` that is owned by - /// the foreign-language code, and make a clone of it for our own use. - /// - /// Safety: the provided value must be a pointer previously obtained by calling - /// the `lower()` or `write()` method of this impl. + /// When lifting, we receive an owned `Arc` that the foreign language code cloned. fn try_lift(v: Self::FfiType) -> ::uniffi::Result<::std::sync::Arc> { let v = v as *const #ident; - // We musn't drop the `Arc` that is owned by the foreign-language code. - let foreign_arc = ::std::mem::ManuallyDrop::new(unsafe { ::std::sync::Arc::::from_raw(v) }); - // Take a clone for our own use. - Ok(::std::sync::Arc::clone(&*foreign_arc)) + Ok(unsafe { ::std::sync::Arc::::from_raw(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..a58977b790 100644 --- a/uniffi_meta/src/ffi_names.rs +++ b/uniffi_meta/src/ffi_names.rs @@ -33,6 +33,12 @@ pub fn method_symbol_name(namespace: &str, object_name: &str, name: &str) -> Str format!("uniffi_{namespace}_fn_method_{object_name}_{name}") } +/// FFI symbol name for the `clone` function for an object. +pub fn clone_fn_symbol_name(namespace: &str, object_name: &str) -> String { + let object_name = object_name.to_ascii_lowercase(); + format!("uniffi_{namespace}_fn_clone_{object_name}") +} + /// FFI symbol name for the `free` function for an object. pub fn free_fn_symbol_name(namespace: &str, object_name: &str) -> String { let object_name = object_name.to_ascii_lowercase(); diff --git a/uniffi_meta/src/lib.rs b/uniffi_meta/src/lib.rs index d2b150d0e9..05a759fe59 100644 --- a/uniffi_meta/src/lib.rs +++ b/uniffi_meta/src/lib.rs @@ -329,6 +329,14 @@ pub struct CallbackInterfaceMetadata { } impl ObjectMetadata { + /// FFI symbol name for the `clone` function for this object. + /// + /// This function is used to increment the reference count before lowering an object to pass + /// back to Rust. + pub fn clone_ffi_symbol_name(&self) -> String { + clone_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.