UB at GDNative termination (seen during integration tests)

[B-head]

Issue Codes:
https://github.com/godot-rust/godot-rust/blob/25603b047364cde1ab1267cb41579a2734ab01d5/gdnative-core/src/object/instance.rs#L104-L209

Reference Information:
#891 (comment)

[Bromeon]

Thanks for the report, and for narrowing it down!
From your linked comment:

This [memory leak] seems to have occurred at least three months ago in CI. https://github.com/godot-rust/godot-rust/runs/5280455202?check_suite_focus=true#step:3:1000

And I could not reproduce the bug in my environment. Instead, the external terminal causes the segmentation fault. 😵

... Undefined behavior! 🎲 (I was going to open the issue after I narrowed down the code that caused UB)

Can you describe the exact circumstances that caused this?

• Godot version
• godot-rust Git revision
• which test caused the segmentation fault (if possible)
• operating system and version

It's a real pity we can't use miri. FFI is a ticking time bomb.

[B-head]

• Godot version
  • Godot_v3.4.3-stable_win64
• godot-rust Git revision
  • All revisions I have worked on so far.
• which test caused the segmentation fault (if possible)
  • At least, test_rust_class_construction().
• operating system and version
  • Windows 10 Pro - 19044.1706
• terminal
  • Git Bash in Git for Windows.

I am currently investigating the bug.

[B-head]

Fixed memory leak of NativeScript object.
Edit: After updating the Godot version to 3.4.4, the segmentation fault no longer occurs.

I think the UB occurred before here.

Ah... there's this.

[Bromeon]

It could be quite hard to consistently reproduce this... Instance::try_emplace() is used by Instance::emplace(), and I've been using this in many applications without issues. Of course that doesn't mean there's no UB, but it means that we will need to investigate more closely under which circumstances this is caused.

Ideally we find some minimal code (simple godot-rust entry point + one #[derive(NativeClass)]) that causes the issue, then we can test it very easily.

We can also use CI, I'll gladly help a dedicated Windows config in GitHub action to run/test such a minimal example.

Ah... there's this.

What do you mean? This is a highly unsafe pointer cast for sure. But is it the line that caused the segfault for you?

[B-head]

What do you mean? This is a highly unsafe pointer cast for sure. But is it the line that caused the segfault for you?

I don't know, but that is the only thing I have not investigated yet.

What I have done so far:

• Use todo!() to avoid type checking of return values.
  • Doing this makes the code easy to comment out.
• Remove unnecessary casts.
• Use std::ffi instead of libc.
• See how arguments are used in the engine codes.
• See codes for the Refarence methods.
• Check for appropriate other library codes.
• Check that the NativeScript object has been constructed.
• Narrowed down that the inclusion of this code will cause segmentation fault.
  • https://github.com/B-head/godot-rust/blob/1b8e658cb9ddb17c60c176bfcc1c22354d265fef/gdnative-core/src/object/instance.rs#L109-L152
  • https://github.com/B-head/godot-rust/blob/1b8e658cb9ddb17c60c176bfcc1c22354d265fef/gdnative-core/src/object/instance.rs#L211-L213
• Narrowed down that the inclusion of this code will cause memory leak of NativeScript object.
  • https://github.com/B-head/godot-rust/blob/1b8e658cb9ddb17c60c176bfcc1c22354d265fef/gdnative-core/src/object/instance.rs#L109-L139
  • https://github.com/B-head/godot-rust/blob/1b8e658cb9ddb17c60c176bfcc1c22354d265fef/gdnative-core/src/object/instance.rs#L211-L213
• This unsound cast is not the cause of UB.

Edit: I tried my best but it didn't work.
B-head@5e59f8a

[Bromeon]

I think this might not be related to maybe_emplace() or anything close, but to the termination of the GDNative module by Godot. A quick look with the debugger showed a segmentation fault in NativeScriptLanguage::_unload_stuff() of Godot's nativescript.cpp. This might be necessary to look into for #910.

For context:

• Fix tests with Godot 3.3 #746
• Recommend reloadable = false with Godot 3.3, and detect when nativescript_init is being called a second time #757
• [3.x] Don't terminate library if other GDNatives are still using it godotengine/godot#49467

While it wasn't a big issue in Godot 3.4, it seems to show up again for the Godot 3.5 RCs on Windows.

If someone has the time to confirm/reproduce or even help debugging this, that would be greatly appreciated.

[Waridley]

You could assign this to me if you want since I'm already familiar with the issue in 3.x. I should be able to look at it this week

[Bromeon]

@Waridley Thank you very much, that's greatly appreciated! ❤️

[Waridley]

Not sure if it's related, but I'm getting a consistent segfault here: https://github.com/godot-rust/godot-rust/blob/1b4754ca073e97242d3cf8d26afd865af889a344/test/src/lib.rs#L227

It's like the GodotString created from "foo" is dropped before actually calling get_meta(). Changing it to this pinpoints the problem:

{
    let foo = Reference::new().into_shared();
    let foo = unsafe { foo.assume_safe() };
    foo.set_meta("foo", "bar");

    instance_id = foo.get_instance_id();

    assert!(unsafe { Node::try_from_instance_id(instance_id).is_none() });

    let reconstructed = unsafe { Reference::from_instance_id(instance_id) };
    
    let key2 = GodotString::from("foo");
    let val = reconstructed.get_meta(key2);
    assert_eq!(
        "bar",
        String::from_variant(&val).unwrap()
    );
    let _SEGFAULT = reconstructed.get_meta("foo");
}

[Waridley]

Maybe the compiler is optimizing drop(arg0) to before the FFI call?

#[doc(hidden)]
#[inline(never)]
pub(crate) unsafe fn icallptr_var_str(
    method_bind: *mut sys::godot_method_bind,
    obj_ptr: *mut sys::godot_object,
    arg0: GodotString,
) -> sys::godot_variant {
    let gd_api = get_api();
    let mut argument_buffer: [*const libc::c_void; 1usize] = [arg0.sys() as *const _ as *const _];
    let mut ret = sys::godot_variant::default();
    let ret_ptr = (&mut ret) as *mut _;
    (gd_api.godot_method_bind_ptrcall)(
        method_bind,
        obj_ptr,
        argument_buffer.as_mut_ptr() as *mut _,
        ret_ptr as *mut _,
    );
    drop(arg0);
    ret
}

[Waridley]

Maybe the compiler is optimizing drop(arg0) to before the FFI call?

Confirmed, using ManuallyDrop fixes it

[Bromeon]

Going to optimistically close this.
Should UB reappear, we can reopen.