Skip to content

Commit

Permalink
Add an assertion that a HostFunc's store agrees on engines
Browse files Browse the repository at this point in the history
This commit adds an assertion which was previously forgotten when
inserting a `HostFunc` into a `Store`. This can happen when a `Linker`
is defined with one engine but it's used to interoperate with a store
defined within a different engine.

A function contains type information that's only valid relative to the
engine that it was defined within. This means that if a function is used
within a different engine then type information may look valid when in
fact it is not. For example it's otherwise possible to insert a function
into an engine with one type and call it in a different engine with a
different type.

Similar to how `Store` misuse is a panic throughout `wasmtime`'s API
this commit also turns this behavior into panic, so there's no API
impact. Documentation has been updated accordingly to indicate that
various functions on `Linker` will panic if a `store` is provided that's
connected to a different `Engine`.
  • Loading branch information
alexcrichton committed Sep 8, 2021
1 parent 8ebaaf9 commit bcb1dc9
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 9 deletions.
8 changes: 8 additions & 0 deletions crates/wasmtime/src/func.rs
Expand Up @@ -1949,6 +1949,14 @@ impl HostFunc {
}

unsafe fn register_trampoline(&self, store: &mut StoreOpaque) {
// This assert is required to ensure that we can indeed safely insert
// `self` into the `store` provided, otherwise the type information we
// have listed won't e correct. This is possible to hit with the public
// API of Wasmtime, and should be documented in relevant functions.
assert!(
Engine::same(&self.engine, store.engine()),
"cannot use a store with a different engine than a linker was created with",
);
let idx = self.export.anyfunc.as_ref().type_index;
store.register_host_trampoline(idx, self.trampoline);
}
Expand Down
4 changes: 3 additions & 1 deletion crates/wasmtime/src/instance.rs
Expand Up @@ -953,7 +953,9 @@ impl<T> InstancePre<T> {
/// # Panics
///
/// Panics if any import closed over by this [`InstancePre`] isn't owned by
/// `store`, or if `store` has async support enabled.
/// `store`, or if `store` has async support enabled. Additionally this
/// function will panic if the `store` provided comes from a different
/// [`Engine`] than the [`InstancePre`] originally came from.
pub fn instantiate(&self, mut store: impl AsContextMut<Data = T>) -> Result<Instance> {
// For the unsafety here the typecheck happened at creation time of this
// structure and then othrewise the `T` of `InstancePre<T>` connects any
Expand Down
64 changes: 57 additions & 7 deletions crates/wasmtime/src/linker.rs
Expand Up @@ -70,6 +70,15 @@ use std::sync::Arc;
/// point only the [`Store`] that owns the [`Global`] can be used to instantiate
/// modules.
///
/// ## Multiple `Engine`s
///
/// The [`Linker`] type is not compatible with usage between multiple [`Engine`]
/// values. An [`Engine`] is provided when a [`Linker`] is created and only
/// stores and items which originate from that [`Engine`] can be used with this
/// [`Linker`]. If more than one [`Engine`] is used with a [`Linker`] then that
/// may cause a panic at runtime, similar to how if a [`Func`] is used with the
/// wrong [`Store`] that can also panic at runtime.
///
/// [`Store`]: crate::Store
/// [`Global`]: crate::Global
pub struct Linker<T> {
Expand Down Expand Up @@ -150,6 +159,11 @@ macro_rules! generate_wrap_async_func {

impl<T> Linker<T> {
/// Creates a new [`Linker`].
///
/// The linker will define functions within the context of the `engine`
/// provided and can only instantiate modules for a [`Store`] that is also
/// defined within the same [`Engine`]. Usage of stores with different
/// [`Engine`]s may cause a panic when used with this [`Linker`].
pub fn new(engine: &Engine) -> Linker<T> {
Linker {
engine: engine.clone(),
Expand Down Expand Up @@ -236,9 +250,6 @@ impl<T> Linker<T> {
/// of the same type as the `item` provided and if shadowing is disallowed.
/// For more information see the documentation on [`Linker`].
///
/// Also returns an error if `item` comes from a different store than this
/// [`Linker`] was created with.
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -417,6 +428,11 @@ impl<T> Linker<T> {
/// for each export is `module_name`, and the name for each export is the
/// name in the instance itself.
///
/// Note that when this API is used the [`Linker`] is no longer compatible
/// with multi-[`Store` ] instantiation because the items defined within
/// this store will belong to the `store` provided, and only the `store`
/// provided.
///
/// # Errors
///
/// Returns an error if the any item is redefined twice in this linker (for
Expand Down Expand Up @@ -505,7 +521,8 @@ impl<T> Linker<T> {
/// # Panics
///
/// Panics if any item used to instantiate the provided [`Module`] is not
/// owned by `store`.
/// owned by `store`, or if the `store` provided comes from a different
/// [`Engine`] than this [`Linker`].
///
/// # Examples
///
Expand Down Expand Up @@ -602,6 +619,15 @@ impl<T> Linker<T> {
{
// NB: this is intended to function the same as `Linker::module_async`,
// they should be kept in sync.

// This assert isn't strictly necessary since it'll bottom out in the
// `HostFunc::to_func` method anyway. This is placed earlier for this
// function though to prevent the functions created here from delaying
// the panic until they're called.
assert!(
Engine::same(&self.engine, store.as_context().engine()),
"different engines for this linker and the store provided"
);
match ModuleKind::categorize(module)? {
ModuleKind::Command => {
self.command(
Expand Down Expand Up @@ -672,6 +698,10 @@ impl<T> Linker<T> {
{
// NB: this is intended to function the same as `Linker::module`, they
// should be kept in sync.
assert!(
Engine::same(&self.engine, store.as_context().engine()),
"different engines for this linker and the store provided"
);
match ModuleKind::categorize(module)? {
ModuleKind::Command => self.command(
store,
Expand Down Expand Up @@ -899,7 +929,8 @@ impl<T> Linker<T> {
/// # Panics
///
/// Panics if any item used to instantiate `module` is not owned by
/// `store`.
/// `store`. Additionally this will panic if the [`Engine`] that the `store`
/// belongs to is different than this [`Linker`].
///
/// # Examples
///
Expand Down Expand Up @@ -958,7 +989,9 @@ impl<T> Linker<T> {
/// # Panics
///
/// This method will panic if any item defined in this linker used by
/// `module` is not owned by `store`.
/// `module` is not owned by `store`. Additionally this will panic if the
/// [`Engine`] that the `store` belongs to is different than this
/// [`Linker`].
///
/// # Examples
///
Expand Down Expand Up @@ -1027,6 +1060,11 @@ impl<T> Linker<T> {
///
/// Note that multiple `Extern` items may be defined for the same
/// module/name pair.
///
/// # Panics
///
/// This function will panic if the `store` provided does not come from the
/// same [`Engine`] that this linker was created with.
pub fn iter<'a: 'p, 'p>(
&'a self,
mut store: impl AsContextMut<Data = T> + 'p,
Expand All @@ -1047,6 +1085,11 @@ impl<T> Linker<T> {
///
/// Returns `None` if this name was not previously defined in this
/// [`Linker`].
///
/// # Panics
///
/// This function will panic if the `store` provided does not come from the
/// same [`Engine`] that this linker was created with.
pub fn get(
&self,
mut store: impl AsContextMut<Data = T>,
Expand All @@ -1073,6 +1116,11 @@ impl<T> Linker<T> {
/// provided.
///
/// Returns `None` if no match was found.
///
/// # Panics
///
/// This function will panic if the `store` provided does not come from the
/// same [`Engine`] that this linker was created with.
pub fn get_by_import(
&self,
mut store: impl AsContextMut<Data = T>,
Expand Down Expand Up @@ -1126,7 +1174,9 @@ impl<T> Linker<T> {
///
/// # Panics
///
/// Panics if the default function found is not owned by `store`.
/// Panics if the default function found is not owned by `store`. This
/// function will also panic if the `store` provided does not come from the
/// same [`Engine`] that this linker was created with.
pub fn get_default(
&self,
mut store: impl AsContextMut<Data = T>,
Expand Down
2 changes: 1 addition & 1 deletion tests/all/linker.rs
Expand Up @@ -256,7 +256,7 @@ fn get_host_function() -> Result<()> {

let mut linker = Linker::new(&engine);
linker.func_wrap("mod", "f1", || {})?;
let mut store = Store::<()>::default();
let mut store = Store::new(&engine, ());
assert!(linker
.get_by_import(&mut store, &module.imports().nth(0).unwrap())
.is_some());
Expand Down

0 comments on commit bcb1dc9

Please sign in to comment.