Fix use-after-free and 3 memory leaks; enforce AddressSanitizer in CI

.github/workflows/full-ci.yml

@@ -138,8 +138,7 @@ jobs:
   godot-itest:
     name: godot-itest (${{ matrix.name }})
     runs-on: ${{ matrix.os }}
-    # TODO: continue-on-error: false, as soon as memory errors are fixed
-    continue-on-error: ${{ contains(matrix.name, 'memcheck') }}
+    continue-on-error: false
     timeout-minutes: 24
     strategy:
       fail-fast: false # cancel all jobs as soon as one fails?
@@ -165,6 +164,10 @@ jobs:
             rust-toolchain: stable
             godot-binary: godot.linuxbsd.editor.dev.x86_64
 
+          # Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks.
+          # Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and
+          # cause false positives like println!. See https://github.com/google/sanitizers/issues/89.
+          # The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one.
           - name: linux-memcheck-gcc
             os: ubuntu-20.04
             rust-toolchain: stable

godot-codegen/src/class_generator.rs

@@ -686,7 +686,7 @@ impl VariantFfi {
     fn type_ptr() -> Self {
         Self {
             sys_method: ident("sys_const"),
-            from_sys_init_method: ident("from_sys_init"),
+            from_sys_init_method: ident("from_sys_init_default"),
         }
     }
 }
@@ -876,7 +876,7 @@ fn make_return(
         }
         (None, Some(return_ty)) => {
             quote! {
-                <#return_ty as sys::GodotFfi>::from_sys_init(|return_ptr| {
+                <#return_ty as sys::GodotFfi>::from_sys_init_default(|return_ptr| {
                     #ptrcall_invocation
                 })
             }

godot-core/src/builtin/array.rs

@@ -70,12 +70,11 @@ impl_builtin_froms!(Array;
 
 impl<T: VariantMetadata> TypedArray<T> {
     fn from_opaque(opaque: sys::types::OpaqueArray) -> Self {
-        let array = Self {
+        // Note: type is not yet checked at this point, because array has not yet been initialized!
+        Self {
             opaque,
             _phantom: PhantomData,
-        };
-        array.check_type();
-        array
+        }
     }
 
     /// Returns the number of elements in the array. Equivalent of `size()` in Godot.
@@ -320,8 +319,9 @@ impl<T: VariantMetadata> TypedArray<T> {
     /// # Panics
     ///
     /// If the inner type doesn't match `T` or no type is set at all.
-    fn check_type(&self) {
+    fn with_checked_type(self) -> Self {
         assert_eq!(self.type_info(), TypeInfo::new::<T>());
+        self
     }
 
     /// Sets the type of the inner array. Can only be called once, directly after creation.
@@ -567,17 +567,9 @@ impl<T: VariantMetadata + ToVariant> TypedArray<T> {
 // }
 
 impl<T: VariantMetadata> GodotFfi for TypedArray<T> {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- Array CoW implementation in C++ expects that on
-        // assignment, the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
@@ -598,28 +590,25 @@ impl<T: VariantMetadata> fmt::Debug for TypedArray<T> {
 /// [`Array::duplicate_deep()`].
 impl<T: VariantMetadata> Share for TypedArray<T> {
     fn share(&self) -> Self {
-        unsafe {
+        let array = unsafe {
             Self::from_sys_init(|self_ptr| {
                 let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
                 let args = [self.sys_const()];
                 ctor(self_ptr, args.as_ptr());
             })
-        }
+        };
+        array.with_checked_type()
     }
 }
 
 impl<T: VariantMetadata> Default for TypedArray<T> {
     #[inline]
     fn default() -> Self {
-        // Note: can't use from_sys_init(), as that calls the default constructor
-        // (because most assignments expect initialized target type)
-        let mut uninit = std::mem::MaybeUninit::<TypedArray<T>>::uninit();
         let mut array = unsafe {
-            let self_ptr = (*uninit.as_mut_ptr()).sys_mut();
-            sys::builtin_call! {
-                array_construct_default(self_ptr, std::ptr::null_mut())
-            };
-            uninit.assume_init()
+            Self::from_sys_init(|self_ptr| {
+                let ctor = sys::builtin_fn!(array_construct_default);
+                ctor(self_ptr, std::ptr::null_mut())
+            })
         };
         array.init_inner_type();
         array
@@ -661,14 +650,14 @@ impl<T: VariantMetadata> FromVariant for TypedArray<T> {
         if variant.get_type() != Self::variant_type() {
             return Err(VariantConversionError);
         }
-        let result = unsafe {
-            Self::from_sys_init(|self_ptr| {
+        let array = unsafe {
+            Self::from_sys_init_default(|self_ptr| {
                 let array_from_variant = sys::builtin_fn!(array_from_variant);
                 array_from_variant(self_ptr, variant.var_sys());
             })
         };
 
-        Ok(result)
+        Ok(array.with_checked_type())
     }
 }
 
@@ -773,7 +762,7 @@ impl<T: VariantMetadata> PartialEq for TypedArray<T> {
             let mut result = false;
             sys::builtin_call! {
                 array_operator_equal(self.sys(), other.sys(), result.sys_mut())
-            };
+            }
             result
         }
     }

godot-core/src/builtin/dictionary.rs

@@ -240,17 +240,9 @@ impl Dictionary {
 // Traits
 
 impl GodotFfi for Dictionary {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- Dictionary CoW implementation in C++ expects that on
-        // assignment, the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result

godot-core/src/builtin/macros.rs

@@ -33,7 +33,7 @@ macro_rules! impl_builtin_traits_inner {
             #[inline]
             fn clone(&self) -> Self {
                 unsafe {
-                    Self::from_sys_init(|self_ptr| {
+                    Self::from_sys_init_default(|self_ptr| {
                         let ctor = ::godot_ffi::builtin_fn!($gd_method);
                         let args = [self.sys_const()];
                         ctor(self_ptr, args.as_ptr());
@@ -116,7 +116,7 @@ macro_rules! impl_builtin_traits_inner {
                     return Err($crate::builtin::variant::VariantConversionError)
                 }
                 let result = unsafe {
-                    Self::from_sys_init(|self_ptr| {
+                    Self::from_sys_init_default(|self_ptr| {
                         let converter = sys::builtin_fn!($gd_method);
                         converter(self_ptr, variant.var_sys());
                     })

godot-core/src/builtin/node_path.rs

@@ -6,7 +6,7 @@
 
 use crate::builtin::GodotString;
 use godot_ffi as sys;
-use godot_ffi::{ffi_methods, GodotFfi};
+use godot_ffi::{ffi_methods, GDExtensionTypePtr, GodotFfi};
 use std::fmt::{Display, Formatter, Result as FmtResult};
 
 pub struct NodePath {
@@ -21,12 +21,18 @@ impl NodePath {
 
 impl GodotFfi for NodePath {
     ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
+
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(GDExtensionTypePtr)) -> Self {
+        let mut result = Self::default();
+        init_fn(result.sys_mut());
+        result
+    }
 }
 
 impl From<&GodotString> for NodePath {
     fn from(path: &GodotString) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(node_path_from_string);
                 let args = [path.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -38,7 +44,7 @@ impl From<&GodotString> for NodePath {
 impl From<&NodePath> for GodotString {
     fn from(path: &NodePath) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_from_node_path);
                 let args = [path.sys_const()];
                 ctor(self_ptr, args.as_ptr());

godot-core/src/builtin/others.rs

@@ -50,7 +50,7 @@ impl Callable {
         // upcast not needed
         let method = method.into();
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(callable_from_object_method);
                 let args = [object.sys_const(), method.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -66,6 +66,7 @@ impl Callable {
 
 impl_builtin_traits! {
     for Callable {
+        // Default => callable_construct_default;
         FromVariant => callable_from_variant;
     }
 }

godot-core/src/builtin/packed_array.rs

@@ -386,17 +386,9 @@ macro_rules! impl_packed_array {
         }
 
         impl GodotFfi for $PackedArray {
-            ffi_methods! {
-                type sys::GDExtensionTypePtr = *mut Opaque;
-                fn from_sys;
-                fn sys;
-                fn write_sys;
-            }
-
-            unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-                // Can't use uninitialized pointer -- Vector CoW implementation in C++ expects that
-                // on assignment, the target CoW pointer is either initialized or nullptr
+            ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+            unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
                 let mut result = Self::default();
                 init_fn(result.sys_mut());
                 result

godot-core/src/builtin/string.rs

@@ -37,43 +37,18 @@ impl GodotString {
 }
 
 impl GodotFfi for GodotString {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- String CoW implementation in C++ expects that on assignment,
-        // the target CoW pointer is either initialized or nullptr
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
     }
 }
 
-impl Default for GodotString {
-    fn default() -> Self {
-        // Note: can't use from_sys_init(), as that calls the default constructor
-        // (because most assignments expect initialized target type)
-
-        let mut uninit = std::mem::MaybeUninit::<GodotString>::uninit();
-
-        unsafe {
-            let self_ptr = (*uninit.as_mut_ptr()).sys_mut();
-            sys::builtin_call! {
-                string_construct_default(self_ptr, std::ptr::null_mut())
-            };
-
-            uninit.assume_init()
-        }
-    }
-}
-
 impl_builtin_traits! {
     for GodotString {
+        Default => string_construct_default;
         Clone => string_construct_copy;
         Drop => string_destroy;
         Eq => string_operator_equal;

godot-core/src/builtin/string_name.rs

@@ -33,17 +33,9 @@ impl StringName {
 }
 
 impl GodotFfi for StringName {
-    ffi_methods! {
-        type sys::GDExtensionTypePtr = *mut Opaque;
-        fn from_sys;
-        fn sys;
-        fn write_sys;
-    }
-
-    unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
-        // Can't use uninitialized pointer -- StringName implementation in C++ expects that on assignment,
-        // the target type is a valid string (possibly empty)
+    ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
 
+    unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
         let mut result = Self::default();
         init_fn(result.sys_mut());
         result
@@ -106,7 +98,7 @@ impl Hash for StringName {
 impl From<&GodotString> for StringName {
     fn from(s: &GodotString) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_name_from_string);
                 let args = [s.sys_const()];
                 ctor(self_ptr, args.as_ptr());
@@ -128,7 +120,7 @@ where
 impl From<&StringName> for GodotString {
     fn from(s: &StringName) -> Self {
         unsafe {
-            Self::from_sys_init(|self_ptr| {
+            Self::from_sys_init_default(|self_ptr| {
                 let ctor = sys::builtin_fn!(string_from_string_name);
                 let args = [s.sys_const()];
                 ctor(self_ptr, args.as_ptr());

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

@@ -55,19 +55,21 @@ macro_rules! impl_variant_traits {
 
         impl FromVariant for $T {
             fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
+                // Type check -- at the moment, a strict match is required.
+                if variant.get_type() != Self::variant_type() {
+                    return Err(VariantConversionError)
+                }
+
                 // In contrast to T -> Variant, the conversion Variant -> T assumes
                 // that the destination is initialized (at least for some T). For example:
                 // void String::operator=(const String &p_str) { _cowdata._ref(p_str._cowdata); }
                 // does a copy-on-write and explodes if this->_cowdata is not initialized.
                 // We can thus NOT use Self::from_sys_init().
-                if variant.get_type() != Self::variant_type() {
-                    return Err(VariantConversionError)
-                }
-                let mut value = <$T>::default();
                 let result = unsafe {
-                    let converter = sys::builtin_fn!($to_fn);
-                    converter(value.sys_mut(), variant.var_sys());
-                    value
+                    Self::from_sys_init_default(|self_ptr| {
+                        let converter = sys::builtin_fn!($to_fn);
+                        converter(self_ptr, variant.var_sys());
+                    })
                 };
 
                 Ok(result)