Skip to content

Crack down on mutability and ownership of vm::Instance #10943

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 3 commits into from
Jun 6, 2025

Conversation

alexcrichton
Copy link
Member

This commit represents more effort to bring safety to vm::Instance and, eventually, ComponentInstance. This is specifically addressing two points of safety around vm::Instance:

  • Previously ownership of this was murky where InstanceHandle sort of represented ownership but sort of didn't either through the InstanceHandle::clone method. Now InstanceHandle has a destructor for instances and no longer has clone, so there's one exclusive owner of an instance.

  • Previously &mut Instance was liberally passed around, but this is not sound because certain fields cannot be mutated (e.g. runtime offset information). While not a perfect solution this PR switches to using Pin<&mut Instance> everywhere instead. This prevents safe access to &mut Instance and we hand-write accessors to individual fields. Notably we omit mutable access to the runtime_info field.

This naturally involved a lot of refactoring internally, but notably this started bringing up preexisting issues around how there are locations in the codebase that simultaneously have &mut Instance and &mut StoreOpaque which is technically not sound due to being able to get back to the instance from the store. Some issues here were address by passing around indices more often such as in instance initialization and const-expr evaluation.

Note that all proxy methods on InstanceHandle are also all removed now and there's now only two: get and get_mut. This reflects how InstanceHandle should in general no longer be used and instead Instance itself, and some pointer-to thereof, should be exclusively used.

cc #10933

This commit represents more effort to bring safety to `vm::Instance`
and, eventually, `ComponentInstance`. This is specifically addressing
two points of safety around `vm::Instance`:

* Previously ownership of this was murky where `InstanceHandle` sort of
  represented ownership but sort of didn't either through the
  `InstanceHandle::clone` method. Now `InstanceHandle` has a destructor
  for instances and no longer has `clone`, so there's one exclusive
  owner of an instance.

* Previously `&mut Instance` was liberally passed around, but this is
  not sound because certain fields cannot be mutated (e.g. runtime
  offset information). While not a perfect solution this PR switches to
  using `Pin<&mut Instance>` everywhere instead. This prevents safe
  access to `&mut Instance` and we hand-write accessors to individual
  fields. Notably we omit mutable access to the `runtime_info` field.

This naturally involved a lot of refactoring internally, but notably
this started bringing up preexisting issues around how there are
locations in the codebase that simultaneously have `&mut Instance` and
`&mut StoreOpaque` which is technically not sound due to being able to
get back to the instance from the store. Some issues here were address
by passing around indices more often such as in instance initialization
and const-expr evaluation.

Note that all proxy methods on `InstanceHandle` are also all removed now
and there's now only two: `get` and `get_mut`. This reflects how
`InstanceHandle` should in general no longer be used and instead
`Instance` itself, and some pointer-to thereof, should be exclusively
used.

cc bytecodealliance#10933
@alexcrichton alexcrichton requested a review from a team as a code owner June 5, 2025 22:57
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team June 5, 2025 22:57
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

nice

@alexcrichton
Copy link
Member Author

I'll also note that ownership-wise this brings core wasm in line with components, where components have OwnedComponentInstance which is now basically the exact same thing as InstanceHandle (and I'll try to eventually get around to aligning on naming conventions too)

@alexcrichton alexcrichton enabled auto-merge June 5, 2025 23:17
@alexcrichton alexcrichton added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@alexcrichton
Copy link
Member Author

While I have no reason to believe that's a flaky test failure I'm neverthless curious. If it's flaky that's quite worrisome. If it's not flaky that's also worrisome...

@alexcrichton alexcrichton added this pull request to the merge queue Jun 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 6, 2025
@alexcrichton
Copy link
Member Author

Ok spurious failures should be fixed by #10947

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 6, 2025
@alexcrichton alexcrichton added this pull request to the merge queue Jun 6, 2025
Merged via the queue into bytecodealliance:main with commit aad93a4 Jun 6, 2025
41 checks passed
@alexcrichton alexcrichton deleted the instance-pin branch June 6, 2025 16:41
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 13, 2025
This commit is the continuation of bytecodealliance#10943 for component instances. The
allocation/vmctx infrastructure was additionally refactored to be shared for
both core and component instances since they behave the exact same way anyway.
This further enables sharing various methods like `vmctx_plus_offset` which are
pretty unsafe internally.

Like bytecodealliance#10943 this necessitated removal of `Index` implementations because
`IndexMut` is not compatible with the returned type being `Pin<&mut T>`
so they were replaced by inherent `get` and `get_mut` methods on the
component instance id type.

Closes bytecodealliance#10933
github-merge-queue bot pushed a commit that referenced this pull request Jun 14, 2025
This commit is the continuation of #10943 for component instances. The
allocation/vmctx infrastructure was additionally refactored to be shared for
both core and component instances since they behave the exact same way anyway.
This further enables sharing various methods like `vmctx_plus_offset` which are
pretty unsafe internally.

Like #10943 this necessitated removal of `Index` implementations because
`IndexMut` is not compatible with the returned type being `Pin<&mut T>`
so they were replaced by inherent `get` and `get_mut` methods on the
component instance id type.

Closes #10933
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 16, 2025
This commit fixes another issue we've discovered in the wasip3
prototyping repository about a code pattern in wasm which Miri flags as
un-sound. Specifically what happened was:

* Invocation of WebAssembly went through `VMFuncRef::array_call` which
  takes a `&self` parameter.

* Inside of WebAssembly though a `ref.func` instruction, or anything
  else that references the original exported function, will
  re-initialize the `VMFuncRef` which writes the `&self` up the stack,
  which is not sound.

Fixing this required changing the signature of `array_call` from `&self`
to `me: NonNull<VMFuncRef>`, and the signature was already `unsafe` so
this is a new unsafe contract for that signature.

In fixing this, however, it was discovered that a mistake was made
in bytecodealliance#10943 where some internal functions for re-initializing a
`VMFuncRef` relied on the previous signature of `&mut self` but that PR
switche to `&self`. This PR corrects these signatures to `Pin<&mut Self>`
and then plumbs around the necessary changes, notably causing some
refactoring in component-related bits.
github-merge-queue bot pushed a commit that referenced this pull request Jun 17, 2025
This commit fixes another issue we've discovered in the wasip3
prototyping repository about a code pattern in wasm which Miri flags as
un-sound. Specifically what happened was:

* Invocation of WebAssembly went through `VMFuncRef::array_call` which
  takes a `&self` parameter.

* Inside of WebAssembly though a `ref.func` instruction, or anything
  else that references the original exported function, will
  re-initialize the `VMFuncRef` which writes the `&self` up the stack,
  which is not sound.

Fixing this required changing the signature of `array_call` from `&self`
to `me: NonNull<VMFuncRef>`, and the signature was already `unsafe` so
this is a new unsafe contract for that signature.

In fixing this, however, it was discovered that a mistake was made
in #10943 where some internal functions for re-initializing a
`VMFuncRef` relied on the previous signature of `&mut self` but that PR
switche to `&self`. This PR corrects these signatures to `Pin<&mut Self>`
and then plumbs around the necessary changes, notably causing some
refactoring in component-related bits.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants