From d380d164cfdba9e091c461baa6855f0a2294ac5b Mon Sep 17 00:00:00 2001 From: bendk Date: Mon, 4 Dec 2023 14:21:43 -0500 Subject: [PATCH] Clone objects when lowering them (#1797) (#1880) Currently, `lower()` always returns a borrow of the object handle. This is fine for function arguments, since you know the object is still alive on the stack while the function is being called. However, for function returns this is not correct. To fix this: clone the handle in `lower()`. Added a test for this -- it was surprisingly easy to cause a segfault with the current behavior. Removed the reexport-scaffolding-macro fixture which broke and often requires changes when the FFI changes. I don't think it's giving us enough value at this point to justify continuing to update it. --- Cargo.lock | 22 -- Cargo.toml | 1 - fixtures/coverall/src/coverall.udl | 4 + fixtures/coverall/src/lib.rs | 5 +- fixtures/coverall/src/traits.rs | 16 +- .../coverall/tests/bindings/test_coverall.kts | 11 + .../coverall/tests/bindings/test_coverall.py | 11 + .../tests/bindings/test_coverall.swift | 13 ++ .../reexport-scaffolding-macro/Cargo.toml | 24 --- .../reexport-scaffolding-macro/src/lib.rs | 197 ------------------ .../kotlin/templates/ObjectRuntime.kt | 13 +- .../kotlin/templates/ObjectTemplate.kt | 8 +- .../python/templates/ObjectTemplate.py | 9 +- .../src/bindings/python/templates/macros.py | 6 +- .../bindings/ruby/templates/ObjectTemplate.rb | 13 +- .../swift/templates/ObjectTemplate.swift | 20 +- uniffi_bindgen/src/interface/object.rs | 25 ++- uniffi_macros/src/export/scaffolding.rs | 4 +- uniffi_macros/src/export/trait_interface.rs | 37 +++- uniffi_macros/src/object.rs | 35 ++-- uniffi_meta/src/ffi_names.rs | 6 + uniffi_meta/src/lib.rs | 8 + 22 files changed, 199 insertions(+), 289 deletions(-) delete mode 100644 fixtures/reexport-scaffolding-macro/Cargo.toml delete mode 100644 fixtures/reexport-scaffolding-macro/src/lib.rs 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.