Merge pull request #1033 from godot-rust/bugfix/variant-dead-objects

Fix `Variant` -> `Gd` conversions not taking into account dead objects

godot-codegen/src/generator/central_files.rs

@@ -71,17 +71,21 @@ pub fn make_core_central_code(api: &ExtensionApi, ctx: &mut Context) -> TokenStr
         #variant_type_traits
 
         #[allow(dead_code)]
+        // Exhaustive, because only new Godot minor versions add new variants, which need either godot-rust minor bump or `api-*` feature.
         pub enum VariantDispatch {
             Nil,
             #(
                 #variant_ty_enumerators_pascal(#variant_ty_enumerators_rust),
             )*
+            /// Special case of a `Variant` holding an object that has been destroyed.
+            FreedObject,
         }
 
         impl VariantDispatch {
             pub fn from_variant(variant: &Variant) -> Self {
                 match variant.get_type() {
                     VariantType::NIL => Self::Nil,
+                    VariantType::OBJECT if !variant.is_object_alive() => Self::FreedObject,
                     #(
                         VariantType::#variant_ty_enumerators_shout
                             => Self::#variant_ty_enumerators_pascal(variant.to::<#variant_ty_enumerators_rust>()),
@@ -100,6 +104,7 @@ pub fn make_core_central_code(api: &ExtensionApi, ctx: &mut Context) -> TokenStr
                     #(
                         Self::#variant_ty_enumerators_pascal(v) => write!(f, "{v:?}"),
                     )*
+                    Self::FreedObject => write!(f, "<Freed Object>"),
                 }
             }
         }

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

@@ -5,12 +5,11 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use crate::arg_into_ref;
 use crate::builtin::{
     GString, StringName, VariantArray, VariantDispatch, VariantOperator, VariantType,
 };
 use crate::meta::error::ConvertError;
-use crate::meta::{ArrayElement, AsArg, FromGodot, ToGodot};
+use crate::meta::{arg_into_ref, ArrayElement, AsArg, FromGodot, ToGodot};
 use godot_ffi as sys;
 use std::{fmt, ptr};
 use sys::{ffi_methods, interface_fn, GodotFfi};
@@ -231,6 +230,18 @@ impl Variant {
         unsafe { interface_fn!(variant_booleanize)(self.var_sys()) != 0 }
     }
 
+    /// 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_alive(&self) -> bool {
+        debug_assert_eq!(self.get_type(), VariantType::OBJECT);
+
+        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! {
         type sys::GDExtensionVariantPtr = *mut Self;
@@ -468,16 +479,18 @@ impl fmt::Display for Variant {
 
 impl fmt::Debug for Variant {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        // Special case for arrays: avoids converting to VariantArray (the only Array type in VariantDispatch), which fails
-        // for typed arrays and causes a panic. This can cause an infinite loop with Debug, or abort.
-        // Can be removed if there's ever a "possibly typed" Array type (e.g. OutArray) in the library.
-
-        if self.get_type() == VariantType::ARRAY {
-            // SAFETY: type is checked, and only operation is print (out data flow, no covariant in access).
-            let array = unsafe { VariantArray::from_variant_unchecked(self) };
-            array.fmt(f)
-        } else {
-            VariantDispatch::from_variant(self).fmt(f)
+        match self.get_type() {
+            // Special case for arrays: avoids converting to VariantArray (the only Array type in VariantDispatch),
+            // which fails for typed arrays and causes a panic. This can cause an infinite loop with Debug, or abort.
+            // Can be removed if there's ever a "possibly typed" Array type (e.g. OutArray) in the library.
+            VariantType::ARRAY => {
+                // SAFETY: type is checked, and only operation is print (out data flow, no covariant in access).
+                let array = unsafe { VariantArray::from_variant_unchecked(self) };
+                array.fmt(f)
+            }
+
+            // VariantDispatch also includes dead objects via `FreedObject` enumerator, which maps to "<Freed Object>".
+            _ => VariantDispatch::from_variant(self).fmt(f),
         }
     }
 }

godot-core/src/meta/error/convert_error.rs

@@ -5,11 +5,10 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
+use godot_ffi::VariantType;
 use std::error::Error;
 use std::fmt;
 
-use godot_ffi::VariantType;
-
 use crate::builtin::Variant;
 use crate::meta::{ArrayTypeInfo, ClassName, ToGodot};
 
@@ -35,6 +34,11 @@ impl ConvertError {
         }
     }
 
+    // /// Create a new custom error for a conversion with the value that failed to convert.
+    // pub(crate) fn with_kind(kind: ErrorKind) -> Self {
+    //     Self { kind, value: None }
+    // }
+
     /// Create a new custom error for a conversion with the value that failed to convert.
     pub(crate) fn with_kind_value<V>(kind: ErrorKind, value: V) -> Self
     where
@@ -323,6 +327,9 @@ pub(crate) enum FromVariantError {
     WrongClass {
         expected: ClassName,
     },
+
+    /// Variant holds an object which is no longer alive.
+    DeadObject,
 }
 
 impl FromVariantError {
@@ -345,6 +352,7 @@ impl fmt::Display for FromVariantError {
             Self::WrongClass { expected } => {
                 write!(f, "cannot convert to class {expected}")
             }
+            Self::DeadObject => write!(f, "variant holds object which is no longer alive"),
         }
     }
 }

godot-core/src/meta/godot_convert/mod.rs

@@ -103,8 +103,7 @@ pub trait FromGodot: Sized + GodotConvert {
     /// If the conversion fails.
     fn from_variant(variant: &Variant) -> Self {
         Self::try_from_variant(variant).unwrap_or_else(|err| {
-            eprintln!("FromGodot::from_variant() failed: {err}");
-            panic!()
+            panic!("FromGodot::from_variant() failed -- {err}");
         })
     }
 }

godot-core/src/obj/raw_gd.rs

@@ -10,7 +10,7 @@ use std::{fmt, ptr};
 use godot_ffi as sys;
 use sys::{interface_fn, GodotFfi, GodotNullableFfi, PtrcallType};
 
-use crate::builtin::Variant;
+use crate::builtin::{Variant, VariantType};
 use crate::meta::error::{ConvertError, FromVariantError};
 use crate::meta::{
     CallContext, ClassName, FromGodot, GodotConvert, GodotFfiVariant, GodotType, RefArg, ToGodot,
@@ -42,6 +42,8 @@ impl<T: GodotClass> RawGd<T> {
     /// Initializes this `RawGd<T>` from the object pointer as a **weak ref**, meaning it does not
     /// initialize/increment the reference counter.
     ///
+    /// If `obj` is null or the instance ID query behind the object returns 0, the returned `RawGd<T>` will have the null state.
+    ///
     /// # Safety
     ///
     /// `obj` must be a valid object pointer or a null pointer.
@@ -51,8 +53,10 @@ impl<T: GodotClass> RawGd<T> {
         } else {
             let raw_id = unsafe { interface_fn!(object_get_instance_id)(obj) };
 
+            // 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("constructed RawGd weak pointer with instance ID 0");
+                .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.
@@ -576,13 +580,27 @@ impl<T: GodotClass> GodotFfiVariant for RawGd<T> {
     }
 
     fn ffi_from_variant(variant: &Variant) -> Result<Self, ConvertError> {
-        let raw = unsafe {
-            // TODO(#234) replace Gd::<Object> with Self when Godot stops allowing illegal conversions
-            // See https://github.com/godot-rust/gdext/issues/158
+        let variant_type = variant.get_type();
 
-            // TODO(uninit) - see if we can use from_sys_init()
+        // Explicit type check before calling `object_from_variant`, to allow for better error messages.
+        if variant_type != VariantType::OBJECT {
+            return Err(FromVariantError::BadType {
+                expected: VariantType::OBJECT,
+                actual: variant_type,
+            }
+            .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_alive() {
+            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.
 
-            // raw_object_init?
             RawGd::<classes::Object>::new_with_uninit(|self_ptr| {
                 let converter = sys::builtin_fn!(object_from_variant);
                 converter(self_ptr, sys::SysPtr::force_mut(variant.var_sys()));
@@ -673,6 +691,9 @@ impl<T: GodotClass> fmt::Debug for RawGd<T> {
 
 /// Runs `init_fn` on the address of a pointer (initialized to null), then returns that pointer, possibly still null.
 ///
+/// This relies on the fact that an object pointer takes up the same space as the FFI representation of an object (`OpaqueObject`).
+/// The pointer is thus used as an opaque handle, initialized by `init_fn`, so that it represents a valid Godot object afterwards.
+///
 /// # Safety
 /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_.
 #[doc(hidden)]

itest/rust/src/builtin_tests/containers/variant_test.rs

@@ -130,6 +130,44 @@ fn variant_bad_conversions() {
         .expect_err("`nil` should not convert to `Dictionary`");
 }
 
+#[itest]
+fn variant_dead_object_conversions() {
+    let obj = Node::new_alloc();
+    let variant = obj.to_variant();
+
+    let result = variant.try_to::<Gd<Node>>();
+    let gd = result.expect("Variant::to() with live object should succeed");
+    assert_eq!(gd, obj);
+
+    obj.free();
+
+    // Verify Display + Debug impl.
+    assert_eq!(format!("{variant}"), "<Freed Object>");
+    assert_eq!(format!("{variant:?}"), "<Freed Object>");
+
+    // Variant::try_to().
+    let result = variant.try_to::<Gd<Node>>();
+    let err = result.expect_err("Variant::try_to::<Gd>() with dead object should fail");
+    assert_eq!(
+        err.to_string(),
+        "variant holds object which is no longer alive: <Freed Object>"
+    );
+
+    // Variant::to().
+    expect_panic("Variant::to() with dead object should panic", || {
+        let _: Gd<Node> = variant.to();
+    });
+
+    // Variant::try_to() -> Option<Gd>.
+    // This conversion does *not* return `None` for dead objects, but an error. `None` is reserved for NIL variants, see object_test.rs.
+    let result = variant.try_to::<Option<Gd<Node>>>();
+    let err = result.expect_err("Variant::try_to::<Option<Gd>>() with dead object should fail");
+    assert_eq!(
+        err.to_string(),
+        "variant holds object which is no longer alive: <Freed Object>"
+    );
+}
+
 #[itest]
 fn variant_bad_conversion_error_message() {
     let variant = 123.to_variant();
@@ -139,11 +177,10 @@ fn variant_bad_conversion_error_message() {
         .expect_err("i32 -> GString conversion should fail");
     assert_eq!(err.to_string(), "cannot convert from INT to STRING: 123");
 
-    // TODO this error isn't great, but unclear whether it can be improved. If not, document.
     let err = variant
         .try_to::<Gd<Node>>()
         .expect_err("i32 -> Gd<Node> conversion should fail");
-    assert_eq!(err.to_string(), "`Gd` cannot be null: null");
+    assert_eq!(err.to_string(), "cannot convert from INT to OBJECT: 123");
 }
 
 #[itest]

itest/rust/src/object_tests/object_test.rs

@@ -535,6 +535,27 @@ fn object_engine_convert_variant_error() {
     );
 }
 
+#[itest]
+fn object_convert_variant_option() {
+    let refc = RefCounted::new_gd();
+    let variant = refc.to_variant();
+
+    // Variant -> Option<Gd>.
+    let gd = Option::<Gd<RefCounted>>::from_variant(&variant);
+    assert_eq!(gd, Some(refc.clone()));
+
+    let nil = Variant::nil();
+    let gd = Option::<Gd<RefCounted>>::from_variant(&nil);
+    assert_eq!(gd, None);
+
+    // Option<Gd> -> Variant.
+    let back = Some(refc).to_variant();
+    assert_eq!(back, variant);
+
+    let back = None::<Gd<RefCounted>>.to_variant();
+    assert_eq!(back, Variant::nil());
+}
+
 #[itest]
 fn object_engine_returned_refcount() {
     let Some(file) = FileAccess::open("res://itest.gdextension", file_access::ModeFlags::READ)