Merge #1865

1865: Fix memory leak in host function envs r=MarkMcCaskey a=MarkMcCaskey

TODO: link to issue

This PR contains a number of changes:

1. Make `WasmerEnv: Clone`
2. Store a pointer to the `clone` function when creating a host function (Notably this is a feature that wouldn't work even if we _could_ use a proper trait object because you can't have a `Sized` trait object and `Clone: Sized`).
3. Store a pointer to the `drop` function when creating a host function.
4. Clone the env via pointer every time an `Instance` is made. Therefore each `Instance` gets its own, unique `Env` per host function with `Env`.
5. Add reference counting and drop logic to a sub-field of `wasmer_export::ExportFunction` which frees the original version of the `Env` (the thing that gets cloned each time an `Instance` is made) with the `drop` function pointer.
6. Change some logic in `vm::Instance` from SoA (struct of arrays) to AoS (array of structs): this uses more memory but is a bit less error prone and can be easily changed later.
7. Add logic on this new struct (`vm::ImportEnv`) that contains the function pointers for each import in `Instance` to drop (with the `drop` fn pointer) when the `vm::Instance` is being dropped. This fixes the original memory leak.
8. Add wrapper functions inside the host function creation functions which makes the layout of the user supplied env-pointer the responsibility of each function.  Thus, rather than `drop` being `Env::drop`, it's a function which frees all wrapper types, traverses indirections and frees the internal `Env` with `Env::drop`.  This simplifies code at the cost of making the `host_env` pointer (`vmctx`) not consistent in terms of what it actually points to.  This change fixes another memory leak related to the creation of host functions.

tl;dr: we're leaning into manually doing virtual method dispatch on `WasmerEnv`s and it actually works great! The biggest issue I have with the PR as-is is that the code isn't as clean/readable/robust as I'd ideally like it to be.

Edit (by @Hywan): This PR fixes #1584, #1714, #1865, #1667.

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>

CHANGELOG.md

@@ -18,6 +18,7 @@
 
 ### Changed
 
+- [#1865](https://github.com/wasmerio/wasmer/pull/1865) Require that implementors of `WasmerEnv` also implement `Send`, `Sync`, and `Clone`.
 - [#1851](https://github.com/wasmerio/wasmer/pull/1851) Improve test suite and documentation of the Wasmer C API
 - [#1874](https://github.com/wasmerio/wasmer/pull/1874) Set `CompilerConfig` to be owned (following wasm-c-api)
 - [#1880](https://github.com/wasmerio/wasmer/pull/1880) Remove cmake dependency for tests

examples/imports_function_env.rs

@@ -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;
@@ -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.
@@ -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>>,
     }
 
     // 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
@@ -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...");
@@ -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);
 

lib/api/src/env.rs

@@ -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)]
@@ -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>,
 /// }
@@ -66,7 +67,7 @@ impl From<ExportError> for HostEnvInitError {
 ///     }
 /// }
 /// ```
-pub trait WasmerEnv {
+pub trait WasmerEnv: Clone + Send + Sync {
     /// The function that Wasmer will call on your type to let it finish
     /// setting up the environment with data from the `Instance`.
     ///
@@ -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 {}
 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)
     }
 }