Segfault on dropping LuaFunction?

[luteberget]

Hello, and thanks for this library, it looks great.

I'm trying to write a configurable/scriptable program where one can register functions from a Lua script to be used later from a GUI Rust program. Running the following in release mode (Linux 64 bit, Rust 1.27.2) creates a segfault when program is dropped. Am I doing something wrong or strange?

extern crate rlua;
use rlua::prelude::*;

#[derive(Debug)]
struct Program<'a> {
    funcs: Vec<(String,LuaFunction<'a>)>,
}

fn mk_program<'a>(lua :&'a Lua, script :&str)  -> LuaResult<Program<'a>> {
    let mut program = Program { funcs: Vec::new()};
    lua.scope(|scope| {
        lua.globals().set("register", scope.create_function_mut(|_, tbl:LuaTable| {
            let name :String = tbl.get("name")?;
            let func :LuaFunction = tbl.get("func")?;
            program.funcs.push((name,func));
            Ok(())
        })?)?;
        lua.eval::<()>(script, None)?;
        Ok(())
    })?;
    Ok(program)
}

fn main(){
    let lua = Lua::new();
    let program = mk_program(&lua, r#"register { name = "test", func = function () end }"#).unwrap();
    println!("{:?}", program);
}

GDB output:

Program { funcs: [("test", Function(Ref(4)))] }

Program received signal SIGSEGV, Segmentation fault.
rlua::lua::Lua::drop_ref::h44c6ee14693a031e (self=0x7fffffffd278, lref=0x7ffff6a50018)
    at /home/bjlut/.cargo/registry/src/github.com-1ecc6299db9ec823/rlua-0.14.0/src/lua.rs:763
763	            ffi::lua_pushnil((*extra).ref_thread);
(gdb) bt
#0  rlua::lua::Lua::drop_ref::h44c6ee14693a031e (self=0x7fffffffd278, lref=0x7ffff6a50018)
    at /home/bjlut/.cargo/registry/src/github.com-1ecc6299db9ec823/rlua-0.14.0/src/lua.rs:763
#1  _$LT$rlua..types..LuaRef$LT$$u27$lua$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h5c2fb308405493c3 (self=0x7ffff6a50018)
    at /home/bjlut/.cargo/registry/src/github.com-1ecc6299db9ec823/rlua-0.14.0/src/types.rs:92
#2  0x0000555555562ad5 in core::ptr::drop_in_place::h4eff9b84948e99de () at /checkout/src/libcore/ptr.rs:59
#3  core::ptr::drop_in_place::hc0a0e904c5e46691 () at /checkout/src/libcore/ptr.rs:59
#4  core::ptr::drop_in_place::hf24a5bf0280dbaeb () at /checkout/src/libcore/ptr.rs:59
#5  core::ptr::drop_in_place::h4dff6797faf34f16 () at /checkout/src/libcore/ptr.rs:59
#6  _$LT$alloc..vec..Vec$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h6e6cc1341dca5ef0 (self=<optimized out>)
    at /checkout/src/liballoc/vec.rs:2166
#7  core::ptr::drop_in_place::hdeb56b48f40b6a81 () at /checkout/src/libcore/ptr.rs:59
#8  core::ptr::drop_in_place::heebf2eae8bc36947 () at /checkout/src/libcore/ptr.rs:59
#9  luasegfault::main::h91edc85fef633fb6 () at src/main.rs:28
#10 0x0000555555560763 in std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::hf36a085e69e29833 () at /checkout/src/libstd/rt.rs:74
#11 0x00005555555ae183 in std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::h86ba874310c8f41e () at libstd/rt.rs:59
#12 std::panicking::try::do_call::h64129d2b0e54f3b8 () at libstd/panicking.rs:310
#13 0x00005555555c4dca in __rust_maybe_catch_panic () at libpanic_unwind/lib.rs:105
#14 0x00005555555b2156 in std::panicking::try::hceef11cfeb87cfe0 () at libstd/panicking.rs:289
#15 std::panic::catch_unwind::h331e7e117781c30b () at libstd/panic.rs:374
#16 std::rt::lang_start_internal::h6264a91317866dd6 () at libstd/rt.rs:58
#17 0x0000555555562d5a in main ()

[kyren]

This shouldn't compile, this is 100% a soundness issue, and I have a fix incoming... I think. I need to do a lot more testing to make sure this fix is right.

As soon as I fix the soundness issue, I'll help you through what you're trying to do, because like I said this SHOULDN'T compile, but it does because of a really bad issue :(

This is a GREAT find, thank you so much for finding this :(

[kyren]

I think a2615a8 fixes this issue, in that the example program you gave no longer compiles and there is now a compiletest_rs test for this specific issue.

The issue allowed parameters passed to callbacks created from Lua::scope to be stored outside the callback, which should never be allowed. Handles that have a 'lua lifetime are not supposed to be stored "long term" at all, really, since they reference whatever specific instance of Lua that they come from and can't outlive it. The Lua instance passed to callbacks is ephemeral, it lives only as long as the callback, so when you convince a handle value to escape the callback, it's referencing a dropped Lua, which is what caused this crash.

This was only a problem inside Lua::scope, because normally functions must be 'static anyway so the borrow checker wouldn't allow sneaking a non-'static reference out. The fix is to add a lifetime bound in Scope::create_function / Scope::create_function_mut to make sure that the 'scope outlives 'callback. It's a bit subtle, but since 'scope must outlive 'callback, and the callback itself must outlive 'scope, this means that the callback can't exfiltrate anything with the 'callback lifetime at all, which is what we want.

This fix probably doesn't help your actual problem, since it just makes this example not compile. The good news is you might not need Lua::scope at all to do what you want to do, that's really only for when you have non-'static types other than Lua handles to deal with. If you simply want to be able to store Lua handles long term, try using the Lua registry with Lua::create_registry_value.

[luteberget]

Thanks so much for the explanation!

I guess I thought that Rust references to Lua values could be carried around as long as the top-level Lua instance is alive (since they are refcounted), but I'm not familiar enough with the low-level Lua API to see exactly how this would work.

The Lua::scope was intended to limit the mutability of the program struct, so I think it still makes sense. Instead of storing LuaFunction in the Program struct, I'll try storing a LuaRegistryKey and also the &Lua top-level instance to delete from the registry on drop. Something like this:

struct StoredFunction<'a>(&'a Lua, Option<LuaRegistryKey>);

impl<'a> StoredFunction<'a> {
    pub fn new(lua :&'a Lua, f :LuaFunction) -> LuaResult<StoredFunction<'a>> {
        Ok(StoredFunction(lua, Some(lua.create_registry_value(f)?)))
    }
    
    pub fn call<A: rlua::ToLuaMulti<'a>, R: rlua::FromLuaMulti<'a>>(&self, args: A) -> LuaResult<R> {
        self.0.registry_value::<LuaFunction>(self.1.as_ref().expect("Stored function error"))
            .and_then(|x| x.call(args))
    }
}

impl<'a> Drop for StoredFunction<'a> {
    fn drop(&mut self) {
        self.0.remove_registry_value(self.1.take().unwrap()).unwrap();
    }
}

[kyren]

I guess I thought that Rust references to Lua values could be carried around as long as the top-level Lua instance is alive (since they are refcounted), but I'm not familiar enough with the low-level Lua API to see exactly how this would work.

There are actually multiple Lua instances, when a callback is called by Lua and given lua_State pointer, a new "ephemeral" Lua instance is created. The fact that all the Lua instances are potentially different and the specific 'lua lifetime used only ends up mattering in Scope methods, because like I said normally callbacks must be 'static anyway.

If you're okay with storing a a &Lua in a type, you can actually just store normal reference types, it's just generally a huge pain to deal with persistently borrowing &Lua, and you tend to run head-first into self borrows, which are even more of a pain to deal with. The reason that RegistryKey exists really is to save you from having to store a &Lua somewhere in the first place. If you wanted a way to get handles out via a "register" callback with, one way is to create a table in Rust, then make a closure that closes over that table and pass that into Lua as the "register" function, and have that closure populate the table. You can also turn the API around to have Lua just return a table of functions, or use an agreed upon global variable to store them.

I know the handle based API has some limitations and sometimes you could envision the API without those limitations, but generally this is because handles are just not designed to be stored inside Lua itself, so anything involving capturing them in callbacks or getting them out of callbacks is probably going to be painful.

[kyren]

In the meantime, I have found another soundness bug along the same lines with a closure capturing its own arguments into an owned value rather than a captured reference, and have had to attempt another fix (1a9c50f). If anybody is confident in their lifetime knowledge and wants to test this out and try to break it, I would be very appreciative, because I'm not very confident there isn't another issue here. I at least have coverage of all the issues with compiletest_rs, but I would really appreciate another set of eyes here.

[kyren]

I went ahead and released 0.14.2 as well with the second fix, hopefully I'm not missing any other safety issues here.

[luteberget]

Great, thanks again for your help.

Just for information, I went ahead with storing the functions in a global table on the Lua side, and then creating a Rc<RefCell<Option<Context>>> to use as a parameter for the functions, to be able to remove the Context on the Rust side again after executing the Lua functions from the global table.