bytecodealliance / lucet

Lucet, the Sandboxing WebAssembly Compiler.
Apache License 2.0
4.06k stars 165 forks source link

⚠ Replace `&mut` requirement on `Vmctx` methods with dynamic checks #520

Closed acfoltzer closed 4 years ago

acfoltzer commented 4 years ago

The Vmctx interface is very tricky. We are essentially presenting the hostcall implementor with a view of the instance's resources (linear memory, globals, and embedder contexts, specifically) that can be aliased both by the host that originally called the Wasm program, as well as any callbacks into Wasm that might then reference or modify those values. That aliasing is potentially exposed when a hostcall's control flow either yields back out to the runtime's calling context, or flows across the FFI boundary back into Wasm, potentially reëmerging in nested hostcalls.

We still rely on manual safety properties for Wasm callbacks, and therefore make the implementor use unsafe in those settings. However we have been relying heavily on the borrow checker, rather than manual inspection, to ensure that instance resources are not borrowed when performing a yield, or that the heap view is not borrowed when growing the memory of the instance.

This patch changes all of the Vmctx methods to instead only require &self, which means the borrow checker no longer enforces these properties. Instead, we perform a dynamic check before any of these dangerous operations using the RefCell machinery that was already in place to facilitate "splitting the borrow" of the Vmctx. Essentially, we attempt to acquire an exclusive (mutable) borrow on all of the relevant views, so that we can terminate the instance with a BorrowError if there are any outstanding borrows at that point.

The main benefit of this change is easier integration with tools designed with Wasmtime's execution model in mind, such as wiggle. Without this change, it is difficult to imagine how to get the &mut Vmctx required to yield from within a wiggle-generated hostcall.

There are a couple downsides: