Detect dead objects before RawGd construction (avoids UB)

Author: Bromeon

godot-core/src/builtin/variant/mod.rs

@@ -8,10 +8,8 @@
 use crate::builtin::{
     GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
 };
-use crate::classes;
 use crate::meta::error::ConvertError;
 use crate::meta::{arg_into_ref, ArrayElement, AsArg, FromGodot, ToGodot};
-use crate::obj::Gd;
 use godot_ffi as sys;
 use std::{fmt, ptr};
 use sys::{ffi_methods, interface_fn, GodotFfi};
@@ -232,20 +230,17 @@ impl Variant {
         unsafe { interface_fn!(variant_booleanize)(self.var_sys()) != 0 }
     }
 
-    /* If we need to detect dead objects in the future, this is an alternative to try_to().
+    /// Assuming that this is of type `OBJECT`, checks whether the object is dead.
+    ///
+    /// Does not check again that the variant has type `OBJECT`.
+    pub(crate) fn is_object_dead(&self) -> bool {
+        debug_assert_eq!(self.get_type(), VariantType::OBJECT);
 
-    /// # Panics
-    /// In Debug mode, if this variant holds an object that has been freed.
-    fn ensure_alive_if_object(&self) {
-        // There is no dedicated API, however to_string() returns a magic variant for dead objects.
-        // This may of course fail if a string repr matches this exactly, but it's very unlikely.
-        debug_assert_ne!(
-            self.to_string(),
-            "<Freed Object>",
-            "Variant holds pointer to a dead object; all operations on it are invalid"
-        )
+        !crate::gen::utilities::is_instance_valid(self)
+
+        // In case there are ever problems with this approach, alternative implementation:
+        // self.stringify() == "<Freed Object>".into()
     }
-    */
 
     // Conversions from/to Godot C++ `Variant*` pointers
     ffi_methods! {

godot-core/src/obj/raw_gd.rs

@@ -53,12 +53,10 @@ impl<T: GodotClass> RawGd<T> {
         } else {
             let raw_id = unsafe { interface_fn!(object_get_instance_id)(obj) };
 
-            // During Variant -> RawGd conversion, it can happen that the variant contains a dead object.
-            // It's not quite clear how Godot detects instance_id == 0 here though, since the Variant holds the object as bytes in an array,
-            // and there is no lookup performed.
-            let Some(instance_id) = InstanceId::try_from_u64(raw_id) else {
-                return Self::null();
-            };
+            // This happened originally during Variant -> RawGd conversion, but at this point it's too late to detect, and UB has already
+            // occurred (the Variant holds the object pointer as bytes in an array, which becomes dangling the moment the actual object dies).
+            let instance_id = InstanceId::try_from_u64(raw_id)
+                .expect("null instance ID when constructing object; this very likely causes UB");
 
             // TODO(bromeon): this should query dynamic type of object, which can be different from T (upcast, FromGodot, etc).
             // See comment in ObjectRtti.
@@ -593,6 +591,12 @@ impl<T: GodotClass> GodotFfiVariant for RawGd<T> {
             .into_error(variant.clone()));
         }
 
+        // Check for dead objects *before* converting. Godot doesn't care if the objects are still alive, and hitting
+        // RawGd::from_obj_sys_weak() is too late and causes UB.
+        if variant.is_object_dead() {
+            return Err(FromVariantError::DeadObject.into_error(variant.clone()));
+        }
+
         let raw = unsafe {
             // Uses RawGd<Object> and not Self, because Godot still allows illegal conversions. We thus check with manual casting later on.
             // See https://github.com/godot-rust/gdext/issues/158.
@@ -603,12 +607,6 @@ impl<T: GodotClass> GodotFfiVariant for RawGd<T> {
             })
         };
 
-        // Explicitly handle case where object is dead. See RawGd::from_obj_sys_weak() for some edge cases and considerations.
-        if raw.is_null() {
-            // Passing `raw` is not useful, it would just print "null" for the value.
-            return Err(FromVariantError::DeadObject.into_error(variant.clone()));
-        }
-
         raw.with_inc_refcount().owned_cast().map_err(|raw| {
             FromVariantError::WrongClass {
                 expected: T::class_name(),