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.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",

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