Merge pull request #846 from godot-rust/bugfix/object-arg-values

Fix user-after-free in `AsObjectArg` pass-by-value (in default-param methods)

Author: Bromeon

.github/workflows/full-ci.yml

@@ -402,7 +402,8 @@ jobs:
       - name: "Build and test hot-reload"
         if: ${{ matrix.with-hot-reload }}
         working-directory: examples/hot-reload/godot/test
-        run: ./run-test.sh
+        # Repeat a few times, our hot reload integration test can sometimes be a bit flaky.
+        run: $RETRY ./run-test.sh
         shell: bash
 
 

godot-codegen/src/generator/default_parameters.rs

@@ -49,10 +49,10 @@ pub fn make_function_definition_with_defaults(
     let receiver_param = &code.receiver.param;
     let receiver_self = &code.receiver.self_prefix;
 
-    let [required_params_impl_asarg, _, required_args_asarg] =
+    let [required_params_impl_asarg, _, _] =
         functions_common::make_params_exprs(required_fn_params.iter().cloned(), false, true, true);
 
-    let [required_params_plain, _, required_args_internal] =
+    let [_, _, required_args_internal] =
         functions_common::make_params_exprs(required_fn_params.into_iter(), false, false, false);
 
     let return_decl = &sig.return_value().decl;
@@ -78,7 +78,7 @@ pub fn make_function_definition_with_defaults(
         impl #builder_lifetime #builder_ty #builder_lifetime {
             fn new(
                 #object_param
-                #( #required_params_plain, )*
+                #( #required_params_impl_asarg, )*
             ) -> Self {
                 Self {
                     #( #builder_inits, )*
@@ -114,7 +114,7 @@ pub fn make_function_definition_with_defaults(
         ) -> #builder_ty #builder_anon_lifetime {
             #builder_ty::new(
                 #object_arg
-                #( #required_args_asarg, )*
+                #( #required_args_internal, )*
             )
         }
     };
@@ -247,26 +247,33 @@ fn make_extender(
             default_value,
         } = param;
 
-        let (field_type, needs_conversion) = type_.private_field_decl();
+        let (field_type, needs_conversion) = type_.default_extender_field_decl();
 
         // Initialize with default parameters where available, forward constructor args otherwise
         let init = if let Some(value) = default_value {
-            make_field_init(name, value, needs_conversion)
+            make_field_init(name, Some(value), needs_conversion)
         } else {
-            quote! { #name }
+            //quote! { #name }
+            make_field_init(name, None, needs_conversion)
+        };
+
+        let builder_arg = if needs_conversion {
+            quote! { self.#name.cow_as_object_arg() }
+        } else {
+            quote! { self.#name }
         };
 
         result.builder_fields.push(quote! { #name: #field_type });
-        result.builder_args.push(quote! { self.#name });
+        result.builder_args.push(builder_arg);
         result.builder_inits.push(init);
     }
 
     for param in default_fn_params {
         let FnParam { name, type_, .. } = param;
         let param_type = type_.param_decl();
-        let (_, field_needs_conversion) = type_.private_field_decl();
+        let (_, field_needs_conversion) = type_.default_extender_field_decl();
 
-        let field_init = make_field_init(name, &quote! { value }, field_needs_conversion);
+        let field_init = make_field_init(name, Some(&quote! { value }), field_needs_conversion);
 
         let method = quote! {
             #[inline]
@@ -285,10 +292,16 @@ fn make_extender(
     result
 }
 
-fn make_field_init(name: &Ident, expr: &TokenStream, needs_conversion: bool) -> TokenStream {
-    if needs_conversion {
-        quote! { #name: #expr.as_object_arg() }
-    } else {
-        quote! { #name: #expr }
+// Rewrite the above using match
+fn make_field_init(
+    name: &Ident,
+    expr: Option<&TokenStream>, // None if the parameter has the same name as the field, and can use shorthand syntax.
+    needs_conversion: bool,
+) -> TokenStream {
+    match (needs_conversion, expr) {
+        (true, Some(expr)) => quote! { #name: #expr.consume_object() },
+        (true, None) /*.             */ => quote! { #name: #name.consume_object() },
+        (false, Some(expr)) => quote! { #name: #expr },
+        (false, None) /*.             */ => quote! { #name },
     }
 }

godot-codegen/src/generator/functions_common.rs

@@ -303,6 +303,7 @@ pub fn make_vis(is_private: bool) -> TokenStream {
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Implementation
 
+// Method could possibly be split -- only one invocation uses all 3 return values, the rest uses only index [0] or [2].
 pub(crate) fn make_params_exprs<'a>(
     method_args: impl Iterator<Item = &'a FnParam>,
     is_virtual: bool,

godot-codegen/src/models/domain.rs

@@ -627,7 +627,8 @@ pub enum RustTy {
         impl_as_object_arg: TokenStream,
 
         /// only inner `T`
-        #[allow(dead_code)] // only read in minimal config
+        #[allow(dead_code)]
+        // only read in minimal config + RustTy::default_extender_field_decl()
         inner_class: Ident,
     },
 
@@ -645,10 +646,13 @@ impl RustTy {
         }
     }
 
-    /// Returns `( <field tokens>, <needs .as_object_arg()> )`.
-    pub fn private_field_decl(&self) -> (TokenStream, bool) {
+    /// Returns `( <field tokens>, <needs .consume_object()> )`.
+    pub fn default_extender_field_decl(&self) -> (TokenStream, bool) {
         match self {
-            RustTy::EngineClass { object_arg, .. } => (object_arg.clone(), true),
+            RustTy::EngineClass { inner_class, .. } => {
+                let cow_tokens = quote! { ObjectCow<crate::classes::#inner_class> };
+                (cow_tokens, true)
+            }
             other => (other.to_token_stream(), false),
         }
     }

godot-codegen/src/util.rs

@@ -28,7 +28,8 @@ pub fn make_imports() -> TokenStream {
         use crate::meta::{ClassName, PtrcallSignatureTuple, VarcallSignatureTuple};
         use crate::classes::native::*;
         use crate::classes::Object;
-        use crate::obj::{Gd, ObjectArg, AsObjectArg};
+        use crate::obj::{Gd, AsObjectArg};
+        use crate::obj::object_arg::{ObjectArg, ObjectCow};
         use crate::sys::GodotFfi as _;
     }
 }

godot-core/src/meta/sealed.rs

@@ -63,7 +63,7 @@ impl Sealed for Variant {}
 impl<T: ArrayElement> Sealed for Array<T> {}
 impl<T: GodotClass> Sealed for Gd<T> {}
 impl<T: GodotClass> Sealed for RawGd<T> {}
-impl<T: GodotClass> Sealed for ObjectArg<T> {}
+impl<T: GodotClass> Sealed for object_arg::ObjectArg<T> {}
 impl<T> Sealed for Option<T>
 where
     T: GodotType,

godot-core/src/obj/gd.rs

@@ -646,8 +646,8 @@ where
     ///
     /// let mut shape: Gd<Node> = some_node();
     /// shape.set_owner(Gd::null_arg());
-    pub fn null_arg() -> crate::obj::ObjectNullArg<T> {
-        crate::obj::ObjectNullArg(std::marker::PhantomData)
+    pub fn null_arg() -> crate::obj::object_arg::ObjectNullArg<T> {
+        crate::obj::object_arg::ObjectNullArg(std::marker::PhantomData)
     }
 }
 

godot-core/src/obj/mod.rs

@@ -15,18 +15,18 @@ mod base;
 mod gd;
 mod guards;
 mod instance_id;
-mod object_arg;
 mod onready;
 mod raw_gd;
 mod traits;
 
+pub(crate) mod object_arg;
 pub(crate) mod rtti;
 
 pub use base::*;
 pub use gd::*;
 pub use guards::{BaseMut, BaseRef, GdMut, GdRef};
 pub use instance_id::*;
-pub use object_arg::*;
+pub use object_arg::AsObjectArg;
 pub use onready::*;
 pub use raw_gd::*;
 pub use traits::*;

godot-core/src/obj/object_arg.rs

@@ -31,6 +31,10 @@ where
 {
     #[doc(hidden)]
     fn as_object_arg(&self) -> ObjectArg<T>;
+
+    /// Returns
+    #[doc(hidden)]
+    fn consume_object(self) -> ObjectCow<T>;
 }
 
 impl<T, U> AsObjectArg<T> for Gd<U>
@@ -41,6 +45,10 @@ where
     fn as_object_arg(&self) -> ObjectArg<T> {
         <&Gd<U>>::as_object_arg(&self)
     }
+
+    fn consume_object(self) -> ObjectCow<T> {
+        ObjectCow::Owned(self.upcast())
+    }
 }
 
 impl<T, U> AsObjectArg<T> for &Gd<U>
@@ -53,6 +61,10 @@ where
         // This function is not part of the public API.
         unsafe { ObjectArg::from_raw_gd(&self.raw) }
     }
+
+    fn consume_object(self) -> ObjectCow<T> {
+        ObjectCow::Borrowed(self.as_object_arg())
+    }
 }
 
 impl<T, U> AsObjectArg<T> for Option<U>
@@ -64,6 +76,13 @@ where
         self.as_ref()
             .map_or_else(ObjectArg::null, AsObjectArg::as_object_arg)
     }
+
+    fn consume_object(self) -> ObjectCow<T> {
+        match self {
+            Some(obj) => obj.consume_object(),
+            None => Gd::null_arg().consume_object(),
+        }
+    }
 }
 
 impl<T> AsObjectArg<T> for ObjectNullArg<T>
@@ -73,13 +92,43 @@ where
     fn as_object_arg(&self) -> ObjectArg<T> {
         ObjectArg::null()
     }
+
+    fn consume_object(self) -> ObjectCow<T> {
+        // Null pointer is safe to borrow.
+        ObjectCow::Borrowed(ObjectArg::null())
+    }
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 #[doc(hidden)]
 pub struct ObjectNullArg<T>(pub(crate) std::marker::PhantomData<*mut T>);
 
+/// Exists for cases where a value must be moved, and retaining `ObjectArg<T>` may not be possible if the source is owned.
+///
+/// Only used in default-param extender structs.
+#[doc(hidden)]
+pub enum ObjectCow<T: GodotClass> {
+    Owned(Gd<T>),
+    Borrowed(ObjectArg<T>),
+}
+
+impl<T> ObjectCow<T>
+where
+    T: GodotClass + Bounds<Declarer = bounds::DeclEngine>,
+{
+    /// Returns the actual `ObjectArg` to be passed to function calls.
+    ///
+    /// [`ObjectCow`] does not implement [`AsObjectArg<T>`] because a differently-named method is more explicit (less errors in codegen),
+    /// and because [`AsObjectArg::consume_object()`] is not meaningful.
+    pub fn cow_as_object_arg(&self) -> ObjectArg<T> {
+        match self {
+            ObjectCow::Owned(gd) => gd.as_object_arg(),
+            ObjectCow::Borrowed(obj) => obj.clone(),
+        }
+    }
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 /// View for object arguments passed to the Godot engine. Never owning; must be null or backed by `Gd<T>`.

godot-core/src/tools/save_load.rs

@@ -164,7 +164,7 @@ where
 {
     // TODO unclone GString
     let res = ResourceSaver::singleton()
-        .save_ex(obj.upcast())
+        .save_ex(obj)
         .path(path.clone())
         .done();
 

itest/rust/src/builtin_tests/containers/array_test.rs

@@ -394,7 +394,7 @@ fn untyped_array_return_from_godot_func() {
     let mut node = Node::new_alloc();
     let mut child = Node::new_alloc();
     child.set_name("child_node".into());
-    node.add_child(child.clone());
+    node.add_child(&child);
     node.queue_free(); // Do not leak even if the test fails.
     let result = node.get_node_and_resource("child_node".into());
 
@@ -431,7 +431,7 @@ fn typed_array_return_from_godot_func() {
     let mut node = Node::new_alloc();
     let mut child = Node::new_alloc();
     child.set_name("child_node".into());
-    node.add_child(child.clone());
+    node.add_child(&child);
     node.queue_free(); // Do not leak even if the test fails.
     let children = node.get_children();
 
@@ -477,10 +477,7 @@ fn array_should_format_with_display() {
 #[cfg(since_api = "4.2")]
 fn array_sort_custom() {
     let mut a = array![1, 2, 3, 4];
-    let func = Callable::from_fn("sort backwards", |args: &[&Variant]| {
-        let res = i32::from_variant(args[0]) > i32::from_variant(args[1]);
-        Ok(Variant::from(res))
-    });
+    let func = backwards_sort_callable();
     a.sort_unstable_custom(func);
     assert_eq!(a, array![4, 3, 2, 1]);
 }
@@ -489,14 +486,19 @@ fn array_sort_custom() {
 #[cfg(since_api = "4.2")]
 fn array_binary_search_custom() {
     let a = array![5, 4, 2, 1];
-    let func = Callable::from_fn("sort backwards", |args: &[&Variant]| {
-        let res = i32::from_variant(args[0]) > i32::from_variant(args[1]);
-        Ok(Variant::from(res))
-    });
+    let func = backwards_sort_callable();
     assert_eq!(a.bsearch_custom(&1, func.clone()), 3);
     assert_eq!(a.bsearch_custom(&3, func), 2);
 }
 
+#[cfg(since_api = "4.2")]
+fn backwards_sort_callable() -> Callable {
+    Callable::from_fn("sort backwards", |args: &[&Variant]| {
+        let res = args[0].to::<i32>() > args[1].to::<i32>();
+        Ok(res.to_variant())
+    })
+}
+
 #[itest]
 fn array_shrink() {
     let mut a = array![1, 5, 4, 3, 8];

itest/rust/src/engine_tests/node_test.rs

@@ -67,10 +67,10 @@ fn node_scene_tree() {
 
     let mut parent = Node::new_alloc();
     parent.set_name("parent".into());
-    parent.add_child(child.clone());
+    parent.add_child(&child);
 
     let mut scene = PackedScene::new_gd();
-    let err = scene.pack(parent.clone());
+    let err = scene.pack(&parent);
     assert_eq!(err, global::Error::OK);
 
     let mut tree = SceneTree::new_alloc();

itest/rust/src/object_tests/object_arg_test.rs

@@ -6,9 +6,9 @@
  */
 
 use godot::builtin::Variant;
-use godot::classes::{ClassDb, Node};
+use godot::classes::{ClassDb, Node, ResourceFormatLoader, ResourceLoader};
 use godot::global;
-use godot::obj::{Gd, NewAlloc};
+use godot::obj::{Gd, NewAlloc, NewGd};
 
 use crate::framework::itest;
 use crate::object_tests::object_test::{user_refc_instance, RefcPayload};
@@ -78,6 +78,24 @@ fn object_arg_null_arg() {
     assert_eq!(error, global::Error::ERR_UNAVAILABLE);
 }
 
+// Regression test for https://github.com/godot-rust/gdext/issues/835.
+#[itest]
+fn object_arg_owned_default_params() {
+    // Calls the _ex() variant behind the scenes.
+    let a = ResourceFormatLoader::new_gd();
+    let b = ResourceFormatLoader::new_gd();
+
+    // Use direct and explicit _ex() call syntax.
+    ResourceLoader::singleton().add_resource_format_loader(a.clone()); // by value
+    ResourceLoader::singleton()
+        .add_resource_format_loader_ex(b.clone()) // by value
+        .done();
+
+    // Clean up (no leaks).
+    ResourceLoader::singleton().remove_resource_format_loader(a);
+    ResourceLoader::singleton().remove_resource_format_loader(b);
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Helpers