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.

Sign up for GitHub

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 memory leak in host function envs #1865

Merged
merged 15 commits into from
Dec 15, 2020
Merged

Fix memory leak in host function envs #1865

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1eaea6e
Fix memory leak in host function envs
Dec 3, 2020
e43d9d2
Experimental: clone host envs during construction
Dec 5, 2020
4ec8c9e
Fix issue with dynamic host env
Dec 5, 2020
ca4736c
Fix tests in C API and deprecated API
Dec 5, 2020
476e8d1
Merge branch 'master' into fix/host-func-env-memory-leak
MarkMcCaskey Dec 7, 2020
6e95c50
Address feedback
Dec 8, 2020
99e0357
Merge branch 'master' into fix/host-func-env-memory-leak
Dec 9, 2020
be3c381
Make `NativeFunc` `Clone`
Dec 9, 2020
ec4d6e6
Address feedback, make code more explicit about the different cases
Dec 10, 2020
bd01d6a
Address feedback
Dec 11, 2020
f798f34
Merge branch 'master' into fix/host-func-env-memory-leak
Dec 12, 2020
27f7e7e
Remove variant from `ImportFunctionEnv`
Dec 15, 2020
62d15fa
Merge branch 'master' into fix/host-func-env-memory-leak
Dec 15, 2020
6a21169
Fix up tests, make `WasmerEnv`: `Send`
Dec 15, 2020
8081aae
Add `WasmerEnv: Sync`
Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/imports_function_env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
// possible to know the size of the `Env` at compile time (i.e it has to
// implement the `Sized` trait) and that it implement the `WasmerEnv` trait.
// We derive a default implementation of `WasmerEnv` here.
#[derive(WasmerEnv)]
#[derive(WasmerEnv, Clone)]
struct Env {
counter: Arc<RefCell<i32>>,
}
Expand Down
33 changes: 17 additions & 16 deletions lib/api/src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ impl From<ExportError> for HostEnvInitError {
/// ```
/// use wasmer::{WasmerEnv, LazyInit, Memory, NativeFunc};
///
/// #[derive(WasmerEnv)]
/// #[derive(WasmerEnv, Clone)]
/// pub struct MyEnvWithNoInstanceData {
/// non_instance_data: u8,
/// }
///
/// #[derive(WasmerEnv)]
/// #[derive(WasmerEnv, Clone)]
/// pub struct MyEnvWithInstanceData {
/// non_instance_data: u8,
/// #[wasmer(export)]
Expand All @@ -54,6 +54,7 @@ impl From<ExportError> for HostEnvInitError {
/// This trait can also be implemented manually:
/// ```
/// # use wasmer::{WasmerEnv, LazyInit, Memory, Instance, HostEnvInitError};
/// #[derive(Clone)]
/// pub struct MyEnv {
/// memory: LazyInit<Memory>,
/// }
Expand All @@ -66,7 +67,7 @@ impl From<ExportError> for HostEnvInitError {
/// }
/// }
/// ```
pub trait WasmerEnv {
pub trait WasmerEnv: Clone {
/// The function that Wasmer will call on your type to let it finish
/// setting up the environment with data from the `Instance`.
///
Expand Down Expand Up @@ -94,26 +95,26 @@ impl WasmerEnv for isize {}
impl WasmerEnv for char {}
impl WasmerEnv for bool {}
impl WasmerEnv for String {}
impl WasmerEnv for ::std::sync::atomic::AtomicBool {}
impl WasmerEnv for ::std::sync::atomic::AtomicI8 {}
impl WasmerEnv for ::std::sync::atomic::AtomicU8 {}
impl WasmerEnv for ::std::sync::atomic::AtomicI16 {}
impl WasmerEnv for ::std::sync::atomic::AtomicU16 {}
impl WasmerEnv for ::std::sync::atomic::AtomicI32 {}
impl WasmerEnv for ::std::sync::atomic::AtomicU32 {}
impl WasmerEnv for ::std::sync::atomic::AtomicI64 {}
impl WasmerEnv for ::std::sync::atomic::AtomicUsize {}
impl WasmerEnv for ::std::sync::atomic::AtomicIsize {}
impl WasmerEnv for dyn ::std::any::Any {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicBool {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicI8 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicU8 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicI16 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicU16 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicI32 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicU32 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicI64 {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicUsize {}
impl<'a> WasmerEnv for &'a ::std::sync::atomic::AtomicIsize {}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need the <'a> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<'a> is needed because of the &: trait impls on things with lifetimes can't omit them. And the reason we need & is to make these types shared somehow, otherwise the impl WasmerEnv for X can't be used here unless we have recursive definitions.

These impls on std types definitely aren't critical; these actually are only work with 'static lifetimes because of other constraints, so this only really works when putting an atomic in a static variable or otherwise doing unsafe things.

I'd be fine to remove them

impl<T: WasmerEnv> WasmerEnv for Box<T> {
fn init_with_instance(&mut self, instance: &Instance) -> Result<(), HostEnvInitError> {
(&mut **self).init_with_instance(instance)
}
}

impl<T: WasmerEnv> WasmerEnv for &'static mut T {
impl<T: WasmerEnv> WasmerEnv for ::std::sync::Arc<::std::sync::Mutex<T>> {
fn init_with_instance(&mut self, instance: &Instance) -> Result<(), HostEnvInitError> {
(*self).init_with_instance(instance)
let mut guard = self.lock().unwrap();
guard.init_with_instance(instance)
}
}

Expand Down
161 changes: 125 additions & 36 deletions lib/api/src/externals/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ pub use inner::{UnsafeMutableEnv, WithUnsafeMutableEnv};

use std::cmp::max;
use std::fmt;
use wasmer_engine::{Export, ExportFunction};
use std::sync::Arc;
use wasmer_engine::{Export, ExportFunction, ExportFunctionMetadata};
use wasmer_vm::{
raise_user_trap, resume_panic, wasmer_call_trampoline, VMCallerCheckedAnyfunc,
VMDynamicFunctionContext, VMExportFunction, VMFunctionBody, VMFunctionEnvironment,
Expand Down Expand Up @@ -87,23 +88,41 @@ impl Function {
where
F: Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
{
let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithoutEnv {
func: Box::new(func),
function_type: ty.clone(),
});
let dynamic_ctx: VMDynamicFunctionContext<DynamicFunctionWithoutEnv> =
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this type annotation isn't required (regardless, I prefer having type annotation in this kind of code, so it's perfectly fine to keep it, just wondered whether it changes something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly no, I thought it was helpful for understanding what's happening with the unsafe block here though.

Actually all these wrapper functions can be created by a helper function, so I should probably just do that

VMDynamicFunctionContext::from_context(DynamicFunctionWithoutEnv {
func: Arc::new(func),
function_type: ty.clone(),
});
// We don't yet have the address with the Wasm ABI signature.
// The engine linker will replace the address with one pointing to a
// generated dynamic trampoline.
let address = std::ptr::null() as *const VMFunctionBody;
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(Box::new(dynamic_ctx)) as *mut _,
let host_env = Box::into_raw(Box::new(dynamic_ctx)) as *mut _;
let vmctx = VMFunctionEnvironment { host_env };
let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
let duped_env: VMDynamicFunctionContext<DynamicFunctionWithoutEnv> = unsafe {
let ptr: *mut VMDynamicFunctionContext<DynamicFunctionWithoutEnv> = ptr as _;
let item: &VMDynamicFunctionContext<DynamicFunctionWithoutEnv> = &*ptr;
item.clone()
};
Box::into_raw(Box::new(duped_env)) as _
};
let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| {
unsafe {
Box::from_raw(ptr as *mut VMDynamicFunctionContext<DynamicFunctionWithoutEnv>)
};
};

Self {
store: store.clone(),
definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: false }),
exported: ExportFunction {
import_init_function_ptr: None,
metadata: Some(Arc::new(ExportFunctionMetadata::new(
host_env,
None,
host_env_clone_fn,
host_env_drop_fn,
))),
vm_function: VMExportFunction {
address,
kind: VMFunctionKind::Dynamic,
Expand Down Expand Up @@ -143,30 +162,60 @@ impl Function {
F: Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
Env: Sized + WasmerEnv + 'static,
{
let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithEnv {
env: Box::new(env),
func: Box::new(func),
function_type: ty.clone(),
});
let dynamic_ctx: VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>> =
VMDynamicFunctionContext::from_context(DynamicFunctionWithEnv {
env: {
let e = Box::new(env);
e
},
func: Arc::new(func),
function_type: ty.clone(),
});
// We don't yet have the address with the Wasm ABI signature.
// The engine linker will replace the address with one pointing to a
// generated dynamic trampoline.
let address = std::ptr::null() as *const VMFunctionBody;
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(Box::new(dynamic_ctx)) as *mut _,
};
// TODO: look into removing transmute by changing API type signatures
let host_env = Box::into_raw(Box::new(dynamic_ctx)) as *mut _;
let vmctx = VMFunctionEnvironment { host_env };
let import_init_function_ptr: fn(_, _) -> Result<(), _> =
|ptr: *mut std::ffi::c_void, instance: *const std::ffi::c_void| {
let ptr = ptr as *mut VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>>;
unsafe {
let env = &mut *ptr;
let env: &mut Env = &mut *env.ctx.env;
let instance = &*(instance as *const crate::Instance);
Env::init_with_instance(env, instance)
}
};
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
Env::init_with_instance,
import_init_function_ptr,
)
});
let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
let duped_env: VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>> = unsafe {
let ptr: *mut VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>> = ptr as _;
let item: &VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>> = &*ptr;
item.clone()
};
Box::into_raw(Box::new(duped_env)) as _
};
let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| {
unsafe {
Box::from_raw(ptr as *mut VMDynamicFunctionContext<DynamicFunctionWithEnv<Env>>)
};
};

Self {
store: store.clone(),
definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }),
exported: ExportFunction {
import_init_function_ptr,
metadata: Some(Arc::new(ExportFunctionMetadata::new(
host_env,
import_init_function_ptr,
host_env_clone_fn,
host_env_drop_fn,
))),
vm_function: VMExportFunction {
address,
kind: VMFunctionKind::Dynamic,
Expand Down Expand Up @@ -220,7 +269,7 @@ impl Function {
exported: ExportFunction {
// TODO: figure out what's going on in this function: it takes an `Env`
// param but also marks itself as not having an env
import_init_function_ptr: None,
metadata: None,
vm_function: VMExportFunction {
address,
vmctx,
Expand Down Expand Up @@ -274,10 +323,20 @@ impl Function {
// Wasm-defined functions have a `VMContext`.
// In the case of Host-defined functions `VMContext` is whatever environment
// the user want to attach to the function.
let box_env = Box::new(env);
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(box_env) as *mut _,
let host_env = Box::into_raw(Box::new(env)) as *mut _;
let vmctx = VMFunctionEnvironment { host_env };
let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
let duped_env = unsafe {
let ptr: *mut Env = ptr as _;
let item: &Env = &*ptr;
item.clone()
};
Box::into_raw(Box::new(duped_env)) as _
};
let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| {
unsafe { Box::from_raw(ptr as *mut Env) };
};

// TODO: look into removing transmute by changing API type signatures
let import_init_function_ptr = Some(unsafe {
std::mem::transmute::<fn(_, _) -> Result<(), _>, fn(_, _) -> Result<(), _>>(
Expand All @@ -290,7 +349,12 @@ impl Function {
store: store.clone(),
definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }),
exported: ExportFunction {
import_init_function_ptr,
metadata: Some(Arc::new(ExportFunctionMetadata::new(
host_env,
import_init_function_ptr,
host_env_clone_fn,
host_env_drop_fn,
))),
vm_function: VMExportFunction {
address,
kind: VMFunctionKind::Static,
Expand Down Expand Up @@ -329,9 +393,20 @@ impl Function {
let address = function.address();

let box_env = Box::new(env);
let vmctx = VMFunctionEnvironment {
host_env: Box::into_raw(box_env) as *mut _,
let host_env = Box::into_raw(box_env) as *mut _;
let vmctx = VMFunctionEnvironment { host_env };
let host_env_clone_fn: fn(*mut std::ffi::c_void) -> *mut std::ffi::c_void = |ptr| {
let duped_env: Env = {
let ptr: *mut Env = ptr as _;
let item: &Env = &*ptr;
item.clone()
};
Box::into_raw(Box::new(duped_env)) as _
};
let host_env_drop_fn: fn(*mut std::ffi::c_void) = |ptr: *mut std::ffi::c_void| {
Box::from_raw(ptr as *mut Env);
};

let signature = function.ty();
// TODO: look into removing transmute by changing API type signatures
let import_init_function_ptr = Some(std::mem::transmute::<
Expand All @@ -343,7 +418,12 @@ impl Function {
store: store.clone(),
definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }),
exported: ExportFunction {
import_init_function_ptr,
metadata: Some(Arc::new(ExportFunctionMetadata {
host_env,
host_env_clone_fn,
host_env_drop_fn,
import_init_function_ptr,
})),
vm_function: VMExportFunction {
address,
kind: VMFunctionKind::Static,
Expand Down Expand Up @@ -712,13 +792,14 @@ pub(crate) trait VMDynamicFunction {
fn function_type(&self) -> &FunctionType;
}

pub(crate) struct VMDynamicFunctionWithoutEnv {
#[derive(Clone)]
pub(crate) struct DynamicFunctionWithoutEnv {
#[allow(clippy::type_complexity)]
func: Box<dyn Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
func: Arc<dyn Fn(&[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
function_type: FunctionType,
}

impl VMDynamicFunction for VMDynamicFunctionWithoutEnv {
impl VMDynamicFunction for DynamicFunctionWithoutEnv {
fn call(&self, args: &[Val]) -> Result<Vec<Val>, RuntimeError> {
(*self.func)(&args)
}
Expand All @@ -727,19 +808,27 @@ impl VMDynamicFunction for VMDynamicFunctionWithoutEnv {
}
}

#[repr(C)]
pub(crate) struct VMDynamicFunctionWithEnv<Env>
pub(crate) struct DynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
{
// This field _must_ come first in this struct.
env: Box<Env>,
Comment on lines -779 to -780
Copy link
Contributor

Choose a reason for hiding this comment

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

Why env is no longer required to be at first position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason before was due to a bad hack where we'd double deref a pointer ignoring the fact that it was in a struct.

We now have each function that is called on the env traverse things correctly, so for example we create a wrapper function where we walk through the types and find what we need. So if we need to operate on the Box<env> itself, we can find it in each function. I think this is a simpler solution because then all the functions always just work on the env, we had an issue where the clone and drop function needed to be called on the top level the wrapper dynamic func context thing but the initialize function needed to be called on the inner env. This fixes that by making the thing each function gets consistent.

function_type: FunctionType,
#[allow(clippy::type_complexity)]
func: Box<dyn Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
func: Arc<dyn Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
env: Box<Env>,
}

impl<Env: Sized + Clone + 'static> Clone for DynamicFunctionWithEnv<Env> {
fn clone(&self) -> Self {
Self {
env: self.env.clone(),
function_type: self.function_type.clone(),
func: self.func.clone(),
}
}
}

impl<Env> VMDynamicFunction for VMDynamicFunctionWithEnv<Env>
impl<Env> VMDynamicFunction for DynamicFunctionWithEnv<Env>
where
Env: Sized + 'static,
{
Expand Down
16 changes: 7 additions & 9 deletions lib/api/src/native.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,18 @@
use std::marker::PhantomData;

use crate::externals::function::{
FunctionDefinition, HostFunctionDefinition, VMDynamicFunction, VMDynamicFunctionWithEnv,
VMDynamicFunctionWithoutEnv, WasmFunctionDefinition,
DynamicFunctionWithEnv, DynamicFunctionWithoutEnv, FunctionDefinition, HostFunctionDefinition,
VMDynamicFunction, WasmFunctionDefinition,
};
use crate::{FromToNativeWasmType, Function, FunctionType, RuntimeError, Store, WasmTypeList};
use crate::{FromToNativeWasmType, Function, RuntimeError, Store, WasmTypeList};
use std::panic::{catch_unwind, AssertUnwindSafe};
use wasmer_engine::ExportFunction;
use wasmer_types::NativeWasmType;
use wasmer_vm::{
VMDynamicFunctionContext, VMExportFunction, VMFunctionBody, VMFunctionEnvironment,
VMFunctionKind,
};
use wasmer_vm::{VMDynamicFunctionContext, VMFunctionBody, VMFunctionEnvironment, VMFunctionKind};

/// A WebAssembly function that can be called natively
/// (using the Native ABI).
#[derive(Clone)]
pub struct NativeFunc<Args = (), Rets = ()> {
definition: FunctionDefinition,
store: Store,
Expand Down Expand Up @@ -186,13 +184,13 @@ macro_rules! impl_native_traits {
VMFunctionKind::Dynamic => {
let params_list = [ $( $x.to_native().to_value() ),* ];
let results = if !has_env {
type VMContextWithoutEnv = VMDynamicFunctionContext<VMDynamicFunctionWithoutEnv>;
type VMContextWithoutEnv = VMDynamicFunctionContext<DynamicFunctionWithoutEnv>;
unsafe {
let ctx = self.vmctx().host_env as *mut VMContextWithoutEnv;
(*ctx).ctx.call(&params_list)?
}
} else {
type VMContextWithEnv = VMDynamicFunctionContext<VMDynamicFunctionWithEnv<std::ffi::c_void>>;
type VMContextWithEnv = VMDynamicFunctionContext<DynamicFunctionWithEnv<std::ffi::c_void>>;
unsafe {
let ctx = self.vmctx().host_env as *mut VMContextWithEnv;
(*ctx).ctx.call(&params_list)?
Expand Down
2 changes: 1 addition & 1 deletion lib/api/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl ValFuncRef for Val {
let export = wasmer_engine::ExportFunction {
// TODO:
// figure out if we ever need a value here: need testing with complicated import patterns
import_init_function_ptr: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the TODO above still valid?

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, I'm not 100% confident in this code which is why that comment is still there, I can investigate it later... I'm not aware of a good way to test this yet, I can investigate that though

Copy link
Member

Choose a reason for hiding this comment

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

We should investigate. I bet is fine as None, but it will be good to verify

metadata: None,
vm_function: wasmer_vm::VMExportFunction {
address: item.func_ptr,
signature,
Expand Down
Loading