Merge pull request #546 from godot-rust/bugfix/subtype-ub

Harden safety around dead and badly typed `Gd<T>` instances

Author: Bromeon

godot-codegen/src/class_generator.rs

@@ -600,6 +600,15 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
     // notify() and notify_reversed() are added after other methods, to list others first in docs.
     let notify_methods = make_notify_methods(class_name, ctx);
 
+    let internal_methods = quote! {
+        fn __checked_id(&self) -> Option<crate::obj::InstanceId> {
+            // SAFETY: only Option due to layout-compatibility with RawGd<T>; it is always Some because stored in Gd<T> which is non-null.
+            let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() };
+            let instance_id = rtti.check_type::<Self>();
+            Some(instance_id)
+        }
+    };
+
     let memory = if class_name.rust_ty == "Object" {
         ident("DynamicRefCount")
     } else if class.is_refcounted {
@@ -608,7 +617,7 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
         ident("ManualMemory")
     };
 
-    // mod re_export needed, because class should not appear inside the file module, and we can't re-export private struct as pub
+    // mod re_export needed, because class should not appear inside the file module, and we can't re-export private struct as pub.
     let imports = util::make_imports();
     let tokens = quote! {
         #![doc = #module_doc]
@@ -625,14 +634,18 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
             #[repr(C)]
             pub struct #class_name {
                 object_ptr: sys::GDExtensionObjectPtr,
-                instance_id: crate::obj::InstanceId,
+
+                // This field should never be None. Type Option<T> is chosen to be layout-compatible with Gd<T>, which uses RawGd<T> inside.
+                // The RawGd<T>'s identity field can be None because of generality (it can represent null pointers, as opposed to Gd<T>).
+                rtti: Option<crate::private::ObjectRtti>,
             }
             #virtual_trait
             #notification_enum
             impl #class_name {
                 #constructor
                 #methods
                 #notify_methods
+                #internal_methods
                 #constants
             }
             unsafe impl crate::obj::GodotClass for #class_name {
@@ -646,12 +659,12 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
                 }
             }
             impl crate::obj::EngineClass for #class_name {
-                 fn as_object_ptr(&self) -> sys::GDExtensionObjectPtr {
-                     self.object_ptr
-                 }
-                 fn as_type_ptr(&self) -> sys::GDExtensionTypePtr {
-                    std::ptr::addr_of!(self.object_ptr) as sys::GDExtensionTypePtr
-                 }
+                fn as_object_ptr(&self) -> sys::GDExtensionObjectPtr {
+                    self.object_ptr
+                }
+                fn as_type_ptr(&self) -> sys::GDExtensionTypePtr {
+                   std::ptr::addr_of!(self.object_ptr) as sys::GDExtensionTypePtr
+                }
             }
             #(
                 impl crate::obj::Inherits<crate::engine::#all_bases> for #class_name {}
@@ -1180,7 +1193,7 @@ fn make_class_method_definition(
     let maybe_instance_id = if method.is_static {
         quote! { None }
     } else {
-        quote! { Some(self.instance_id) }
+        quote! { self.__checked_id() }
     };
 
     let fptr_access = if cfg!(feature = "codegen-lazy-fptrs") {

godot-core/src/builtin/meta/class_name.rs

@@ -61,18 +61,18 @@ impl ClassName {
         self.c_str.to_str().unwrap()
     }
 
-    /// Converts the class name to a GString.
-    pub fn to_godot_string(&self) -> GString {
+    /// Converts the class name to a `GString`.
+    pub fn to_gstring(&self) -> GString {
         self.with_string_name(|s| s.into())
     }
 
-    /// Converts the class name to a StringName.
+    /// Converts the class name to a `StringName`.
     pub fn to_string_name(&self) -> StringName {
         self.with_string_name(|s| s.clone())
     }
 
     /// The returned pointer is valid indefinitely, as entries are never deleted from the cache.
-    /// Since we use Box<StringName>, HashMap reallocations don't affect the validity of the StringName.
+    /// Since we use `Box<StringName>`, `HashMap` reallocations don't affect the validity of the StringName.
     #[doc(hidden)]
     pub fn string_sys(&self) -> sys::GDExtensionStringNamePtr {
         self.with_string_name(|s| s.string_sys())

godot-core/src/builtin/meta/signature.rs

@@ -167,7 +167,8 @@ macro_rules! impl_varcall_signature_for_tuple {
                     unsafe { varcall_arg::<$Pn, $n>(args_ptr, method_name) },
                 )*) ;
 
-                varcall_return::<$R>(func(instance_ptr, args), ret, err)
+                let rust_result = func(instance_ptr, args);
+                varcall_return::<$R>(rust_result, ret, err)
             }
 
             #[inline]
@@ -181,9 +182,9 @@ macro_rules! impl_varcall_signature_for_tuple {
             ) -> Self::Ret {
                 //$crate::out!("out_class_varcall: {method_name}");
 
-                // Note: varcalls are not safe from failing, if the happen through an object pointer -> validity check necessary.
+                // Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary.
                 if let Some(instance_id) = maybe_instance_id {
-                    crate::engine::ensure_object_alive(Some(instance_id), object_ptr, method_name);
+                    crate::engine::ensure_object_alive(instance_id, object_ptr, method_name);
                 }
 
                 let class_fn = sys::interface_fn!(object_method_bind_call);
@@ -298,7 +299,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
             ) -> Self::Ret {
                 // $crate::out!("out_class_ptrcall: {method_name}");
                 if let Some(instance_id) = maybe_instance_id {
-                    crate::engine::ensure_object_alive(Some(instance_id), object_ptr, method_name);
+                    crate::engine::ensure_object_alive(instance_id, object_ptr, method_name);
                 }
 
                 let class_fn = sys::interface_fn!(object_method_bind_ptrcall);

godot-core/src/engine/mod.rs

@@ -24,6 +24,9 @@ mod script_instance;
 pub use gfile::{GFile, NotUniqueError};
 pub use script_instance::{create_script_instance, ScriptInstance};
 
+#[cfg(debug_assertions)]
+use crate::builtin::meta::ClassName;
+
 /// Support for Godot _native structures_.
 ///
 /// Native structures are a niche API in Godot. These are low-level data types that are passed as pointers to/from the engine.
@@ -218,15 +221,14 @@ pub(crate) fn object_ptr_from_id(instance_id: InstanceId) -> sys::GDExtensionObj
     unsafe { sys::interface_fn!(object_get_instance_from_id)(instance_id.to_u64()) }
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Implementation of this file
+
 pub(crate) fn ensure_object_alive(
-    instance_id: Option<InstanceId>,
+    instance_id: InstanceId,
     old_object_ptr: sys::GDExtensionObjectPtr,
     method_name: &'static str,
 ) {
-    let Some(instance_id) = instance_id else {
-        panic!("{method_name}: cannot call method on null object")
-    };
-
     let new_object_ptr = object_ptr_from_id(instance_id);
 
     assert!(
@@ -242,8 +244,26 @@ pub(crate) fn ensure_object_alive(
     );
 }
 
-// ----------------------------------------------------------------------------------------------------------------------------------------------
-// Implementation of this file
+#[cfg(debug_assertions)]
+pub(crate) fn ensure_object_inherits(
+    derived: &ClassName,
+    base: &ClassName,
+    instance_id: InstanceId,
+) -> bool {
+    // TODO static cache.
+
+    if derived == base
+        || base == &Object::class_name() // always true
+        || ClassDb::singleton().is_parent_class(derived.to_string_name(), base.to_string_name())
+    {
+        return true;
+    }
+
+    panic!(
+        "Instance of ID {instance_id} has type {derived} but is incorrectly stored in a Gd<{base}>.\n\
+        This may happen if you change an object's identity through DerefMut."
+    )
+}
 
 // Separate function, to avoid constructing string twice
 // Note that more optimizations than that likely make no sense, as loading is quite expensive
@@ -253,7 +273,7 @@ where
 {
     ResourceLoader::singleton()
         .load_ex(path.clone())
-        .type_hint(T::class_name().to_godot_string())
+        .type_hint(T::class_name().to_gstring())
         .done() // TODO unclone
         .and_then(|res| res.try_cast::<T>().ok())
 }

godot-core/src/lib.rs

@@ -65,6 +65,8 @@ pub mod private {
         sys::plugin_foreach!(__GODOT_PLUGIN_REGISTRY; visitor);
     }
 
+    pub use crate::obj::rtti::ObjectRtti;
+
     pub struct ClassConfig {
         pub is_tool: bool,
     }

godot-core/src/obj/gd.rs

@@ -263,8 +263,7 @@ impl<T: GodotClass> Gd<T> {
     ///
     /// 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
+        // 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() }
     }
@@ -278,8 +277,7 @@ impl<T: GodotClass> Gd<T> {
     /// and will panic in a defined manner. Encountering such panics is almost always a bug you should fix, and not a
     /// runtime condition to check against.
     pub fn is_instance_valid(&self) -> bool {
-        // This call refreshes the instance ID, and recognizes dead objects.
-        self.instance_id_or_none().is_some()
+        self.raw.is_instance_valid()
     }
 
     /// **Upcast:** convert into a smart pointer to a base class. Always succeeds.
@@ -443,7 +441,7 @@ impl<T: GodotClass> Gd<T> {
 impl<T, M> Gd<T>
 where
     T: GodotClass<Mem = M>,
-    M: mem::PossiblyManual + mem::Memory,
+    M: mem::Memory + mem::PossiblyManual,
 {
     /// Destroy the manually-managed Godot object.
     ///
@@ -463,38 +461,61 @@ where
         // 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.
 
+        // free() is likely to be invoked in destructors during panic unwind. In this case, we cannot panic again.
+        // Instead, we print an error and exit free() immediately. The closure is supposed to be used in a unit return statement.
+        let is_panic_unwind = std::thread::panicking();
+        let error_or_panic = |msg: String| {
+            if is_panic_unwind {
+                crate::godot_error!(
+                    "Encountered 2nd panic in free() during panic unwind; will skip destruction:\n{msg}"
+                );
+            } else {
+                panic!("{}", msg);
+            }
+        };
+
         // 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);
-        assert_ne!(
-            ref_counted, Some(true),
-            "called free() on Gd<Object> which points to a RefCounted dynamic type; free() only supported for manually managed types\n\
-            object: {self:?}"
-        );
+        if ref_counted == Some(true) {
+            return error_or_panic(format!(
+                "Called free() on Gd<Object> which points to a RefCounted dynamic type; free() only supported for manually managed types\n\
+                Object: {self:?}"
+            ));
+        }
 
         // If ref_counted returned None, that means the instance was destroyed
-        assert!(
-            ref_counted == Some(false) && self.is_instance_valid(),
-            "called free() on already destroyed object"
-        );
+        if ref_counted != Some(false) || !self.is_instance_valid() {
+            return error_or_panic("called free() on already destroyed object".to_string());
+        }
+
+        // If the object is still alive, make sure the dynamic type matches. Necessary because subsequent checks may rely on the
+        // static type information to be correct. This is a no-op in Release mode.
+        // Skip check during panic unwind; would need to rewrite whole thing to use Result instead. Having BOTH panic-in-panic and bad type is
+        // a very unlikely corner case.
+        if !is_panic_unwind {
+            self.raw.check_dynamic_type("free");
+        }
 
         // 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"
-        );
+        if bound {
+            return error_or_panic(
+                "called free() while a bind() or bind_mut() call is active".to_string(),
+            );
+        }
 
         // 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());
         }
 
+        // TODO: this might leak associated data in Gd<T>, e.g. ClassName.
         std::mem::forget(self);
     }
 }
@@ -508,10 +529,12 @@ impl<T: GodotClass> GodotConvert for Gd<T> {
 
 impl<T: GodotClass> ToGodot for Gd<T> {
     fn to_godot(&self) -> Self::Via {
+        self.raw.check_rtti("Gd<T>::to_godot");
         self.clone()
     }
 
     fn into_godot(self) -> Self::Via {
+        self.raw.check_rtti("Gd<T>::into_godot");
         self
     }
 }
@@ -622,7 +645,7 @@ impl<T: GodotClass> Export for Gd<T> {
 
         // Godot does this by default too; the hint is needed when the class is a resource/node,
         // but doesn't seem to make a difference otherwise.
-        let hint_string = T::class_name().to_godot_string();
+        let hint_string = T::class_name().to_gstring();
 
         PropertyHintInfo { hint, hint_string }
     }

godot-core/src/obj/mod.rs

@@ -19,6 +19,8 @@ mod onready;
 mod raw;
 mod traits;
 
+pub(crate) mod rtti;
+
 pub use base::*;
 pub use gd::*;
 pub use guards::*;
@@ -27,4 +29,6 @@ pub use onready::*;
 pub use raw::*;
 pub use traits::*;
 
+// Do not re-export rtti here.
+
 type GdDerefTarget<T> = <<T as GodotClass>::Declarer as dom::Domain>::DerefTarget<T>;