Clone objects when lowering them (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.

Author: bendk

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<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

@@ -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

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()

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())
+}

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
+object NoPointer

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 %}

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 %}

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,15 +150,15 @@ 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 %}
 
     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 %}
 

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

@@ -31,8 +31,15 @@ def self._uniffi_check(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 %}

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")