Merge pull request #744 from godot-rust/bugfix/base-access

Test `Base<T>` destruction and swapping

Author: Bromeon

godot-core/src/obj/base.rs

@@ -14,6 +14,9 @@ use std::mem::ManuallyDrop;
 ///
 /// Behaves similarly to [`Gd`][crate::obj::Gd], but is more constrained. Cannot be constructed by the user.
 pub struct Base<T: GodotClass> {
+    // Like `Gd`, it's theoretically possible that Base is destroyed while there are still other Gd pointers to the underlying object. This is
+    // safe, may however lead to unintended behavior. The base_test.rs file checks some of these scenarios.
+
     // Internal smart pointer is never dropped. It thus acts like a weak pointer and is needed to break reference cycles between Gd<T>
     // and the user instance owned by InstanceStorage.
     //
@@ -34,19 +37,37 @@ pub struct Base<T: GodotClass> {
 }
 
 impl<T: GodotClass> Base<T> {
+    /// "Copy constructor": allows to share a `Base<T>` weak pointer.
+    ///
+    /// The return value is a weak pointer, so it will not keep the instance alive.
+    ///
     /// # Safety
-    /// The returned Base is a weak pointer, so holding it will not keep the object alive. It must not be accessed after the object is destroyed.
+    /// `base` must be alive at the time of invocation, i.e. user `init()` (which could technically destroy it) must not have run yet.
+    /// If `base` is destroyed while the returned `Base<T>` is in use, that constitutes a logic error, not a safety issue.
     pub(crate) unsafe fn from_base(base: &Base<T>) -> Base<T> {
-        Base::from_obj(Gd::from_obj_sys_weak(base.as_gd().obj_sys()))
+        debug_assert!(base.obj.is_instance_valid());
+        Base::from_obj(Gd::from_obj_sys_weak(base.obj.obj_sys()))
     }
 
+    /// Create base from existing object (used in script instances).
+    ///
+    /// The return value is a weak pointer, so it will not keep the instance alive.
+    ///
     /// # Safety
-    /// The returned Base is a weak pointer, so holding it will not keep the object alive. It must not be accessed after the object is destroyed.
+    /// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base<T>` is in use, that constitutes a logic
+    /// error, not a safety issue.
     pub(crate) unsafe fn from_gd(gd: &Gd<T>) -> Self {
+        debug_assert!(gd.is_instance_valid());
         Base::from_obj(Gd::from_obj_sys_weak(gd.obj_sys()))
     }
 
-    // Note: not &mut self, to only borrow one field and not the entire struct
+    /// Create new base from raw Godot object.
+    ///
+    /// The return value is a weak pointer, so it will not keep the instance alive.
+    ///
+    /// # Safety
+    /// `base_ptr` must point to a valid, live object at the time of invocation. If it is destroyed while the returned `Base<T>` is in use,
+    /// that constitutes a logic error, not a safety issue.
     pub(crate) unsafe fn from_sys(base_ptr: sys::GDExtensionObjectPtr) -> Self {
         assert!(!base_ptr.is_null(), "instance base is null pointer");
 
@@ -76,16 +97,7 @@ impl<T: GodotClass> Base<T> {
         (*self.obj).clone()
     }
 
-    /// Returns a [`Gd`] referencing the same object as this reference.
-    ///
-    /// Using this method to call methods on the base field of a Rust object is discouraged, instead use the
-    /// methods from [`WithBaseField`](super::WithBaseField) when possible.
-    #[doc(hidden)]
-    pub fn as_gd(&self) -> &Gd<T> {
-        &self.obj
-    }
-
-    // Currently only used in outbound virtual calls (for scripts).
+    // Currently only used in outbound virtual calls (for scripts); search for: base_field(self).obj_sys().
     #[doc(hidden)]
     pub fn obj_sys(&self) -> sys::GDExtensionObjectPtr {
         self.obj.obj_sys()

godot-core/src/obj/gd.rs

@@ -241,9 +241,11 @@ impl<T: GodotClass> Gd<T> {
     ///
     /// This method is safe and never panics.
     pub fn instance_id_unchecked(&self) -> InstanceId {
+        let instance_id = self.raw.instance_id_unchecked();
+
         // SAFETY: a `Gd` can only be created from a non-null `RawGd`, meaning `raw.instance_id_unchecked()` will
         // always return `Some`.
-        unsafe { self.raw.instance_id_unchecked().unwrap_unchecked() }
+        unsafe { instance_id.unwrap_unchecked() }
     }
 
     /// Checks if this smart pointer points to a live object (read description!).

itest/rust/src/object_tests/base_test.rs

@@ -5,7 +5,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::framework::itest;
+use crate::framework::{expect_panic, itest};
 use godot::prelude::*;
 
 #[itest(skip)]
@@ -105,6 +105,86 @@ fn base_gd_self() {
     obj.free();
 }
 
+// Hardening against https://github.com/godot-rust/gdext/issues/711.
+#[itest]
+fn base_smuggling() {
+    let (mut obj, extracted_base) = create_object_with_extracted_base();
+
+    // This works because Gd<T> additionally stores the instance ID (through cached_rtti).
+    assert_eq!(extracted_base.to_gd().instance_id(), obj.instance_id());
+
+    // This _also_ works because Gd<T> has the direct object pointer to the Godot object.
+    obj.set_position(Vector2::new(1.0, 2.0));
+    assert_eq!(
+        extracted_base.to_gd().get_position(),
+        Vector2::new(1.0, 2.0)
+    );
+
+    // Destroy base externally.
+    extracted_base.to_gd().free();
+
+    // Access to object should now fail.
+    expect_panic("object with dead base: calling base methods", || {
+        obj.get_position();
+    });
+    expect_panic("object with dead base: bind()", || {
+        obj.bind();
+    });
+    expect_panic("object with dead base: instance_id()", || {
+        obj.instance_id();
+    });
+    expect_panic("object with dead base: clone()", || {
+        let _ = obj.clone();
+    });
+    expect_panic("object with dead base: upcast()", || {
+        obj.upcast::<Object>();
+    });
+
+    // Now vice versa: destroy object, access base.
+    let (obj, extracted_base) = create_object_with_extracted_base();
+    obj.free();
+
+    expect_panic("accessing extracted base of dead object", || {
+        extracted_base.to_gd().get_position();
+    });
+}
+
+// While base swapping isn't an encouraged workflow, it can also be regarded as a quicker way to swap all individual properties of two base
+// objects -- which is also allowed. It's also similar to slicing in C++. So this is a Ship-of-Theseus problem, and we don't install ergonomic
+// obstacles to prevent it. Here, we test that results are expected and safe.
+#[itest]
+fn base_swapping() {
+    let (one, mut one_ext_base) = create_object_with_extracted_base();
+    let one_id = one.instance_id();
+
+    let mut two = Based::new_alloc();
+    let two_id = two.instance_id();
+
+    std::mem::swap(&mut one_ext_base, &mut two.bind_mut().base);
+
+    // Gd<T> itself isn't affected (it stores the ID separately).
+    assert_eq!(one_id, one.instance_id());
+    assert_eq!(two_id, two.instance_id());
+
+    // However, the base now has the other object's ID. Gd<T> and T.base having distinct IDs is a bit unintuitive and could lead to follow-up
+    // logic errors. One option to prevent this would be to add a base integrity check on the entire Gd<T> API (it can't be done from the
+    // Base<T> side, since that only has direct access to the object pointer, while Gd<T> has access to the object pointer _and_ the base field).
+    // Not sure if this is worth the effort + complexity though, given that it almost requires malice to get into such a situation.
+    assert_eq!(one.instance_id(), two.bind().base().instance_id());
+    assert_eq!(two.instance_id(), one_ext_base.to_gd().instance_id());
+
+    one.free();
+    two.free();
+}
+
+fn create_object_with_extracted_base() -> (Gd<Baseless>, Base<Node2D>) {
+    let mut extracted_base = None;
+    let obj = Baseless::smuggle_out(&mut extracted_base);
+    let extracted_base = extracted_base.expect("smuggling didn't work");
+
+    (obj, extracted_base)
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 use renamed_bases::Based;
@@ -139,3 +219,13 @@ impl Based {
 struct Baseless {
     // No need for fields, we just test if we can access this as Gd<Node2D>.
 }
+
+impl Baseless {
+    /// Steals the `Base<T>` from a newly constructed object and stores it in the output parameter.
+    fn smuggle_out(other_base: &mut Option<Base<Node2D>>) -> Gd<Self> {
+        Gd::from_init_fn(|base| {
+            *other_base = Some(base);
+            Self {}
+        })
+    }
+}