Merge #325

325: Address sanitizers for Rust r=Bromeon a=Bromeon We have missed a lot of UB that occurred only in Rust code and wasn't detected by AddressSanitizer outside of C++ code. This PR enables ASan for Rust code using `-Zsanitizer=address` in the two memcheck CI jobs. As such, this is a follow-up to #133. The CI currently fails because there is still some UB on `master`... ideally we can fix this soon. bors try Co-authored-by: Jan Haller <bromeon@gmail.com>
godot-rust · Jul 6, 2023 · 4ebd736 · 4ebd736
2 parents 565bf10 + a3012f4
commit 4ebd736
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 33 deletions.
diff --git a/.github/composite/godot-itest/action.yml b/.github/composite/godot-itest/action.yml
@@ -45,6 +45,11 @@ inputs:
     default: ''
     description: "Extra values for the RUSTFLAGS env var"
 
+  rust-target:
+    required: false
+    default: ''
+    description: "If provided, acts as an argument for '--target', and results in output files written to ./target/{target}"
+
   with-llvm:
     required: false
     default: ''
@@ -118,11 +123,23 @@ runs:
       shell: bash
 
     - name: "Build gdext (itest)"
-      run: |
-        cargo build -p itest ${{ inputs.rust-extra-args }}
-      shell: bash
       env:
         RUSTFLAGS: ${{ inputs.rust-env-rustflags }}
+        TARGET: ${{ inputs.rust-target }}
+      run: |
+        targetArgs=""
+        if [[ -n "$TARGET" ]]; then
+          targetArgs="--target $TARGET"
+        fi
+        
+        cargo build -p itest ${{ inputs.rust-extra-args }} $targetArgs
+        
+        # Instead of modifying .gdextension, rename the output directory
+        if [[ -n "$TARGET" ]]; then
+          rm -rf target/debug
+          mv target/$TARGET/debug target
+        fi
+      shell: bash
 
     # This step no longer fails if there's a diff, as we expect header to be forward-compatible; instead issues a warning
     # However, still fails if patch cannot be applied (conflict).
@@ -162,8 +179,10 @@ runs:
       run: |
         cd itest/godot
         echo "OUTCOME=itest" >> $GITHUB_ENV
-        $GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 | tee "${{ runner.temp }}/log.txt" | tee >(grep "SCRIPT ERROR:" -q && {
-        	printf "\n -- Godot engine encountered error, abort...\n";
+        $GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 \
+        | tee "${{ runner.temp }}/log.txt" \
+        | tee >(grep -E "SCRIPT ERROR:|Can't open dynamic library" -q && {
+        	printf "\n::error::godot-itest: unrecoverable Godot error, abort...\n";
         	pkill godot
             echo "OUTCOME=godot-runtime" >> $GITHUB_ENV
         	exit 2

diff --git a/.github/composite/rust/action.yml b/.github/composite/rust/action.yml
@@ -31,11 +31,11 @@ runs:
   steps:
     - name: "Configure"
       id: configure
+      env:
+        CS: ${{ inputs.components }}
       run: |
         echo "Rust cache shared-key: '${{ runner.os }}-${{ inputs.rust }}${{ inputs.cache-key }}'"
-        echo "components=$( for c in ${cs//,/ }; do echo -n ' --component' $c; done )" >> $GITHUB_OUTPUT
-      env:
-        cs: ${{ inputs.components }}
+        echo "components=$( for c in ${CS//,/ }; do echo -n ' --component' $c; done )" >> $GITHUB_OUTPUT
       shell: bash
 
     - name: "Rustup"

diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml
@@ -202,17 +202,22 @@ jobs:
             godot-prebuilt-patch: '4.1'
 
           # Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks.
+          # See also https://rustc-dev-guide.rust-lang.org/sanitizers.html.
+          #
           # 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.
+          #
+          # There is also a gcc variant besides clang, which is currently not used.
           - name: linux-memcheck-nightly
             os: ubuntu-20.04
             artifact-name: linux-memcheck-clang-nightly
             godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
             godot-args: -- --disallow-focus
             rust-toolchain: nightly
-            rust-env-rustflags: -Zrandomize-layout
+            rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
             rust-extra-args: --features godot/custom-godot
+            # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two.
+            rust-target: x86_64-unknown-linux-gnu
             godot-prebuilt-patch: '4.1'
 
           # Linux under Godot 4.0.x
@@ -246,8 +251,9 @@ jobs:
             godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
             godot-args: -- --disallow-focus
             rust-toolchain: nightly
-            rust-env-rustflags: -Zrandomize-layout
-
+            rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
+            # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two.
+            rust-target: x86_64-unknown-linux-gnu
 
     steps:
       - uses: actions/checkout@v3
@@ -262,6 +268,7 @@ jobs:
           rust-extra-args: ${{ matrix.rust-extra-args }}
           rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}
           rust-env-rustflags: ${{ matrix.rust-env-rustflags }}
+          rust-target: ${{ matrix.rust-target }}
           with-llvm: ${{ contains(matrix.name, 'macos') && contains(matrix.rust-extra-args, 'custom-godot') }}
           godot-check-header: ${{ matrix.godot-check-header }}
 

diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs
@@ -385,7 +385,7 @@ fn make_central_items(api: &ExtensionApi, build_config: &str, ctx: &mut Context)
             .push(ident(&util::shout_to_pascal(name)));
         result
             .variant_op_enumerators_ord
-            .push(Literal::i32_unsuffixed(op.value));
+            .push(util::make_enumerator_ord(op.value));
     }
 
     for enum_ in api.global_enums.iter() {
@@ -499,7 +499,7 @@ fn make_enumerator(
     let enumerator_name = &type_names.json_builtin_name;
     let pascal_name = to_pascal_case(enumerator_name);
     let rust_ty = to_rust_type(enumerator_name, None, ctx);
-    let ord = Literal::i32_unsuffixed(value);
+    let ord = util::make_enumerator_ord(value);
 
     (ident(&pascal_name), rust_ty.to_token_stream(), ord)
 }

diff --git a/godot-codegen/src/class_generator.rs b/godot-codegen/src/class_generator.rs
@@ -684,7 +684,7 @@ fn make_notification_enum(
     let mut notification_enumerators_ord = Vec::new();
     for (constant_ident, constant_value) in all_constants {
         notification_enumerators_pascal.push(constant_ident);
-        notification_enumerators_ord.push(constant_value);
+        notification_enumerators_ord.push(util::make_enumerator_ord(constant_value));
     }
 
     let code = quote! {

diff --git a/godot-codegen/src/util.rs b/godot-codegen/src/util.rs
@@ -35,7 +35,7 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {
 
     for enumerator in values {
         let name = make_enumerator_name(&enumerator.name, &enum_.name);
-        let ordinal = Literal::i32_unsuffixed(enumerator.value);
+        let ordinal = make_enumerator_ord(enumerator.value);
 
         enumerators.push(quote! {
             pub const #name: Self = Self { ord: #ordinal };
@@ -120,7 +120,7 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {
                 let err = sys::PrimitiveConversionError::new(via);
                 let ord = i32::try_from(via).map_err(|_| err)?;
                 <Self as crate::obj::EngineEnum>::try_from_ord(ord).ok_or(err)
-            }
+        }
 
             fn try_into_via(self) -> std::result::Result<Self::Via, Self::IntoViaError> {
                 Ok(<Self as crate::obj::EngineEnum>::ord(self).into())
@@ -486,6 +486,10 @@ pub fn parse_native_structures_format(input: &str) -> Option<Vec<NativeStructure
         .collect()
 }
 
+pub(crate) fn make_enumerator_ord(ord: i32) -> Literal {
+    Literal::i32_unsuffixed(ord)
+}
+
 pub(crate) fn to_rust_expr(expr: &str, ty: &RustTy) -> TokenStream {
     // println!("\n> to_rust_expr({expr}, {ty:?})");
 

diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs
@@ -18,7 +18,7 @@ pub trait VarcallSignatureTuple: PtrcallSignatureTuple {
     // TODO(uninit) - can we use this for varcall/ptrcall?
     // ret: sys::GDExtensionUninitializedVariantPtr
     // ret: sys::GDExtensionUninitializedTypePtr
-    unsafe fn varcall<C: GodotClass>(
+    unsafe fn varcall(
         instance_ptr: sys::GDExtensionClassInstancePtr,
         args_ptr: *const sys::GDExtensionConstVariantPtr,
         ret: sys::GDExtensionVariantPtr,
@@ -35,7 +35,7 @@ pub trait PtrcallSignatureTuple {
 
     // Note: this method imposes extra bounds on GodotFfi, which may not be implemented for user types.
     // We could fall back to varcalls in such cases, and not require GodotFfi categorically.
-    unsafe fn ptrcall<C: GodotClass>(
+    unsafe fn ptrcall(
         instance_ptr: sys::GDExtensionClassInstancePtr,
         args_ptr: *const sys::GDExtensionConstTypePtr,
         ret: sys::GDExtensionTypePtr,
@@ -63,7 +63,6 @@ pub trait PtrcallSignatureTuple {
 //
 use crate::builtin::meta::*;
 use crate::builtin::{FromVariant, ToVariant, Variant};
-use crate::obj::GodotClass;
 
 macro_rules! impl_varcall_signature_for_tuple {
     (
@@ -102,7 +101,7 @@ macro_rules! impl_varcall_signature_for_tuple {
 
 
             #[inline]
-            unsafe fn varcall<C : GodotClass>(
+            unsafe fn varcall(
                 instance_ptr: sys::GDExtensionClassInstancePtr,
                 args_ptr: *const sys::GDExtensionConstVariantPtr,
                 ret: sys::GDExtensionVariantPtr,
@@ -135,7 +134,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
             type Params = ($($Pn,)*);
             type Ret = $R;
 
-            unsafe fn ptrcall<C : GodotClass>(
+            unsafe fn ptrcall(
                 instance_ptr: sys::GDExtensionClassInstancePtr,
                 args_ptr: *const sys::GDExtensionConstTypePtr,
                 ret: sys::GDExtensionTypePtr,

diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs
@@ -12,13 +12,12 @@ macro_rules! gdext_call_signature_method {
     (
         ptrcall,
         $Signature:ty,
-        $Class:ty,
         $instance_ptr:ident, $args:ident, $ret:ident,
         $func:expr,
         $method_name:ident,
         $ptrcall_type:path
     ) => {
-        <$Signature as $crate::builtin::meta::PtrcallSignatureTuple>::ptrcall::<$Class>(
+        <$Signature as $crate::builtin::meta::PtrcallSignatureTuple>::ptrcall(
             $instance_ptr,
             $args,
             $ret,
@@ -31,12 +30,11 @@ macro_rules! gdext_call_signature_method {
     (
         varcall,
         $Signature:ty,
-        $Class:ty,
         $instance_ptr:ident, $args:ident, $ret:ident, $err:ident,
         $func:expr,
         $method_name:ident
     ) => {
-        <$Signature as $crate::builtin::meta::VarcallSignatureTuple>::varcall::<$Class>(
+        <$Signature as $crate::builtin::meta::VarcallSignatureTuple>::varcall(
             $instance_ptr,
             $args,
             $ret,

diff --git a/godot-macros/src/method_registration/register_method.rs b/godot-macros/src/method_registration/register_method.rs
@@ -31,8 +31,8 @@ pub fn gdext_register_method(class_name: &Ident, method_signature: &TokenStream)
 
     let wrapped_method = wrap_with_unpacked_params(class_name, &signature_info);
 
-    let varcall_func = get_varcall_func(class_name, method_name, &sig, &wrapped_method);
-    let ptrcall_func = get_ptrcall_func(class_name, method_name, &sig, &wrapped_method);
+    let varcall_func = get_varcall_func(method_name, &sig, &wrapped_method);
+    let ptrcall_func = get_ptrcall_func(method_name, &sig, &wrapped_method);
 
     quote! {
         {
@@ -72,7 +72,6 @@ pub fn gdext_register_method(class_name: &Ident, method_signature: &TokenStream)
 }
 
 fn get_varcall_func(
-    class_name: &Ident,
     method_name: &Ident,
     sig: &TokenStream,
     wrapped_method: &TokenStream,
@@ -93,7 +92,6 @@ fn get_varcall_func(
                         godot::private::gdext_call_signature_method!(
                             varcall,
                             #sig,
-                            #class_name,
                             instance_ptr,
                             args,
                             ret,
@@ -119,7 +117,6 @@ fn get_varcall_func(
 }
 
 fn get_ptrcall_func(
-    class_name: &Ident,
     method_name: &Ident,
     sig: &TokenStream,
     wrapped_method: &TokenStream,
@@ -138,7 +135,6 @@ fn get_ptrcall_func(
                         godot::private::gdext_call_signature_method!(
                             ptrcall,
                             #sig,
-                            #class_name,
                             instance_ptr,
                             args,
                             ret,

diff --git a/godot-macros/src/method_registration/virtual_method_callback.rs b/godot-macros/src/method_registration/virtual_method_callback.rs
@@ -36,7 +36,6 @@ pub fn gdext_virtual_method_callback(
                 godot::private::gdext_call_signature_method!(
                     ptrcall,
                     #sig,
-                    #class_name,
                     instance_ptr,
                     args,
                     ret,