Test base smuggling and swapping

Author: Bromeon

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 {}
+        })
+    }
+}