Fixes for returning objects (mozilla#1797)

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.

fixtures/coverall/src/coverall.udl

@@ -22,6 +22,9 @@ namespace coverall {
     void test_getters(Getters g);
 
     sequence<string> ancestor_names(NodeTrait node);
+
+    Getters test_round_trip_through_rust(Getters getters);
+    void test_round_trip_through_foreign(Getters getters);
 };
 
 dictionary SimpleDict {
@@ -207,6 +210,7 @@ interface Getters {
     string? get_option(string v, boolean arg2);
     sequence<i32> get_list(sequence<i32> v, boolean arg2);
     void get_nothing(string v);
+    Coveralls round_trip_object(Coveralls coveralls);
 };
 
 // Test trait #2

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<RwLock<u64>> = Lazy::new(|| RwLock::new(0));
 

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<Option<String>, ComplexError>;
     fn get_list(&self, v: Vec<i32>, arg2: bool) -> Vec<i32>;
     fn get_nothing(&self, v: String);
+    fn round_trip_object(&self, coveralls: Arc<Coveralls>) -> Arc<Coveralls>;
+}
+
+pub fn test_round_trip_through_rust(getters: Arc<dyn Getters>) -> Arc<dyn Getters> {
+    getters
+}
+
+pub fn test_round_trip_through_foreign(getters: Arc<dyn Getters>) {
+    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<Coveralls>) -> Arc<Coveralls> {
+        coveralls
+    }
 }
 
 pub fn make_rust_getters() -> Arc<dyn Getters> {

fixtures/coverall/tests/bindings/test_coverall.kts

@@ -257,6 +257,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
@@ -372,6 +376,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

fixtures/coverall/tests/bindings/test_coverall.py

@@ -314,6 +314,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
@@ -413,5 +416,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()

fixtures/coverall/tests/bindings/test_coverall.swift

@@ -283,6 +283,10 @@ class SwiftGetters: Getters {
     func getList(v: [Int32], arg2: Bool) -> [Int32] { arg2 ? v : [] }
     func getNothing(v: String) -> () {
     }
+
+    func roundTripObject(coveralls: Coveralls) -> Coveralls {
+        return coveralls
+    }
 }
 
 
@@ -412,3 +416,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())
+}

uniffi_bindgen/src/bindings/kotlin/templates/ObjectRuntime.kt

@@ -132,6 +132,12 @@ abstract class FFIObject(
         // To be overridden in subclasses.
     }
 
+    fun uniffiCloneHandle(): UniffiHandle {
+        return rustCall() { status ->
+            _UniFFILib.INSTANCE.{{ obj.ffi_object_clone().name() }}(handle, status)
+        }
+    }
+
     override fun destroy() {
         // Only allow a single call to this method.
         // TODO: maybe we should log a warning if called more than once?
@@ -162,7 +168,7 @@ abstract class FFIObject(
         } 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)
+            return block(this.uniffiCloneHandle())
         } finally {
             // This decrement always matches the increment we performed above.
             if (this.callCounter.decrementAndGet() == 0L) {

uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt

@@ -109,7 +109,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 handleMap.newHandle(value)
         {%- endmatch %}

uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py

@@ -21,6 +21,9 @@ def __del__(self):
         if handle is not None:
             _rust_call(_UniffiLib.{{ obj.ffi_object_free().name() }}, handle)
 
+    def uniffi_clone_handle(self):
+        return _rust_call(_UniffiLib.{{ obj.ffi_object_clone().name() }}, self._uniffi_handle)
+
     # Used by alternative constructors or any methods which return this type.
     @classmethod
     def _make_instance_(cls, handle):
@@ -54,13 +57,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._uniffi_handle", eq) %})
+        return {{ eq.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self.uniffi_clone_handle()", 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._uniffi_handle", ne) %})
+        return {{ ne.return_type().unwrap()|lift_fn }}({% call py::to_ffi_call_with_prefix("self.uniffi_clone_handle()", ne) %})
 {%-         when UniffiTrait::Hash { hash } %}
             {%- call py::method_decl("__hash__", hash) %}
 {%      endmatch %}
@@ -95,9 +98,7 @@ def check(value: {{ type_name }}):
     def lower(value: {{ type_name }}):
         {%- match obj.imp() %}
         {%- 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.new_handle(value)
         {%- endmatch %}

uniffi_bindgen/src/bindings/python/templates/macros.py

@@ -106,7 +106,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) }},
@@ -135,14 +135,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 %}
 

uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb

@@ -32,7 +32,14 @@ def self._uniffi_check(inst)
   end
 
   def self._uniffi_lower(inst)
-    return inst.instance_variable_get :@handle
+    return inst._uniffi_clone_handle()
+  end
+
+  def _uniffi_clone_handle()
+    return {{ ci.namespace()|class_name_rb }}.rust_call(
+      :{{ obj.ffi_object_clone().name() }},
+      @handle
+    )
   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("@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::setup_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 %}

uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift

@@ -14,6 +14,10 @@ public class {{ impl_class_name }}: {{ protocol_name }} {
         self.handle = handle
     }
 
+    public func uniffiCloneHandle() -> UInt64 {
+        return try! rustCall { {{ obj.ffi_object_clone().name() }}(self.handle, $0) }
+    }
+
     {%- match obj.primary_constructor() %}
     {%- when Some with (cons) %}
     public convenience init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} {
@@ -42,7 +46,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 +79,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 +116,7 @@ public struct {{ ffi_converter_name }}: FfiConverter {
     public static func lower(_ value: {{ type_name }}) -> UInt64 {
         {%- match obj.imp() %}
         {%- when ObjectImpl::Struct %}
-        return value.handle
+        return value.uniffiCloneHandle()
         {%- when ObjectImpl::Trait %}
         return handleMap.newHandle(obj: value)
         {%- endmatch %}

uniffi_bindgen/src/interface/ffi.rs

@@ -141,6 +141,29 @@ pub struct FfiFunction {
 }
 
 impl FfiFunction {
+    pub fn ffi_clone(name: String) -> Self {
+        Self {
+            name,
+            arguments: vec![FfiArgument {
+                name: "handle".to_owned(),
+                type_: FfiType::Handle,
+            }],
+            return_type: Some(FfiType::Handle),
+            ..Default::default()
+        }
+    }
+
+    pub fn ffi_free(name: String) -> Self {
+        Self {
+            name,
+            arguments: vec![FfiArgument {
+                name: "handle".to_owned(),
+                type_: FfiType::Handle,
+            }],
+            ..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),