mlua-rs / mlua

High level Lua 5.4/5.3/5.2/5.1 (including LuaJIT) and Roblox Luau bindings to Rust with async/await support
Other
1.76k stars 139 forks source link

`UserDataRef` is no longer working in 0.10.0 #475

Closed sxyazi closed 1 month ago

sxyazi commented 1 month ago

I have the following code that worked in 0.9.9:

use mlua::UserDataRef;

struct Ctx {
  foo: String,
}

type CtxRef<'lua> = UserDataRef<'lua, Ctx>;

fn main() -> mlua::Result<()> {
  let cx = Ctx { foo: "Hello world!!!".to_string() };

  let lua = mlua::Lua::new();
  lua.scope(|scope| {
    lua.set_named_registry_value("cx", scope.create_any_userdata_ref(&cx)?)?;

    lua.globals().set(
      "test",
      lua.create_function(|lua, ()| {
        let cx: CtxRef = lua.named_registry_value("cx")?;
        Ok(cx.foo.clone())
      })?,
    )?;

    lua.load(r#" print(test()) "#).exec()?;

    Ok(())
  })
}

which prints:

Hello world!!!

In mlua 0.10.0, the lifetime bound of UserDataRef was removed, so I replaced it with:

- type CtxRef<'lua> = UserDataRef<'lua, Ctx>;
+ type CtxRef = UserDataRef<Ctx>;

but it doesn't work now:

Error: CallbackError { traceback: "stack traceback:\n\t[C]: in function 'test'\n\t[string \"src/main.rs:24:13\"]:1: in main chunk", cause: UserDataTypeMismatch }

I tried using &Ctx instead of Ctx, but doesn't work either:

- type CtxRef<'lua> = UserDataRef<'lua, Ctx>;
+ type CtxRef<'a> = UserDataRef<&'a Ctx>;
khvzak commented 1 month ago

Unfortunately UserDataRef in scope context has a soundness bug in v0.9, so it no longer allowed.

The right way in v0.10 would be:

        lua.globals().set(
            "test",
            lua.create_function(|lua, ()| {
                Ok(lua
                    .named_registry_value::<AnyUserData>("cx")?
                    .borrow_scoped(|cx: &Ctx| cx.foo.clone()))
            })?,
        )?;

or alternatively:

    lua.scope(|scope| {
        let cx = scope.create_any_userdata_ref(&cx)?;

        lua.globals().set(
            "test",
            lua.create_function(move |_, ()| Ok(cx.borrow_scoped(|cx: &Ctx| cx.foo.clone())))?,
        )?;

        lua.load(r#"print(test()) "#).exec()?;

        Ok(())
    })
sxyazi commented 1 month ago

Thanks for the response @khvzak!

I tried fixing all the named_registry_value() parts in my code by wrapping them in an extra layer of borrow_scoped().

But I found that I'm using a lot of UserDataRef in my function parameters, and I can't be sure if they come from scope, since they're passed in by the user, and my function is expected to work properly in both scoped and non-scoped contexts.

So now I have to write:

type UrlRef<'lua> = UserDataRef<'lua, crate::url::Url>;

let copy_file = lua.create_function(|lua, (from, to): (UrlRef, UrlRef)| {
  match std::fs::copy(&*from, &*to) {
    Ok(n) => (n, Value::Nil).into_lua_multi(&lua),
    Err(e) => (Value::Nil, e.raw_os_error()).into_lua_multi(&lua),
  }
})?;

as:

let copy_file = lua.create_function(|lua, (from, to): (AnyUserData, AnyUserData)| {
  from.borrow_scoped(|from: &crate::url::Url| {
    to.borrow_scoped(|to: &crate::url::Url| {
      match std::fs::copy(&*from, &*to) {
        Ok(n) => (n, Value::Nil).into_lua_multi(&lua),
        Err(e) => (Value::Nil, e.raw_os_error()).into_lua_multi(&lua),
      }
    })
  })?? // <- We need a double question mark here since each closure returns a Result, and `create_function` also expects a Result
})?;

It feels a bit verbose and creates unnecessary indentation levels.

Is UserDataRef just temporarily disabled in v0.10.0, or will it no longer support scoped usage permanently, and is there any way to simplify this code?

khvzak commented 1 month ago

But I found that I'm using a lot of UserDataRef in my function parameters, and I can't be sure if they come from scope, since they're passed in by the user, and my function is expected to work properly in both scoped and non-scoped contexts.

AnyUserData::borrow_scoped work for any context, scoped and non-scoped.

Is UserDataRef just temporarily disabled in v0.10.0, or will it no longer support scoped usage permanently, and is there any way to simplify this code?

The problem with UserDataRef in v0.9 in scoped context is that it can outlive scope and refer to freed memory:

let lua = mlua::Lua::new();
let cx: CtxRef = {
    let cx = Ctx {
        foo: "Hello world!!!".to_string(),
    };

    lua.scope(|scope| {
        lua.set_named_registry_value("cx", scope.create_any_userdata_ref(&cx)?)?;
        lua.named_registry_value("cx")
    })?
};
println!("{:?}", cx.foo); // use after free!

So we need to make sure that obtained reference is temporary and will never outlive scope (what the borrow_scoped does). I don't know a good safe solution for this. If you're happy to use unsafe the problem can be solved with a simple helper:

struct UserDataUnsafeRef<T: 'static>(*const T);

impl<T> FromLua for UserDataUnsafeRef<T> {
    fn from_lua(value: Value, lua: &Lua) -> Result<Self> {
        let ud = AnyUserData::from_lua(value, lua)?;
        ud.borrow_scoped(|t| UserDataUnsafeRef(t as *const T))
    }
}

impl<T> Deref for UserDataUnsafeRef<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        unsafe { &*self.0 }
    }
}

Then UserDataRef can be replaced with UserDataUnsafeRef and everything should work as before.

sxyazi commented 1 month ago

I see the bug with UserDataRef now, I'll go with borrow_scoped to see if I can make it simpler, thanks for your great explanation and examples, and congrats on the new 0.10 release!