Rust's newly added `assert_unsafe_precondition` makes GDNative .dlls that are compiled in debug mode crash on startup

[Houtamelo]

Minimal reproducible project: GDNative - Bug Reproduction.zip

Compiled using Rust version 1.79.0-nightly (8b2459c1f 2024-04-09).

Godot version: 3.5.3 Platform: Windows 10 64 bit

How to Reproduce

1. Compile the rust project in debug mode. (run cargo build in the directory "rust")
2. Replace the .dll file in the godot project (path: "godot\Bin\gdnative_minimal_bug_reproduction.dll") with the generated .dll file (path: "target/debug/gdnative_minimal_bug_reproduction.dll").
3. Open the project using Godot's editor.
4. Run the scene "Node.tscn"
5. The game will crash during startup, without printing "Hello World" in the console.

Steps 1 and 2 can also be done by running "build_and_move_debug.bat"

---

Since assert_unsafe_precondition is stripped in release mode, this bug only happens in debug mode. This can be proven by mimicking the reproduction steps but compiling in release mode:

1. Compile the rust project in release mode. (run cargo build --release in the directory "rust")
2. Replace the .dll file in the godot project (path: "godot\Bin\gdnative_minimal_bug_reproduction.dll") with the generated .dll file (path: "target/release/gdnative_minimal_bug_reproduction.dll").
3. Open the project using Godot's editor.
4. Run the scene "Node.tscn"
5. The game will run normally, printing "Hello World" in the console.

Steps 1 and 2 can also be done by running "build_and_move_release.bat"

---

Additional info

When using the panic handler recipe in the book, it manages to catch a stack trace of the first error:

E 0:00:00.501   <unset>: [RUST] file 'library\core\src\panicking.rs' at line 215: panic occurred: "unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`"
  <C++ Source>  rust\src\lib.rs:67 @ <unset>()

This stack trace was gathered on another project though, here's the script:

fn init_panic_hook() {
    let old_hook = std::panic::take_hook();
    std::panic::set_hook(Box::new(move |panic_info| {
        let loc_string;
        if let Some(location) = panic_info.location() {
            loc_string = format!("file '{}' at line {}", location.file(), location.line());
        } else {
            loc_string = own!("unknown location")
        }

        let error_message;
        if let Some(s) = panic_info.payload().downcast_ref::<&str>() {
            error_message = format!("[RUST] {}: panic occurred: {:?}", loc_string, s);
        } else if let Some(s) = panic_info.payload().downcast_ref::<String>() {
            error_message = format!("[RUST] {}: panic occurred: {:?}", loc_string, s);
        } else {
            error_message = format!("[RUST] {}: unknown panic occurred", loc_string);
        }
 line 67 ->>>    godot_error!("{}", error_message);  <<<- line 67
        (*(old_hook.as_ref()))(panic_info);

        unsafe {
            if let Some(gd_panic_hook) = autoload::<Node>("rust_panic_hook") {
                gd_panic_hook.call("rust_panic_hook", &[GodotString::from_str(error_message).to_variant()]);
            }
        }
    }));
}

Current workaround is to simply not use debug mode, but that disables some useful assertions made by GDNative that can catch many UB cases.

[mivort]

Starting from Rust 1.78, debug builds now check for null pointer in slice_from_raw_parts method, so now it seems to crash on start here (null pointer gets passed along with zero length for empty slices): https://github.com/godot-rust/gdnative/blob/639aae777da653c8da11c80732a9febba5b80315/gdnative-core/src/export/method.rs#L320-L322

The rationale for considering null pointers invalid for zero-length slices is that Option monad is optimized on compiler level in a way that zero-values are considered as None (so if zero-length slice is constructed from 0x0/0x0 parts, it would be matched as None instead of Some() inside of Option). It's perfectly fine to pass any non-null bogus, e.g. 0x1 etc.

I'm currently using this a quick workaround:

let args = if num_args > 0 { std::slice::from_raw_parts(args, num_args as usize) } else { &[] };

...but it may be sub-optimal, as it adds a branching condition in a pretty hot path. So maybe there's a better way to introduce a validation for zero-length slices and prevent passing null as args.

[BenjiFlaming]

Can confirm that I'm experiencing this under Ubuntu 20.04 (Linux 64-bit) with Rust 0.78.0 and Godot 3.5.3.

Can also confirm that the workaround fixed the crash here - thanks so much for sharing it! :)

[BenjiFlaming]

Iterating on the workaround, I've adjusted my copy of method.rs to include the original code in release builds (and the workaround in debug builds). Not necessarily the best way to do it, but sharing in case it's helpful:

    pub unsafe fn from_sys(num_args: libc::c_int, args: *mut *mut sys::godot_variant) -> Self {
        let args = Self::safe_args(num_args, args);
        let args = std::mem::transmute::<&[*mut sys::godot_variant], &[&Variant]>(args);
        Self {
            idx: 0,
            args,
            offset_index: 0,
        }
    }

    /// Convert args to a slice in debug builds - avoids crash when using Rust 1.78 and beyond
    #[doc(hidden)]
    #[inline]
    #[cfg(debug_assertions)]
    unsafe fn safe_args(
        num_args: libc::c_int,
        args: *mut *mut sys::godot_variant,
    ) -> &'a [*mut sys::godot_variant] {
        if num_args > 0 {
            std::slice::from_raw_parts(args, num_args as usize)
        } else {
            &[]
        }
    }

    /// Convert args to a slice in release builds - using more effecient code than the version above
    #[doc(hidden)]
    #[inline]
    #[cfg(not(debug_assertions))]
    unsafe fn safe_args(
        num_args: libc::c_int,
        args: *mut *mut sys::godot_variant,
    ) -> &'a [*mut sys::godot_variant] {
        std::slice::from_raw_parts(args, num_args as usize)
    }

[bend-n]

the only way to avoid such a branch is by simply keeping it as a pointer, instead of turning it into a slice, but that would take some work.