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

Fix cyclic ref in WasmerEnv #2327

Merged
merged 11 commits into from
May 29, 2021
26 changes: 26 additions & 0 deletions lib/api/src/exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@ impl Exports {
}
}

/// Like `get_with_generics` but with a WeakReference to the `InstanceRef` internally.
/// This is useful for passing data into `WasmerEnv`, for example.
pub fn get_with_generics_weak<'a, T, Args, Rets>(&'a self, name: &str) -> Result<T, ExportError>
where
Args: WasmTypeList,
Rets: WasmTypeList,
T: ExportableWithGenerics<'a, Args, Rets>,
{
let mut out: T = self.get_with_generics(name)?;
out.into_weak_instance_ref();
Ok(out)
}

/// Get an export as an `Extern`.
pub fn get_extern(&self, name: &str) -> Option<&Extern> {
self.map.get(name)
Expand Down Expand Up @@ -294,6 +307,11 @@ pub trait Exportable<'a>: Sized {
///
/// [`Instance`]: crate::Instance
fn get_self_from_extern(_extern: &'a Extern) -> Result<&'a Self, ExportError>;

/// Convert the extern internally to hold a weak reference to the `InstanceRef`.
/// This is useful for preventing cycles, for example for data stored in a
/// type implementing `WasmerEnv`.
fn into_weak_instance_ref(&mut self);
}

/// A trait for accessing exports (like [`Exportable`]) but it takes generic
Expand All @@ -302,6 +320,10 @@ pub trait Exportable<'a>: Sized {
pub trait ExportableWithGenerics<'a, Args: WasmTypeList, Rets: WasmTypeList>: Sized {
/// Get an export with the given generics.
fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result<Self, ExportError>;
/// Convert the extern internally to hold a weak reference to the `InstanceRef`.
/// This is useful for preventing cycles, for example for data stored in a
/// type implementing `WasmerEnv`.
fn into_weak_instance_ref(&mut self);
}

/// We implement it for all concrete [`Exportable`] types (that are `Clone`)
Expand All @@ -310,4 +332,8 @@ impl<'a, T: Exportable<'a> + Clone + 'static> ExportableWithGenerics<'a, (), ()>
fn get_self_from_extern_with_generics(_extern: &'a Extern) -> Result<Self, ExportError> {
T::get_self_from_extern(_extern).map(|i| i.clone())
}

fn into_weak_instance_ref(&mut self) {
<Self as Exportable>::into_weak_instance_ref(self);
}
}
19 changes: 19 additions & 0 deletions lib/api/src/externals/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,17 @@ impl Function {
fn closures_unsupported_panic() -> ! {
unimplemented!("Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840")
}

/// Check if the function holds a strong `InstanceRef`.
/// None means there's no `InstanceRef`, strong or weak.
// TODO: maybe feature gate this, we only need it for tests...
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
pub fn is_strong_instance_ref(&self) -> Option<bool> {
self.exported
.vm_function
.instance_ref
.as_ref()
.map(|v| v.is_strong())
}
}

impl<'a> Exportable<'a> for Function {
Expand All @@ -725,6 +736,14 @@ impl<'a> Exportable<'a> for Function {
_ => Err(ExportError::IncompatibleType),
}
}

fn into_weak_instance_ref(&mut self) {
self.exported
.vm_function
.instance_ref
.as_mut()
.map(|v| v.into_weak());
}
}

impl fmt::Debug for Function {
Expand Down
11 changes: 11 additions & 0 deletions lib/api/src/externals/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,13 @@ impl Global {
pub fn same(&self, other: &Self) -> bool {
Arc::ptr_eq(&self.vm_global.from, &other.vm_global.from)
}

/// Check if the global holds a strong `InstanceRef`.
/// None means there's no `InstanceRef`, strong or weak.
// TODO: maybe feature gate this, we only need it for tests...
Copy link
Member

Choose a reason for hiding this comment

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

This is only used in tests. As such I would expect to only be visible there (can we delete it from here and move the logic somewhere else?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it can't be, these accessors access private info. It has to be a method on the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One solution to this would be to define a trait with these methods on it and export it in like wasmer::test and mark that module as #[hidden]so it doesn't show up in docs. Then these methods will only be available if you import wasmer::test::SomeTrait

pub fn is_strong_instance_ref(&self) -> Option<bool> {
self.vm_global.instance_ref.as_ref().map(|v| v.is_strong())
}
}

impl fmt::Debug for Global {
Expand All @@ -231,4 +238,8 @@ impl<'a> Exportable<'a> for Global {
_ => Err(ExportError::IncompatibleType),
}
}

fn into_weak_instance_ref(&mut self) {
self.vm_global.instance_ref.as_mut().map(|v| v.into_weak());
}
}
11 changes: 11 additions & 0 deletions lib/api/src/externals/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,13 @@ impl Memory {
pub fn same(&self, other: &Self) -> bool {
Arc::ptr_eq(&self.vm_memory.from, &other.vm_memory.from)
}

/// Check if the memory holds a strong `InstanceRef`.
/// None means there's no `InstanceRef`, strong or weak.
// TODO: maybe feature gate this, we only need it for tests...
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
pub fn is_strong_instance_ref(&self) -> Option<bool> {
self.vm_memory.instance_ref.as_ref().map(|v| v.is_strong())
}
}

impl<'a> Exportable<'a> for Memory {
Expand All @@ -261,4 +268,8 @@ impl<'a> Exportable<'a> for Memory {
_ => Err(ExportError::IncompatibleType),
}
}

fn into_weak_instance_ref(&mut self) {
self.vm_memory.instance_ref.as_mut().map(|v| v.into_weak());
}
}
9 changes: 9 additions & 0 deletions lib/api/src/externals/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ impl<'a> Exportable<'a> for Extern {
// Since this is already an extern, we can just return it.
Ok(_extern)
}

fn into_weak_instance_ref(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super fan of this function name. I think we can just have into_weak (the API user should not care about how is implemented?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the reason for the extra precision is due to overlap in ownership. It's not really a weak version of the thing it's being called on. That thing can still keep things alive. The only thing being made weak here is an internal InstanceRef. So I do think to that extent, it does matter, it's generally not something users should have to interact with unless they're doing low level things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this function again, we probably need to make it unsafe because you can probably get use after free with it. I'll investigate this with a test and maybe update it

match self {
Self::Function(f) => f.into_weak_instance_ref(),
Self::Global(g) => g.into_weak_instance_ref(),
Self::Memory(m) => m.into_weak_instance_ref(),
Self::Table(t) => t.into_weak_instance_ref(),
}
}
}

impl StoreObject for Extern {
Expand Down
11 changes: 11 additions & 0 deletions lib/api/src/externals/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,13 @@ impl Table {
pub fn same(&self, other: &Self) -> bool {
Arc::ptr_eq(&self.vm_table.from, &other.vm_table.from)
}

/// Check if the table holds a strong `InstanceRef`.
/// None means there's no `InstanceRef`, strong or weak.
// TODO: maybe feature gate this, we only need it for tests...
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
pub fn is_strong_instance_ref(&self) -> Option<bool> {
self.vm_table.instance_ref.as_ref().map(|v| v.is_strong())
}
}

impl<'a> Exportable<'a> for Table {
Expand All @@ -159,4 +166,8 @@ impl<'a> Exportable<'a> for Table {
_ => Err(ExportError::IncompatibleType),
}
}

fn into_weak_instance_ref(&mut self) {
self.vm_table.instance_ref.as_mut().map(|v| v.into_weak());
}
}
10 changes: 10 additions & 0 deletions lib/api/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,12 @@ macro_rules! impl_native_traits {
}

}
/// Check if the function holds a strong `InstanceRef`.
/// None means there's no `InstanceRef`, strong or weak.
// TODO: maybe feature gate this, we only need it for tests...
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
pub fn is_strong_instance_ref(&self) -> Option<bool> {
self.exported.vm_function.instance_ref.as_ref().map(|v| v.is_strong())
}
}

#[allow(unused_parens)]
Expand All @@ -208,6 +214,10 @@ macro_rules! impl_native_traits {
use crate::exports::Exportable;
crate::Function::get_self_from_extern(_extern)?.native().map_err(|_| crate::exports::ExportError::IncompatibleType)
}

fn into_weak_instance_ref(&mut self) {
self.exported.vm_function.instance_ref.as_mut().map(|v| v.into_weak());
}
}
};
}
Expand Down
Loading