lua_pcall/lua_error invocations might be unsafe (all of them)

[jonas-schievink]

Currently, rlua makes sure to only call Lua API functions that can cause an error from within error_guard, which uses lua_pcall to run a Rust closure in a protected environment. This causes all Lua errors to be caught by lua_pcall.

Since Lua's error handling uses setjmp and longjmp to do non-local control flow, an error will longjmp to the lua_pcall, skipping across the Rust closure passed to error_guard. According to IRC this is undefined behaviour in Rust. EDIT: People are also claiming that it's unsafe in this user forum thread.

Fixing this is hard. It would require that all uses of lua_pcall only call C functions. And calling lua_error from Rust would never be a safe thing to do, since it must skip across a Rust stack frame.

If this is really UB, the only way to write a safe Lua wrapper would involve writing C code. Not sure if that's really the case, though...

[kyren]

So, it's things like this and #19 that make me think that writing my own Lua interpreter in Rust might be simpler than writing a sound bindings system.

I was actually aware of this before, but I was being a bit fast and loose with error_guard.. the parts of the closures passed to error_guard that could longjmp were all at the beginning, and I was assuming (probably incorrectly) that this would be safe since nothing would be on the stack. I now understand that.. I suppose it is IMPOSSIBLE to write sound rust that calls into C that may longjmp, so the only sound way to do this is to take everything that is currently inside an error_guard, and write some C code for it.

In the meantime, I have dramatically shortened what is actually inside error_guard in commit 2bd7a2e, but I suppose to TECHNICALLY be sound, I HAVE to write that code in C instead of lua somehow. In the meantime, the rust code that is inside error_guard is just calls to C code and the only things on the stack are Copy types.

[jonas-schievink]

Looks like all error_guard calls now only wrap a single Lua API function (+ lua_pop, which can be moved outside the error_guard). We could make do with a reasonably small set of C functions that get passed a function pointer and calls it in protected mode. Not sure if that's the best idea, but it certainly results in the least amount of C code written.

To be able to safely call lua_error, we'd have to change callbacks to be able to return an error code. At first glance, making lua_CFunction able to return -1 to indicate "throw error at top of stack" could work.

[kyren]

So, I don't think writing some custom C code will matter, because if you take the stance that a longjmp with rust anywhere on the stack being lonjmped over is unsound, you also can't use callback_error, which purposefully calls ffi::lua_error.

I suppose I could write more C code to call rust and actually do the longjmp?

Edit: yeah, sorry I realized later that you said exactly this in your last paragraph and I just missed it, sorry!

[jonas-schievink]

you also can't use callback_error, which purposefully calls ffi::lua_error.

Yeah, that could be made to work by indicating errors via a return value from any lua_CFunction (see my last paragraph). Might need a bunch of refactoring, though.

I suppose I could write more C code to call rust and actually do the longjmp?

I think we have the same idea. I'm imagining something like this:

int call_rust(lua_State* state) {
    int result = rust_fn(state);
    if (result == -1) lua_error(state);
    else return result;
}

[kyren]

This is especially infuriating to me, because my current build process for some things I can't talk about involves building lua 5.3 out of tree, and are in environments where it is currently impossible for me to cross compile C code, which means that I have to have some exposed method of building rlua which involves the person using it manually building some C code that should exist outside of Lua 5.3.

I know this is not a problem for normal uses, but this is ostensibly a Chucklefish library, and it is massively annoying to require compiling custom C code.

Are we SURE that triggering lonjmp from rust is 100% unsafe all the time, even if only Copy types are on the stack? It doesn't seem to be actually unsafe, only theoretically so.

[jonas-schievink]

in environments where it is currently impossible for me to cross compile C code

That's interesting, you're doing a pure-Rust project? Would be an awesome statement for the Rust ecosystem, I didn't think it's that mature already.

Are we SURE that triggering lonjmp from rust is 100% unsafe all the time, even if only Copy types are on the stack? It doesn't seem to be actually unsafe, only theoretically so.

Well, that's kind of the problem: No one really knows all details of what's unsafe when. It does sound safe in practice - C++ also makes this safe if no destructor needs to be called. Maybe we should involve the unsafe code team? It seems bad for the language (and this issue - especially the solution - proves it) to categorically make uses of longjmp UB.

[kyren]

That's interesting, you're doing a pure-Rust project? Would be an awesome statement for the Rust ecosystem, I didn't think it's that mature already.

It's not exactly pure rust, there is a top level c++ shim of varying size depending on the target (which I can't talk about), and I could absolutely put the C code there, it just seems very deeply ODD to ship with some C code and then have a mode where you compile the rust code, and then are supposed to copy paste some custom C code into the final product? The actual practical problem is that I can't use the gcc crate, because I can't necessarily cross compile with the system C compiler (though I suppose I could possibly set that up?)

Maybe it's not as big of an issue as I'm thinking?

[kyren]

So, I've had time to sleep on this issue and #19.

I'm pretty convinced that eventually I'm going to have to write a lot more wrapper code, and before I get into wrapping everything marked 'm' in the Lua API docs, I'd really like to know if I HAVE to write this in C. I'd really really really prefer not to.

I feel like maybe the people responding that this is UB are imagining something different than what I'm imagining. All we want to do is simply use rust to guard the longjmp, absolutely as close as possible to the C code that does it, with either only Copy types on the stack or better yet nothing on the stack.

I feel like, at some level, this HAS to work, right? I understand that people would be reluctant to officially say this is not UB, but is there a possibility of getting some kind of official word on this? You said maybe we should involve the unsafe code team, I think maybe that might be best? I'm still pretty new at this, I don't know exactly what that entails.. do they have something like the bat signal? :D

I think I would definitely agree that calling setjmp from rust is pretty.. weird, and possibly would just always be considered UB (that comment on IRC, "more like destructors won't run, though the double-return will confuse it"). That's NOT what we're doing though, we're only dealing with a C API that has called setjmp, and is going to longjmp on us, all we want to do is trigger the longjmp.

[jonas-schievink]

You said maybe we should involve the unsafe code team, I think maybe that might be best? I'm still pretty new at this, I don't know exactly what that entails.. do they have something like the bat signal? :D

I think that's done best by posting in this Forum category https://internals.rust-lang.org/c/language-design/unsafe-code-guidelines or possibly by opening an issue on this repo: https://github.com/nikomatsakis/rust-memory-model

I think I would definitely agree that calling setjmp from rust is pretty.. weird, and possibly would just always be considered UB (that comment on IRC, "more like destructors won't run, though the double-return will confuse it"). That's NOT what we're doing though, we're only dealing with a C API that has called setjmp, and is going to longjmp on us, all we want to do is trigger the longjmp.

This is probably right. Actually, longjmp reminds me of yield used by coroutines, which performs additional checks on in-scope borrows, so maybe something like that could apply here, too?

[kyren]

Well, that was a long interesting journey, but I think from the discussion in rust-lang/rust#48251 we can conclude that this is intended to be supported in rust going forward. It may not work with 1.24.x and possibly 1.25.x on windows without some small hacks, but that is at least considered a rust bug with a fix pending. I think we can close this bug?

[jonas-schievink]

Crisis averted!