-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
I'll also note that ownership-wise this brings core wasm in line with components, where components have |
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... |
Ok spurious failures should be fixed by #10947 |
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
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
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.
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.
This commit represents more effort to bring safety to
vm::Instance
and, eventually,ComponentInstance
. This is specifically addressing two points of safety aroundvm::Instance
:Previously ownership of this was murky where
InstanceHandle
sort of represented ownership but sort of didn't either through theInstanceHandle::clone
method. NowInstanceHandle
has a destructor for instances and no longer hasclone
, 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 usingPin<&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 theruntime_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
andget_mut
. This reflects howInstanceHandle
should in general no longer be used and insteadInstance
itself, and some pointer-to thereof, should be exclusively used.cc #10933