Object::set_script on owner is UB

On the Godot side, Object::set_script immediately frees the script instance, causing self to point to garbage for the rest of the function. Any operations on self is consequently UB because of the invalid state. This is problematic because it's expected that owner is an unsafe pointer, but not self, which should be a safe reference.

set_script is incidentally marked unsafe, but only because Object is not reference counted, so it may still come as a surprise to users who call it, because the unsafe keyword may be used so liberally that it no longer makes users alert.

Take this modified hello_world example that showcase the problem:

#[macro_use]
extern crate gdnative;

#[derive(gdnative::NativeClass)]
#[inherit(gdnative::Node)]
struct HelloWorld(i64);

#[gdnative::methods]
impl HelloWorld {
    fn _init(_owner: gdnative::Node) -> Self {
        HelloWorld(42)
    }

    #[export]
    fn _ready(&self, mut owner: gdnative::Node) {
        unsafe {
            owner.set_script(None);
        }
        assert_eq!(42, self.0, "self should not be holding garbage");
        godot_print!("hello, world.")
    }
}

fn init(handle: gdnative::init::InitHandle) {
    handle.add_class::<HelloWorld>();
}

godot_gdnative_init!();
godot_nativescript_init!(init);
godot_gdnative_terminate!();

Output:

thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `42`,
 right: `944####680`: self should not be holding garbage', examples/hello_world/src/lib.rs:19:9

Dealing with the problem

As there are more ways that owner can be freed (along with its script instance), it might be best to replace the userdata Box with an Rc (or even Arc. I'm not sure if it's possible for Godot to free an Object from another thread) to prevent UB even if the Godot side called the destructor midway.

[karroffel]

I was starting to write a lot of stuff about this problem on the review for #181, I decided it's probably better to discuss the issue here and then see how this might affect the PR.

Unfortunately the usual assumptions about Rust's ownership/borrowing do not hold true for self in the GDNative bindings. It is possible to have a method take &mut self as the self argument. Since in GDScript it's okay to pass object references between threads another thread could call another &mut self or even just &self method in the meantime, so the aliasing assumptions of mutable references can be violated even from Rust itself in safe code (using Variant and call (if that's implemented, I do not remember right now 😄 ) ).

---

To make it "safe" one way might be to use Arc<RwLock<T>> but there we come to a point where trying to add in safety has quite some run-time cost.

I remember a conversation here on Github with @nical about this kind of stuff and we two agreed that safety is good if it's not too costly. At the end of the day this is a still a binding to a game-engine where you most likely picked a natively-compiling language for performance reasons.

I think adding an Arc<RwLock<T>> to all instances and a mutex-acquire/release for all method calls might be a bit overkill. Rc<RefCell<T>> is a bit nicer in that regard but does not actually prevent UB either.

---

We already have a function in place to override specific methods during binding generation, I would just suggest to always make set_script unsafe in case in future a group of functions is marked as safe for some reason.

Since we are dealing with Godot which is accessed via a FFI and uses an unsafe language with scripting languages that do not have guarantees like Rust we have to identify and accept the places where UB can happen.

Imagine we manage to solve this case of UB, what about having a method called from two threads (possible from two threads from GDScript) and one of them panics and unwinds. At that point the other thread might observe UB as well. This could actually be solved by the Arc<RwLock<T>> thing, but performance implications make this have a bad backtaste.

---

Maybe one thing that could be done is instead of using Box we can use Arc<UnsafeCell<T>> knowing that safety can be violated from other scripts at any time anyway. This way we can prevent the issue with set_script and it should be less costly than Arc<RwLock<T>> while being less safe (but more predictable in terms of performance).

Maybe @nical can give an opinion on this as well, I might be missing something that can actually make this safe and not costly or maybe I'm missing something else and even my other suggestions are wrong 😅

Indeed, what you said about Godot is true, and there is a bigger problem underlying this specific surface case. I was in too much of a haste to report and attempt a fix without digging deep enough first.

Now that the problem is properly identified, I think we shouldn't just say that it's expected and move on. Usually people don't reach for Rust just for the raw performance. Rather, it's performance predicated on the condition that safe things are safe, and unsafe things are marked unsafe. The breaking of safety assumptions undermines Rust's value by a great margin. To me, at least.

And it's about what the compiler assumes, too. Although rustc probably won't reorder things around FFI functions and transmutes, with sufficiently complex code (i.e. actual game), optimization becomes hard to reason about, and may bring some nasty surprises when the assumptions it relies on are broken. I'd rather not take that risk.

---

Now, while I think safety should be the default, user choice may be possible with unsafety being opt-in, using associated types. Consider this definition of NativeClass:

pub trait NativeClass: Sized {
    type Base: GodotObject;
    type UserData: UserData<Target=Self>; // = SafeUserData<Self> with associated type defaults?
    // ...
}

pub unsafe trait UserData: Sized {
    type Target: NativeClass;

    fn new(val: Self::Target) -> Self;
    unsafe fn into_user_data(this: Self) -> *const libc::c_void;
    unsafe fn consume_user_data_unchecked(ptr: *const libc::c_void) -> Self;
    unsafe fn clone_from_user_data_unchecked(ptr: *const libc::c_void) -> Self;

    fn map<F, U>(this: &Self, op: F) -> U
    where
        F: FnOnce(&mut T) -> U; // Can be made something like `for<'a> FnOnce(Self::Ref<'a>) -> U` with associated type constructors, so the deref can be explicitly unsafe as well?
}

pub struct SafeUserData<T>(Arc<RwLock<T>>);

unsafe impl<T: NativeClass> UserData for SafeUserData<T> {
    type Target = T;
    // ...
}

So, instead of a singular NativeInstanceWrapper to fit all, we allow anything the user likes to be used as user data: Arc<RwLock<T>>, Arc<UnsafeCell<T>> or just a plain old Box<RefCell<T>>, so long as it can be passed to Godot and passed back. I think it might be a good middle ground if there is no cheap way to make things safe, so people don't pay for what they didn't ask for.

---

We already have a function in place to override specific methods during binding generation, I would just suggest to always make set_script unsafe in case in future a group of functions is marked as safe for some reason.

Since we are dealing with Godot which is accessed via a FFI and uses an unsafe language with scripting languages that do not have guarantees like Rust we have to identify and accept the places where UB can happen.

It is fine for unsafe functions to cause UB when used incorrectly, that's why they're unsafe after all. But with a Box wrapper, there is no way to call set_script and friends without immediately and unconditionally causing UB, by effectively creating an instance with invalid data. That's a bit bad to have in the bindings in my opinion.

Another way to prevent the "locks on every method call" situation may be to force the interior mutability pattern, by making NativeClass Send + Sync, and requiring that all exported functions take &self. The userdata will be only Arc<T>.

The downside is obvious: users will have to handle their own locks, so the code will not be the most straightforward.

---

This may work together with the UserData suggestion even without ATC, as methods are wrapped with a macro rather than a generic function: map can be implemented individually on each UserData type rather than as a trait item, and it should work just as fine. We can provide several implementations for different needs:

• Arc<Mutex<T>> where T: Send, gives &mut.
• Arc<RwLock<T>> where T: Send + Sync, gives &mut.
• Arc<T> where T: Send + Sync, immutable references only.

[karroffel]

I think we shouldn't just say that it's expected and move on. Usually people don't reach for Rust just for the raw performance. Rather, it's performance predicated on the condition that safe things are safe, and unsafe things are marked unsafe.

Indeed, I fear like I didn't express myself properly. I do think we should strive to be as clear and "honest" about the safety as we can while making things as safe as possible!

---

I do like the suggestion of using associated types to define the implementation, this is a really nice idea I didn't think of yet!

I realized that even with the Box<RefCell<T>> approach you can cause panics by using variant calls, so it wouldn't actually be much worse than a deadlock when using RwLock<T> instead.

specs is using a similar system for customizing the backing storage of components and that works quite nicely! So I think this is a nice middle ground. The derive macro could be extended to make this customizable too, I like this solution!

I realized that even with the Box<RefCell<T>> approach you can cause panics by using variant calls, so it wouldn't actually be much worse than a deadlock when using RwLock<T> instead.

Panics and deadlocks are technically safe in Rust parlance, so that's a bit different from causing aliasing from safe code, but deadlocks sure are very problematic too.

Maybe we can make map return Result, and make the locking UserData implementations customizable using associated constants (until constant generics become possible):

pub enum DeadlockPolicy {
    Allow, // locks normally
    Pessimistic, // returns Nil immediately on try_lock failure
    Timeout(Duration), // via parking_lot, or using custom code over std Mutex
}

pub trait LockOptions {
    const DEADLOCK_POLICY: DeadlockPolicy;
}

impl<T: NativeClass + Send, O: LockOptions> MutexUserData<T, O> {
    // ... stuff ...
    fn map<F, U>(this: &Self, op: F) -> Result<U, Err>
    where
        F: FnOnce(&mut T) -> U,
    {
        // the match will just be optimized away, so there will be no runtime cost: https://rust.godbolt.org/z/S_to5o
        match O::DEADLOCK_POLICY {
            Allow => { /* ... */ },
            Pessimistic => { /* ... */ },
            Timeout(dur) => { /* ... */ },
        }
    }
}

Then we can modify godot_wrap_method so that it prints the Err and returns Nil upon failure to acquire lock, just like how it behaves currently when the inner function panics.

That, along with documentation on what could potentially cause deadlocks (Variant calls on anything, anything that could emit non-deferred signals, downcasting owner to Instance when that's available, maybe?) attached to the appropriate UserData types, should be enough, I suppose.