Skip to content

Commit

Permalink
Fix memory leak for RefCounted objects returned by engine methods
Browse files Browse the repository at this point in the history
The refcount was accidentally incremented on the Rust call-site, even though Godot already does that.
Added regression test for this case.
  • Loading branch information
Bromeon committed Feb 5, 2023
1 parent 7aed5da commit 3158825
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
7 changes: 6 additions & 1 deletion godot-core/src/obj/gd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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))
}
}

Expand Down
14 changes: 13 additions & 1 deletion itest/rust/src/object_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 3158825

Please sign in to comment.