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

Merged
merged 15 commits into from
Dec 15, 2020
Merged
19 changes: 9 additions & 10 deletions examples/imports_function_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
//!
//! Ready?

use std::cell::RefCell;
use std::sync::Arc;
use std::sync::{Arc, Mutex};
use wasmer::{imports, wat2wasm, Function, Instance, Module, Store, WasmerEnv};
use wasmer_compiler_cranelift::Cranelift;
use wasmer_engine_jit::JIT;
Expand Down Expand Up @@ -57,11 +56,11 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
let module = Module::new(&store, wasm_bytes)?;

// We create some shared data here, `Arc` is required because we may
// move our WebAssembly instance to another thread to run it. RefCell
// move our WebAssembly instance to another thread to run it. Mutex
// lets us get shared mutabilty which is fine because we know we won't
// run host calls concurrently. If concurrency is a possibilty, we'd have
// to use a `Mutex`.
let shared_counter: Arc<RefCell<i32>> = Arc::new(RefCell::new(0));
let shared_counter: Arc<Mutex<i32>> = Arc::new(Mutex::new(0));

// Once we have our counter we'll wrap it inside en `Env` which we'll pass
// to our imported functions.
Expand All @@ -70,17 +69,17 @@ 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>>,
counter: Arc<Mutex<i32>>,
Comment on lines -75 to +74
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change the examples? The previous examples should work as is. Or am I missing something?

Copy link
Contributor Author

@MarkMcCaskey MarkMcCaskey Dec 15, 2020

Choose a reason for hiding this comment

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

Yeah, the previous examples were incorrect, we just didn't enforce it. WasmerEnv needs to be Send and Sync because Instance is Send and Sync and we don't synchronize access to functions. So it's possible in safe Rust to get a NativeFunc on multiple threads that points to the same underlying function. In order for the WasmerEnv to be able to handle being used this way, it must be both Send and Sync. This shouldn't affect much, if any, real code. For example, WASI was not affected at all by this change. Emscripten was only affected because it was doing some weird stuff with pointers.

}

// Create the functions
fn get_counter(env: &Env) -> i32 {
*env.counter.borrow()
*env.counter.lock().unwrap()
}
fn add_to_counter(env: &Env, add: i32) -> i32 {
let mut counter_ref = env.counter.borrow_mut();
let mut counter_ref = env.counter.lock().unwrap();

*counter_ref += add;
*counter_ref
Expand All @@ -106,7 +105,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
.get_function("increment_counter_loop")?
.native::<i32, i32>()?;

let counter_value: i32 = *shared_counter.borrow();
let counter_value: i32 = *shared_counter.lock().unwrap();
println!("Initial ounter value: {:?}", counter_value);

println!("Calling `increment_counter_loop` function...");
Expand All @@ -115,7 +114,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
// It will loop five times thus incrementing our counter five times.
let result = increment_counter_loop.call(5)?;

let counter_value: i32 = *shared_counter.borrow();
let counter_value: i32 = *shared_counter.lock().unwrap();
println!("New counter value (host): {:?}", counter_value);
assert_eq!(counter_value, 5);

Expand Down
33 changes: 17 additions & 16 deletions lib/api/src/env.rs
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 + Send {
/// 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
Loading