Skip to content
This repository was archived by the owner on Mar 24, 2022. It is now read-only.

Hostcall macros and Vmctx rework#157

Merged
acfoltzer merged 26 commits intomasterfrom
acf/terrarium-integration
May 1, 2019
Merged

Hostcall macros and Vmctx rework#157
acfoltzer merged 26 commits intomasterfrom
acf/terrarium-integration

Conversation

@acfoltzer
Copy link
Contributor

This PR includes a few things developed while updating Terrarium to use newer versions of Lucet.

Termination and unwinding

We now use a combination of panic! and catch_unwind to ensure that any resources that were held at the time an instance is terminated (such as Mutexes or Files) are properly disposed of.

Note that so far, this only works for the case where we are in at most one hostcall context. If we have a stack with guest -> host -> guest -> host, the first host segment will not be properly unwound.

We believe this is a reasonable limitation at this point, but more work will be required to support unwinding across guest stacks.

lucet_hostcall_terminate!

This replaces Vmctx::terminate(), and is implemented in terms of panic!. As before, it allows the user to return an Any value for the termination details. It must only be used within the scope of...

lucet_hostcalls!

This is a macro for defining hostcalls in Rust. In particular, it uses catch_unwind to make sure that panics raised by lucet_hostcall_terminate! are properly caught, with the instance shutting down in a terminated state.

Vmctx interface rework

Because so many resources are accessed through Vmctx, practical applications quickly ran into limitations with borrow-checking. In particular, holding a mutable reference to the heap meant that one could not hold even an immutable reference to an embedder context, which is a big problem if one needed to copy data from one to the other. Even if Vmctx's resources were accessed via fields, allowing split borrows, we would still have similar borrowing issues if multiple embedder contexts are used.

This PR uses RefCells to instead enforce borrowing rules dynamically. In practice, the innocuous situations described above will work fine, but if the borrowing rules get violated, the instance will terminate using the machinery described above.

The Vmctx heap, globals, and get_embed_ctx interfaces are now all subject to this dynamic borrowing. The latter is now a bit awkward to access from outside of a hostcall, as it returns an Option<Result<Ref<T>, BorrowError>>, so maybe we'll want more lucet_runtime::Error variants down the road.

WASI stdio default

We're required to provide working file descriptors for stdio, but we now default to providing /dev/null, with the option to inherit stdio. When we have a chance to write a properly Rusty system for managing WASI capabilities, we should turn these into virtual handles that mimic /dev/null's behavior, but currently we must have a real file descriptor there.

The return of --reserved-size for lucetc

Overrides --{min,max}-reserved-size if set, making them equal.

When we clean up these interfaces, it'd be nice to make these pseudo files rather than opening an
actual file decriptor, so there are no system calls involved in creating a new WASI context.
This makes it possible to properly unwind within hostcalls, though panics that travel across the
host/guest boundary will still not behave properly due to stack layout.
This makes almost all of the `Vmctx` methods into `&self`, so hostcalls won't have to go through as
many contortions when using multiple parts of the context. To report dynamic borrowing errors, there
is a new `TerminationDetails` variant.
@tyler
Copy link
Member

tyler commented Apr 26, 2019

Amazing work.

Have you looked at how much overhead the hostcall wrapping adds at runtime to simple hostcalls?

pchickey
pchickey previously approved these changes Apr 26, 2019
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work on this! Really like how it turned out.

@jedisct1
Copy link
Contributor

👍

pchickey
pchickey previously approved these changes Apr 27, 2019
Also adds a privileged type for terminating from C
Copy link
Contributor

@awortman-fastly awortman-fastly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how this reads 🎉

This patch renames these fields to emphasize that they are views into memory managed elsewhere,
rather than a normal Rust-allocated structure. Also adds comments whenever we use `Box::leak()` to
avoid normal Rust deallocation.
…ration

Adapted new hostcalls to new interfaces
/// ```ignore
/// #[$attr1]
/// #[$attr2]
/// ... // any number of attributes are supported; in most cases you will want `#[no_mangle]`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a case where would not want #[no_mangle]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful when we're defining hostcalls for a MockModuleBuilder, so hostcalls for different tests don't interfere with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, these aren't technically hostcalls, but are the fake guest functions that go into the module

Copy link
Contributor

@jedisct1 jedisct1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement, and the solution is very clean.

No apparent regression, even on macOS.

Great!

@acfoltzer acfoltzer merged commit f30cb57 into master May 1, 2019
@acfoltzer acfoltzer deleted the acf/terrarium-integration branch July 11, 2019 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants