Bugfix: prevent Gd destruction while user object is bound

Previously, free() and engine-induced destruction of manually managed objects did not check whether
a bind() or bind_mut() call was ongoing, which allowed to pull the rug under the guard, leaving
user references dangling.

Author: Bromeon

godot-core/src/obj/gd.rs

@@ -206,10 +206,12 @@ impl<T: GodotClass> Gd<T> {
         })
     }
 
-    /// ⚠️ Returns the last known, possibly invalid instance ID of this object.
+    /// Returns the last known, possibly invalid instance ID of this object.
     ///
     /// This function does not check that the returned instance ID points to a valid instance!
     /// Unless performance is a problem, use [`instance_id()`][Self::instance_id] instead.
+    ///
+    /// This method is safe and never panics.
     pub fn instance_id_unchecked(&self) -> InstanceId {
         // SAFETY:
         // A `Gd` can only be created from a non-null `RawGd`. Meaning `raw.instance_id_unchecked()` will
@@ -390,10 +392,15 @@ where
     /// example to the node tree in case of nodes.
     ///
     /// # Panics
-    /// * When the referred-to object has already been destroyed.
-    /// * When this is invoked on an upcast `Gd<Object>` that dynamically points to a reference-counted type (i.e. operation not supported).
+    /// - When the referred-to object has already been destroyed.
+    /// - When this is invoked on an upcast `Gd<Object>` that dynamically points to a reference-counted type (i.e. operation not supported).
+    /// - When the object is bound by an ongoing `bind()` or `bind_mut()` call (through a separate `Gd` pointer).
     pub fn free(self) {
+        // Note: this method is NOT invoked when the free() call happens dynamically (e.g. through GDScript or reflection).
+        // As such, do not use it for operations and validations to perform upon destruction.
+
         // TODO disallow for singletons, either only at runtime or both at compile time (new memory policy) and runtime
+        use dom::Domain;
 
         // Runtime check in case of T=Object, no-op otherwise
         let ref_counted = T::Mem::is_ref_counted(&self.raw);
@@ -408,7 +415,17 @@ where
             "called free() on already destroyed object"
         );
 
-        // This destroys the Storage instance, no need to run destructor again
+        // SAFETY: object must be alive, which was just checked above. No multithreading here.
+        // Also checked in the C free_instance_func callback, however error message can be more precise here and we don't need to instruct
+        // the engine about object destruction. Both paths are tested.
+        let bound = unsafe { T::Declarer::is_currently_bound(&self.raw) };
+        assert!(
+            !bound,
+            "called free() while a bind() or bind_mut() call is active"
+        );
+
+        // SAFETY: object alive as checked.
+        // This destroys the Storage instance, no need to run destructor again.
         unsafe {
             sys::interface_fn!(object_destroy)(self.raw.obj_sys());
         }

godot-core/src/obj/traits.rs

@@ -275,6 +275,15 @@ pub mod dom {
         where
             T: GodotClass<Declarer = Self>,
             F: FnOnce(&mut T) -> R;
+
+        /// Check if the object is a user object *and* currently locked by a `bind()` or `bind_mut()` guard.
+        ///
+        /// # Safety
+        /// Object must be alive.
+        #[doc(hidden)]
+        unsafe fn is_currently_bound<T>(obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>;
     }
 
     /// Expresses that a class is declared by the Godot engine.
@@ -293,6 +302,13 @@ pub mod dom {
                     .expect("scoped mut should not be called on a null object"),
             )
         }
+
+        unsafe fn is_currently_bound<T>(_obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>,
+        {
+            false
+        }
     }
 
     /// Expresses that a class is declared by the user.
@@ -309,6 +325,13 @@ pub mod dom {
             let mut guard = obj.bind_mut();
             closure(&mut *guard)
         }
+
+        unsafe fn is_currently_bound<T>(obj: &RawGd<T>) -> bool
+        where
+            T: GodotClass<Declarer = Self>,
+        {
+            obj.storage().unwrap_unchecked().is_bound()
+        }
     }
 }
 

godot-core/src/storage.rs

@@ -40,7 +40,7 @@ mod single_threaded {
         base: Base<T::Base>,
 
         // Declared after `user_instance`, is dropped last
-        pub lifecycle: cell::Cell<Lifecycle>,
+        pub(super) lifecycle: cell::Cell<Lifecycle>,
         godot_ref_count: cell::Cell<u32>,
     }
 
@@ -81,6 +81,11 @@ mod single_threaded {
             );
         }
 
+        pub fn is_bound(&self) -> bool {
+            // Needs to borrow mutably, otherwise it succeeds if shared borrows are alive.
+            self.user_instance.try_borrow_mut().is_err()
+        }
+
         pub fn get(&self) -> cell::Ref<T> {
             self.user_instance.try_borrow().unwrap_or_else(|_e| {
                 panic!(
@@ -122,7 +127,7 @@ mod multi_threaded {
     use std::sync;
     use std::sync::atomic::{AtomicU32, Ordering};
 
-    use crate::obj::{Base, Gd, GodotClass, Inherits, Share};
+    use crate::obj::{Base, Gd, GodotClass, Inherits};
     use crate::out;
 
     use super::Lifecycle;
@@ -158,7 +163,7 @@ mod multi_threaded {
         base: Base<T::Base>,
 
         // Declared after `user_instance`, is dropped last
-        pub lifecycle: AtomicLifecycle,
+        pub(super) lifecycle: AtomicLifecycle,
         godot_ref_count: AtomicU32,
     }
 
@@ -195,6 +200,11 @@ mod multi_threaded {
             );
         }
 
+        pub fn is_bound(&self) -> bool {
+            // Needs to borrow mutably, otherwise it succeeds if shared borrows are alive.
+            self.user_instance.try_write().is_err()
+        }
+
         pub fn get(&self) -> sync::RwLockReadGuard<T> {
             self.user_instance.read().unwrap_or_else(|_e| {
                 panic!(
@@ -311,7 +321,14 @@ pub unsafe fn as_storage<'u, T: GodotClass>(
 /// # Safety
 /// `instance_ptr` is assumed to point to a valid instance. This function must only be invoked once for a pointer.
 pub unsafe fn destroy_storage<T: GodotClass>(instance_ptr: sys::GDExtensionClassInstancePtr) {
-    let _drop = Box::from_raw(instance_ptr as *mut InstanceStorage<T>);
+    let raw = instance_ptr as *mut InstanceStorage<T>;
+
+    assert!(
+        !(*raw).is_bound(),
+        "tried to destroy object while a bind() or bind_mut() call is active"
+    );
+
+    let _drop = Box::from_raw(raw);
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

itest/rust/src/object_tests/object_test.rs

@@ -224,6 +224,20 @@ fn object_new_has_instance_id() {
     obj.free();
 }
 
+#[itest]
+fn object_dynamic_free() {
+    let mut obj = Gd::<SignalEmitter>::new_default();
+    let id = obj.instance_id();
+
+    obj.call("free".into(), &[]);
+
+    assert_eq!(
+        Gd::<SignalEmitter>::try_from_instance_id(id),
+        None,
+        "dynamic free() call must destroy object"
+    );
+}
+
 #[itest]
 fn object_user_bind_after_free() {
     let obj = Gd::new(SignalEmitter {}); // type doesn't matter, just Object-derived
@@ -235,6 +249,53 @@ fn object_user_bind_after_free() {
     });
 }
 
+#[itest]
+fn object_user_free_during_bind() {
+    let obj = Gd::new(SignalEmitter {}); // type doesn't matter, just Object-derived
+    let guard = obj.bind();
+
+    let copy = obj.clone(); // TODO clone allowed while bound?
+
+    expect_panic("direct free() on user while it's bound", move || {
+        copy.free();
+    });
+
+    drop(guard);
+    assert!(
+        obj.is_instance_valid(),
+        "object lives on after failed free()"
+    );
+    obj.free(); // now succeeds
+}
+
+#[itest]
+fn object_user_dynamic_free_during_bind() {
+    // Note: we could also test if GDScript can access free() when an object is bound, to check whether the panic is handled or crashes
+    // the engine. However, that is only possible under the following scenarios:
+    // 1. Multithreading -- needs to be outlawed on Gd<T> in general, anyway. If we allow a thread-safe Gd<T>, we however need to handle that.
+    // 2. Re-entrant calls -- Rust binds a Gd<T>, calls GDScript, which frees the same Gd. This is the same as the test here.
+    // 3. Holding a guard (GdRef/GdMut) across function calls -- not possible, guard's lifetime is coupled to a Gd and cannot be stored in
+    //    fields or global variables due to that.
+
+    let obj = Gd::new(SignalEmitter {}); // type doesn't matter, just Object-derived
+    let guard = obj.bind();
+
+    let mut copy = obj.clone(); // TODO clone allowed while bound?
+
+    expect_panic("engine-routed free() on user while it's bound", move || {
+        copy.call("free".into(), &[]);
+    });
+
+    drop(guard);
+    assert!(
+        obj.is_instance_valid(),
+        "object lives on after failed dynamic free()"
+    );
+    obj.free(); // now succeeds
+}
+
+// TODO test if engine destroys it, eg. call()
+
 #[itest]
 fn object_user_call_after_free() {
     let obj = Gd::new(SignalEmitter {}); // type doesn't matter, just Object-derived
@@ -617,7 +678,6 @@ fn object_user_bad_downcast() {
 #[itest]
 fn object_engine_manual_free() {
     // Tests if no panic or memory leak
-
     {
         let node = Node3D::new_alloc();
         let node2 = node.clone();
@@ -637,10 +697,11 @@ fn object_engine_shared_free() {
 
 #[itest]
 fn object_engine_manual_double_free() {
-    expect_panic("double free()", || {
-        let node = Node3D::new_alloc();
-        let node2 = node.clone();
-        node.free();
+    let node = Node3D::new_alloc();
+    let node2 = node.clone();
+    node.free();
+
+    expect_panic("double free()", move || {
         node2.free();
     });
 }
@@ -653,6 +714,17 @@ fn object_engine_refcounted_free() {
     expect_panic("calling free() on RefCounted object", || node2.free())
 }
 
+#[itest]
+fn object_user_double_free() {
+    let mut obj = Gd::<SignalEmitter>::new_default();
+    let obj2 = obj.clone();
+    obj.call("free".into(), &[]);
+
+    expect_panic("double free()", move || {
+        obj2.free();
+    });
+}
+
 #[itest]
 fn object_user_share_drop() {
     let drop_count = Rc::new(RefCell::new(0));
@@ -865,11 +937,10 @@ impl SignalEmitter {
     fn do_use();
 }
 
-/// Test that godot can call a method that takes `&self`, while there already exists an immutable reference
+/// Test that Godot can call a method that takes `&self`, while there already exists an immutable reference
 /// to that type acquired through `bind`.
 ///
-/// This test is not signal-specific, the original bug would happen whenever godot would call a method that
-/// takes `&self`. However this was the easiest way to test the bug i could find.
+/// This test is not signal-specific, the original bug would happen whenever Godot would call a method that takes `&self`.
 #[itest]
 fn double_use_reference() {
     let double_use: Gd<DoubleUse> = Gd::new_default();
@@ -891,7 +962,7 @@ fn double_use_reference() {
 
     assert!(guard.used.get(), "use_1 was not called");
 
-    std::mem::drop(guard);
+    drop(guard);
 
     double_use.free();
     emitter.free();