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

What to do about cross-Store calls? #958

Closed
alexcrichton opened this issue Feb 19, 2020 · 2 comments · Fixed by #1016
Closed

What to do about cross-Store calls? #958

alexcrichton opened this issue Feb 19, 2020 · 2 comments · Fixed by #1016
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself

Comments

@alexcrichton
Copy link
Member

In reviewing some code today I started wondering what would happen if you started mixing Store values and instances together. For example you can create two Instance objects in two Store objects, what would happen when they're linked?

The concrete things I know of today we have to worry about are:

  • Mainly the Compiler shared state in a Store. This notably contains two fields:
    • signatures is a Store-local registry of all known wasm signatures, mapping them to an index for signature checks during call_indirect. This is actually memory unsafe today because if you mix two Store objects then two different signatures could get the same shared index, meaning call_indirect could call the wrong thing.
    • trap_registry is also a Store-local registry of information about traps. While I don't think this is related to memory safety it does mean that if you're calling code in one instance but started in another the trap in the second instance won't be resolved correctly and we'll get the wrong trap information out of it.
  • There's also maybe issues with the cache but I suspect not, it's not shared state across instances in a Store that I've looked at too too deeply.

I started implementing a fix where we'd simply reject linking instances together if they come from two different Store values, but this is also a problem with any Val::FuncRef getting stored in a table across instances. Especially with reference types this gets really hairy to guard, so I don't think it'll be easy to simply block access at all entry points.

The only fix I can think of is to have a truly global map for all this, but it feels bad to have a truly global ever-expanding map that's never deallocated from. I think we'll want to figure out a way to remove items from the map at least when a Store is dropped (maybe sooner?). In any case wanted to make sure there was an open issue for this!

@pchickey
Copy link
Contributor

pchickey commented Feb 19, 2020

Maybe, with the introduction of FuncRef, we need a layer in the hierarchy above Store? Some programs may want to have just one of these globally, but in our use case we may want to map one of those per request. In general, for both resource consumption and side channels, we want to eliminate global state like the map you describe.

@alexcrichton
Copy link
Member Author

Ok so from our discussion on the call today, the answer here appears to be "disallow this", so we'll need to look into guarding all call-sites and such to ensure cross-store objects cannot be mixed at runtime.

@alexcrichton alexcrichton added the wasmtime:api Related to the API of the `wasmtime` crate itself label Feb 27, 2020
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Feb 27, 2020
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 added a commit to alexcrichton/wasmtime that referenced this issue Mar 2, 2020
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 added a commit that referenced this issue Mar 10, 2020
* Disallow values to cross stores

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 #958

* Clarify how values test they come from stores

* Allow null anyref to initialize tables
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 a pull request may close this issue.

2 participants