Skip to content
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

Refactor vm::Instance for better Store safety #9565

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Nov 5, 2024

At various times, for example in libcalls, we need to go from a raw vmctx pointer that Wasm gave us to both an &mut Instance and an &mut Store{Opaque,Context}. This is a pretty fundamental unsafe aspect of implementing a language runtimne in Rust, but we can tighten things up a bit and make them a little safer than they currently are. This commit is such an attempt and is notably tackling the issue of creating multiple store borrows after we have an instance borrow.

This commit makes the following changes:

  • Instance doesn't expose a method to get the raw *mut dyn VMStore pointer or otherwise create a store borrow.

  • You cannot construct an Instance directly from a vmctx pointer.

  • There is now an InstanceAndStore type that represents unique access to both an Instance and a Store.

  • You can (with unsafe) create an InstanceAndStore from a raw vmctx pointer. This generally only happens inside a couple "bottlenecks", not all throughout the codebase, like the shared plumbing code for libcalls.

  • The InstanceAndStore can be unpacked into a mutable borrow of an Instance and a mutable borrow of a Store. This unpacking takes holds a mutable borrow of the original InstanceAndStore, so double borrows of the store are no longer a concern, so long as you don't use unsafe to create new InstanceAndStores for the same vmctx.

  • All Instance methods and functions that previously would unsafely turn the Instance's internal store pointer into a store borrow to perform some action now take a store argument, and this store is threaded around as necessary.

Altogether, I feel that we've ended up with an architecture that is a safety improvement over where we were previously, and will help us properly avoid Rust UB.

For what its worth, I do not think this is the end state. I foresee at least two additional follow ups that I unfortunately do not currently have time for:

  1. Create ComponentInstanceAndStore, which is the same thing that this commit did but for components. I started on this but ran out of fuel while trying to update macros for all the component shims and transcoders.

  2. Stop dealing with &mut vm::Tables and &mut vm::Globals and all that in the runtime. Instead just deal with indices, similar to how things are structured in the host API level. This refactoring was not previously possible due to (1) the wasmtime versus wasmtime-runtime crate split and (2) the lack of StoreOpaques threaded through the VM internals. The first blocker was addressed a few months ago, this commit removes the second blocker. This will still be a pretty large refactoring though, but I think ultimately will be worth it.

@fitzgen fitzgen requested a review from a team as a code owner November 5, 2024 20:00
@fitzgen fitzgen requested review from alexcrichton and removed request for a team November 5, 2024 20:00
At various times, for example in libcalls, we need to go from a raw `vmctx`
pointer that Wasm gave us to both an `&mut Instance` and an `&mut
Store{Opaque,Context}`. This is a pretty fundamental unsafe aspect of
implementing a language runtimne in Rust, but we can tighten things up a bit and
make them a little safer than they currently are. This commit is such an
attempt and is notably tackling the issue of creating multiple store borrows
after we have an instance borrow.

This commit makes the following changes:

* `Instance` doesn't expose a method to get the raw `*mut dyn VMStore` pointer
  or otherwise create a store borrow.

* You cannot construct an `Instance` directly from a `vmctx` pointer.

* There is now an `InstanceAndStore` type that represents unique access to both
  an `Instance` and a `Store`.

* You can (with `unsafe`) create an `InstanceAndStore` from a raw `vmctx`
  pointer. This generally only happens inside a couple "bottlenecks", not all
  throughout the codebase, like the shared plumbing code for libcalls.

* The `InstanceAndStore` can be unpacked into a mutable borrow of an `Instance`
  and a mutable borrow of a `Store`. This unpacking takes holds a mutable borrow
  of the original `InstanceAndStore`, so double borrows of the store are no
  longer a concern, so long as you don't use `unsafe` to create new
  `InstanceAndStore`s for the same `vmctx`.

* All `Instance` methods and functions that previously would unsafely turn the
  `Instance`'s internal store pointer into a store borrow to perform some action
  now take a store argument, and this store is threaded around as necessary.

Altogether, I feel that we've ended up with an architecture that is a safety
improvement over where we were previously, and will help us properly avoid Rust
UB.

For what its worth, I do not think this is the end state. I foresee at least two
additional follow ups that I unfortunately do not currently have time for:

1. Create `ComponentInstanceAndStore`, which is the same thing that this commit
   did but for components. I started on this but ran out of fuel while trying to
   update macros for all the component shims and transcoders.

2. Stop dealing with `&mut vm::Table`s and `&mut vm::Global`s and all that in
   the runtime. Instead just deal with indices, similar to how things are
   structured in the host API level. This refactoring was not previously possible
   due to (1) the `wasmtime` versus `wasmtime-runtime` crate split and (2) the lack
   of `StoreOpaque`s threaded through the VM internals. The first blocker was
   addressed a few months ago, this commit removes the second blocker. This will
   still be a pretty large refactoring though, but I think ultimately will be worth
   it.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great 👍

One comment about the 'static though on the dyn VMStore type though.

crates/wasmtime/src/runtime/vm/instance.rs Outdated Show resolved Hide resolved
@fitzgen fitzgen added this pull request to the merge queue Nov 5, 2024
Merged via the queue into bytecodealliance:main with commit 8818b25 Nov 5, 2024
40 checks passed
@fitzgen fitzgen deleted the instance-and-store-split branch November 5, 2024 22:08
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.

2 participants