Merge #133

133: Fix use-after-free and 3 memory leaks; enforce AddressSanitizer in CI r=Bromeon a=Bromeon

Changes implementation of the `Gd` smart pointer to cache the instance ID in each object. To my knowledge, instance IDs are the only reliable way to check for object validity in Godot, as raw object pointers may become dangling.

In addition, this PR fixes 3 memory leaks around arrays and dictionaries. Those occurred due to `from_sys_init()` using `T::default()` in too many places. This essentially lead to allocating a default-constructed object, which is then immediately overwritten by the `init` function, which _also_ allocates a new object. I refactored the `GodotFfi::from_sys_init()` to **never** call `default()`, and add an explicit `from_sys_init_default()` for cases where this is desired. This also cuts down on some of the boilerplate.

---

Both use-after-free and memory leaks were discovered using AddressSanitizer/LeakSanitizer. Great tooling from the C++ world, which also proves useful for us, as miri can't be used in FFI contexts. From now on, UB or leaks detected by ASan/LSan will cause a hard error in CI. The tools are not 100% bullet-proof; they didn't detect the following UAF case in a short test, but they are still of great value as a more systematic counter to memory errors.

<details><summary>use-after-free, false negative</summary>

```rs
let mut boks = Box::new(44);
let ptr = std::ptr::addr_of_mut!(*boks);
println!("deref={}", unsafe { *ptr }); // output: deref=44
drop(boks);
println!("deref={}", unsafe { *ptr }); // output: deref=1719666059
```

</details>

On a side note, getting these working correctly in CI was a bit of a marathon because ASan/LSan don't have stacktraces for dynamically loaded libraries (a known wontfix problem, see google/sanitizers#89). Additionally, false positives for memory leaks were reported: a simple `println!` would cause 1024 bytes of non-reclaimable memory. Therefore, I had to compile a special version of nightly Godot that disables dynamic library unloading via `dlclose`, to keep the stacktrace around, and this seemed to fix the false-positive issue as well. Although likely unrelated, what I also found during research was rust-lang/rust#19776.

Fixes #89.

Co-authored-by: Jan Haller <[email protected]>

Author: bors[bot]

.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)