Skip to content

Commit 3158825

Browse files
committed
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.
1 parent 7aed5da commit 3158825

File tree

2 files changed

+19
-2
lines changed

2 files changed

+19
-2
lines changed

godot-core/src/obj/gd.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,9 @@ impl<T: GodotClass> Gd<T> {
488488
/// Runs `init_fn` on the address of a pointer (initialized to null). If that pointer is still null after the `init_fn` call,
489489
/// then `None` will be returned; otherwise `Gd::from_obj_sys(ptr)`.
490490
///
491+
/// This method will **NOT** increment the reference-count of the object, as it assumes the input to come from a Godot API
492+
/// return value.
493+
///
491494
/// # Safety
492495
/// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_.
493496
#[doc(hidden)]
@@ -496,7 +499,9 @@ impl<T: GodotClass> Gd<T> {
496499

497500
// Initialize pointer with given function, return Some(ptr) on success and None otherwise
498501
let object_ptr = raw_object_init(init_fn);
499-
sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys(ptr))
502+
503+
// Do not increment ref-count; assumed to be return value from FFI.
504+
sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys_weak(ptr))
500505
}
501506
}
502507

itest/rust/src/object_test.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use crate::{expect_panic, itest};
88
use godot::bind::{godot_api, GodotClass, GodotExt};
99
use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector3};
10-
use godot::engine::{Node, Node3D, Object, RefCounted};
10+
use godot::engine::{file_access, FileAccess, Node, Node3D, Object, RefCounted};
1111
use godot::obj::Share;
1212
use godot::obj::{Base, Gd, InstanceId};
1313
use godot::sys::GodotFfi;
@@ -38,6 +38,7 @@ pub fn run() -> bool {
3838
ok &= object_engine_convert_variant();
3939
ok &= object_user_convert_variant_refcount();
4040
ok &= object_engine_convert_variant_refcount();
41+
ok &= object_engine_returned_refcount();
4142
ok &= object_engine_up_deref();
4243
ok &= object_engine_up_deref_mut();
4344
ok &= object_engine_upcast();
@@ -265,6 +266,17 @@ fn check_convert_variant_refcount(obj: Gd<RefCounted>) {
265266
assert_eq!(obj.get_reference_count(), 1);
266267
}
267268

269+
#[itest]
270+
fn object_engine_returned_refcount() {
271+
let Some(file) = FileAccess::open("res://itest.gdextension".into(), file_access::ModeFlags::READ) else {
272+
panic!("failed to open file used to test FileAccess")
273+
};
274+
assert!(file.is_open());
275+
276+
// There was a bug which incremented ref-counts of just-returned objects, keep this as regression test.
277+
assert_eq!(file.get_reference_count(), 1);
278+
}
279+
268280
#[itest]
269281
fn object_engine_up_deref() {
270282
let node3d: Gd<Node3D> = Node3D::new_alloc();

0 commit comments

Comments
 (0)