Disallow values to cross stores #1016
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sunfishcode
reviewed
Feb 27, 2020
crates/api/src/values.rs
Outdated
| pub(crate) fn comes_from_same_store(&self, store: &Store) -> bool { | ||
| match self { | ||
| Val::FuncRef(f) => Store::same(store, f.store()), | ||
| _ => true, |
There was a problem hiding this comment.
It'd be helpful to have a brief comment here, mentioning that none of the numeric value types inherently depend on a store.
There was a problem hiding this comment.
I went ahead and opted to switch this to an exhaustive match and added a comment for integers
crates/api/src/values.rs
Outdated
| pub(crate) fn comes_from_same_store(&self, store: &Store) -> bool { | ||
| match self { | ||
| Val::FuncRef(f) => Store::same(store, f.store()), | ||
| _ => true, |
There was a problem hiding this comment.
Do we need to handle AnyRef here too, for the InternalRef case?
There was a problem hiding this comment.
Currently it technically doesn't need to because we don't really support anyref in wasmtime at the moment. I went ahead and explicitly matched it to always return false with a TODO comment about how to fix it in the future.
alexcrichton
force-pushed
the
require-same-instance
branch
from
630dbd1 to
008a3f6
Compare
Lots of internals in the wasmtime-{jit,runtime} crates are highly
unsafe, so it's up to the `wasmtime` API crate to figure out how to make
it safe. One guarantee we need to provide is that values never cross
between stores. For example you can't take a function in one store and
move it over into a different instance in a different store. This
dynamic check can't be performed at compile time and it's up to
`wasmtime` to do the check itself.
This adds a number of checks, but not all of them, to the codebase for
now. This primarily adds checks around instantiation, globals, and
tables. The main hole in this is functions, where you can pass in
arguments or return values that are not from the right store. For now
though we can't compile modules with `anyref` parameters/returns anyway,
so we should be good. Eventually when that is supported we'll need to
put the guards in place.
Closes bytecodealliance#958
alexcrichton
force-pushed
the
require-same-instance
branch
from
008a3f6 to
5051295
Compare
arkpar
pushed a commit
to paritytech/wasmtime
that referenced
this pull request
Mar 4, 2020
sunfishcode
approved these changes
Mar 9, 2020
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
May 4, 2020
Turns out this was a typo from bytecodealliance#1016!
alexcrichton
added a commit
that referenced
this pull request
May 4, 2020
Turns out this was a typo from #1016!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lots of internals in the wasmtime-{jit,runtime} crates are highly
unsafe, so it's up to the
wasmtimeAPI crate to figure out how to makeit safe. One guarantee we need to provide is that values never cross
between stores. For example you can't take a function in one store and
move it over into a different instance in a different store. This
dynamic check can't be performed at compile time and it's up to
wasmtimeto do the check itself.This adds a number of checks, but not all of them, to the codebase for
now. This primarily adds checks around instantiation, globals, and
tables. The main hole in this is functions, where you can pass in
arguments or return values that are not from the right store. For now
though we can't compile modules with
anyrefparameters/returns anyway,so we should be good. Eventually when that is supported we'll need to
put the guards in place.
Closes #958