From 31588250b1db48db99ee1e40707a924d5d667519 Mon Sep 17 00:00:00 2001
From: Jan Haller <bromeon@gmail.com>
Date: Sun, 5 Feb 2023 15:51:09 +0100
Subject: [PATCH] Fix memory leak for RefCounted objects returned by engine
 methods

The refcount was accidentally incremented on the Rust call-site, even though Godot already does that.
Added regression test for this case.
---
 godot-core/src/obj/gd.rs      |  7 ++++++-
 itest/rust/src/object_test.rs | 14 +++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs
index 70312cbe5..235f1ee4d 100644
--- a/godot-core/src/obj/gd.rs
+++ b/godot-core/src/obj/gd.rs
@@ -488,6 +488,9 @@ impl<T: GodotClass> Gd<T> {
     /// Runs `init_fn` on the address of a pointer (initialized to null). If that pointer is still null after the `init_fn` call,
     /// then `None` will be returned; otherwise `Gd::from_obj_sys(ptr)`.
     ///
+    /// This method will **NOT** increment the reference-count of the object, as it assumes the input to come from a Godot API
+    /// return value.
+    ///
     /// # Safety
     /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_.
     #[doc(hidden)]
@@ -496,7 +499,9 @@ impl<T: GodotClass> Gd<T> {
 
         // Initialize pointer with given function, return Some(ptr) on success and None otherwise
         let object_ptr = raw_object_init(init_fn);
-        sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys(ptr))
+
+        // Do not increment ref-count; assumed to be return value from FFI.
+        sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys_weak(ptr))
     }
 }
 
diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs
index 6dadd0c9e..d5f148d3a 100644
--- a/itest/rust/src/object_test.rs
+++ b/itest/rust/src/object_test.rs
@@ -7,7 +7,7 @@
 use crate::{expect_panic, itest};
 use godot::bind::{godot_api, GodotClass, GodotExt};
 use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector3};
-use godot::engine::{Node, Node3D, Object, RefCounted};
+use godot::engine::{file_access, FileAccess, Node, Node3D, Object, RefCounted};
 use godot::obj::Share;
 use godot::obj::{Base, Gd, InstanceId};
 use godot::sys::GodotFfi;
@@ -38,6 +38,7 @@ pub fn run() -> bool {
     ok &= object_engine_convert_variant();
     ok &= object_user_convert_variant_refcount();
     ok &= object_engine_convert_variant_refcount();
+    ok &= object_engine_returned_refcount();
     ok &= object_engine_up_deref();
     ok &= object_engine_up_deref_mut();
     ok &= object_engine_upcast();
@@ -265,6 +266,17 @@ fn check_convert_variant_refcount(obj: Gd<RefCounted>) {
     assert_eq!(obj.get_reference_count(), 1);
 }
 
+#[itest]
+fn object_engine_returned_refcount() {
+    let Some(file) = FileAccess::open("res://itest.gdextension".into(), file_access::ModeFlags::READ) else {
+        panic!("failed to open file used to test FileAccess")
+    };
+    assert!(file.is_open());
+
+    // There was a bug which incremented ref-counts of just-returned objects, keep this as regression test.
+    assert_eq!(file.get_reference_count(), 1);
+}
+
 #[itest]
 fn object_engine_up_deref() {
     let node3d: Gd<Node3D> = Node3D::new_alloc();