Skip to content

Contextify #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 13, 2018
Merged

Contextify #98

merged 7 commits into from
Nov 13, 2018

Conversation

kyren
Copy link
Contributor

@kyren kyren commented Oct 29, 2018

Fixes #97 and the only current API panic condition

HUGE API change, the entire previous API has been moved to a context callback to take advantage of generative lifetimes. This both fixed a soundness but and removes an API wart, but requires callbacks to use.

kyren added 6 commits October 29, 2018 21:11
As a side effect, would fix #97, which is the actual driving motivation behind
this change.

Adds compiletest_rs tests for context lifetime invariance, and all the tests and
compiletest_rs tests pass, but the documentation and doctests are *all* screwed
up.

There are some outstanding API design questions, but the strategy SEEMS to
actually work.
@kyren
Copy link
Contributor Author

kyren commented Oct 30, 2018

There's one thing that was possible before this change that will no longer be possible, and that's mixing outer handles with inner handles from a Scope::create_function callback. This works on master but there is no equivalent in the contextify branch:

let lua = Lua::new();
let table = lua.create_table().unwrap();
lua.scope(|scope| {
    scope
        .create_function_mut(|lua, ()| {
            // this will fail, `lua` and `table` unique lifetimes are different
            lua.globals().set("t", table.clone()).unwrap();
            Ok(())
        }).unwrap()
        .call::<_, ()>(())
        .unwrap();
});
assert_eq!(
    lua.globals()
        .get::<_, Table>("t")
        .unwrap()
        .get::<_, String>("a")
        .unwrap(),
    "b"
);

Somewhat confusingly, you CAN still do this on contextify (and there is a test for it):

Lua::new().context(|lua| {
    let table = lua.create_table().unwrap();
    lua.scope(|scope| {
        scope
            .create_function_mut(|_, ()| {
                // Fine, because only one lifetime is involved
                table.set("a", "b").unwrap();
                Ok(())
            }).unwrap()
            .call::<_, ()>(())
            .unwrap();
    });
    assert_eq!(table.get::<_, String>("a").unwrap(), "b");
});

which is to say, that you can still ofc access outer variables inside a scope callback like you would expect, there's just no way to say that the two 'lua lifetimes are compatible.

You COULD imagine a Context::scope that actually just re-uses the outer 'lua lifetime on the inside, but this would not be sound: This would allow Scope::create_function to capture Context<'lua> from its arguments inside itself, which like everywhere else in rlua is not safe to allow.

The reason this is not safe has to do with coroutines. Context is now actually fairly simple, it is just a ffi::lua_State pointer and some marker types, and this is actually great because it is conceptually much simpler than it was before. Context contains a state pointer, and then marker types with a lifetime that is guaranteed not to outlive the validity of the state pointer it holds.

However, if we used the outer 'lua lifetime in the inner scoped functions, this would no longer be true! It would be possible for a script to create a coroutine with its own state pointer, use that coroutine to call the scoped function (which will be passed the state pointer from the coroutine), then garbage collect the coroutine, then call the scoped function again. If the scoped function could capture Contexts from its arguments, it would be capturing the invalidated coroutine lua_State pointer and that would lead to UB.

This situation would actually be basically the same if I had fixed the unsoundness in #97 in any of the other ways that I mentioned, this is not unique to this particular PR. The root unsoundness problem is how loose the 'lua lifetime was which is also what enables this pattern, so fixing one almost has to break the other.

The only alternative that I can come up, which is also potentially conceptually simpler, is to have a single Context for the entire duration of Lua that is always the main state. If I did that, it would never actually be harmful to capture arguments, because the lua_State pointer lives just as long as Lua does (this was mentioned as one of the possibilities to fix #97 as well). It's a bit gross in that it makes Lua stack usage kind of weird, and it is never a good idea to capture arguments outside of a call to Lua::scope (because of gc concerns) so that would probably still need to be disallowed, but it would allow this one pattern.

Right now, I don't have a huge motivation to do this. I think this is an extreme corner case, and it is almost always a better idea to use Function::bind to hold onto outer handle values in a callback. In fact the entire reason I added Function::bind was to do this sort of thing -- those kind of function binds will play nicely with the Lua garbage collector, whereas any solution that allows you to capture handles will never play nicely with the gc. It's only in scoped functions where this isn't as big of a deal since the function has a defined finite lifetime anyway, but even then using Function::bind instead might allow the values to be garbage collected sooner.

If anybody disagrees with this and thinks that the pattern above is super useful to them, please let me know. If you think that the alternative lifetime definition makes Scope easier to understand, and you think making that change would be worth it, let me know. Right now I'm I guess mildly against the idea, but I'm willing to discuss it. I might merge this PR in the next week or so if I can't come up with any additional problems, but even after merging I'm still at least somewhat open to the idea of simplifying the way Context works, I just need to know if somebody thinks that's important.

@kyren
Copy link
Contributor Author

kyren commented Nov 13, 2018

I'm going to merge this, I haven't been able to come up with any better ideas and haven't thought of any terrible problems with it.

This still needs more work w.r.t. documentation etc, but it can at least sit in master while I iterate on it some more.

@kyren kyren merged commit 4e4d65e into master Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unbounded callback lifetimes cause rlua to be unsound
1 participant