Merge #1663

1663: Make Function Env immutable r=MarkMcCaskey a=MarkMcCaskey Alternative / related to #1625 Resolves #1612 # Review - [x] 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]>
wasmerio · Nov 20, 2020 · 3c788dd · 3c788dd
2 parents 92ee697 + 77c12e9
commit 3c788dd
Show file tree

Hide file tree

Showing 51 changed files with 838 additions and 756 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@
 
 ### Changed
 
+- [#1663](https://github.com/wasmerio/wasmer/pull/1663) Function environments passed to host functions now must be passed by `&` instead of `&mut`. This is a breaking change. This change fixes a race condition when a host function is called from multiple threads. If you need mutability in your environment, consider using `std::sync::Mutex` or other synchronization primitives.
 - [#1830](https://github.com/wasmerio/wasmer/pull/1830) Minimum supported Rust version bumped to 1.47.0
 - [#1810](https://github.com/wasmerio/wasmer/pull/1810) Make the `state` field of `WasiEnv` public
 

diff --git a/examples/imports_function_env.rs b/examples/imports_function_env.rs
@@ -74,10 +74,10 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     }
 
     // Create the functions
-    fn get_counter(env: &mut Env) -> i32 {
+    fn get_counter(env: &Env) -> i32 {
         *env.counter.borrow()
     }
-    fn add_to_counter(env: &mut Env, add: i32) -> i32 {
+    fn add_to_counter(env: &Env, add: i32) -> i32 {
         let mut counter_ref = env.counter.borrow_mut();
 
         *counter_ref += add;

diff --git a/lib/api/Cargo.toml b/lib/api/Cargo.toml
@@ -67,6 +67,8 @@ llvm = [
     "wasmer-compiler-llvm",
     "compiler",
 ]
+# enables internal features used by the deprecated API.
+deprecated = []
 default-compiler = []
 default-engine = []
 

diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs
@@ -6,7 +6,9 @@ use crate::FunctionType;
 use crate::NativeFunc;
 use crate::RuntimeError;
 pub use inner::{FromToNativeWasmType, HostFunction, WasmTypeList, WithEnv, WithoutEnv};
-use std::cell::RefCell;
+#[cfg(feature = "deprecated")]
+pub use inner::{UnsafeMutableEnv, WithUnsafeMutableEnv};
+
 use std::cmp::max;
 use std::fmt;
 use wasmer_vm::{
@@ -125,11 +127,11 @@ impl Function {
     #[allow(clippy::cast_ptr_alignment)]
     pub fn new_with_env<F, Env>(store: &Store, ty: &FunctionType, env: Env, func: F) -> Self
     where
-        F: Fn(&mut Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
+        F: Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static,
         Env: Sized + 'static,
     {
         let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithEnv {
-            env: RefCell::new(env),
+            env: Box::new(env),
             func: Box::new(func),
             function_type: ty.clone(),
         });
@@ -214,7 +216,7 @@ impl Function {
     /// };
     /// let env = Env { multiplier: 2 };
     ///
-    /// fn sum_and_multiply(env: &mut Env, a: i32, b: i32) -> i32 {
+    /// fn sum_and_multiply(env: &Env, a: i32, b: i32) -> i32 {
     ///     (a + b) * env.multiplier
     /// }
     ///
@@ -254,6 +256,47 @@ impl Function {
         }
     }
 
+    /// Function used by the deprecated API to call a function with a `&mut` Env.
+    ///
+    /// This is not a stable API and may be broken at any time.
+    ///
+    /// # Safety
+    /// - This function is only safe to use from the deprecated API.
+    #[doc(hidden)]
+    #[cfg(feature = "deprecated")]
+    pub unsafe fn new_native_with_unsafe_mutable_env<F, Args, Rets, Env>(
+        store: &Store,
+        env: Env,
+        func: F,
+    ) -> Self
+    where
+        F: HostFunction<Args, Rets, WithUnsafeMutableEnv, Env>,
+        Args: WasmTypeList,
+        Rets: WasmTypeList,
+        Env: UnsafeMutableEnv + 'static,
+    {
+        let function = inner::Function::<Args, Rets>::new(func);
+        let address = function.address();
+
+        let box_env = Box::new(env);
+        let vmctx = VMFunctionEnvironment {
+            host_env: Box::into_raw(box_env) as *mut _,
+        };
+        let signature = function.ty();
+
+        Self {
+            store: store.clone(),
+            definition: FunctionDefinition::Host(HostFunctionDefinition { has_env: true }),
+            exported: ExportFunction {
+                address,
+                kind: VMFunctionKind::Static,
+                vmctx,
+                signature,
+                call_trampoline: None,
+            },
+        }
+    }
+
     /// Returns the [`FunctionType`] of the `Function`.
     ///
     /// # Example
@@ -628,18 +671,16 @@ where
 {
     function_type: FunctionType,
     #[allow(clippy::type_complexity)]
-    func: Box<dyn Fn(&mut Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
-    env: RefCell<Env>,
+    func: Box<dyn Fn(&Env, &[Val]) -> Result<Vec<Val>, RuntimeError> + 'static>,
+    env: Box<Env>,
 }
 
 impl<Env> VMDynamicFunction for VMDynamicFunctionWithEnv<Env>
 where
     Env: Sized + 'static,
 {
     fn call(&self, args: &[Val]) -> Result<Vec<Val>, RuntimeError> {
-        // TODO: the `&mut *self.env.as_ptr()` is likely invoking some "mild"
-        //      undefined behavior due to how it's used in the static fn call
-        unsafe { (*self.func)(&mut *self.env.as_ptr(), &args) }
+        (*self.func)(&*self.env, &args)
     }
     fn function_type(&self) -> &FunctionType {
         &self.function_type
@@ -979,6 +1020,14 @@ mod inner {
         fn function_body_ptr(self) -> *const VMFunctionBody;
     }
 
+    /// Marker trait to limit what the hidden APIs needed for the deprecated API
+    /// can be used on.
+    ///
+    /// Marks an environment as being passed by `&mut`.
+    #[cfg(feature = "deprecated")]
+    #[doc(hidden)]
+    pub unsafe trait UnsafeMutableEnv: Sized {}
+
     /// Empty trait to specify the kind of `HostFunction`: With or
     /// without an environment.
     ///
@@ -994,6 +1043,18 @@ mod inner {
 
     impl HostFunctionKind for WithEnv {}
 
+    /// An empty struct to help Rust typing to determine
+    /// when a `HostFunction` has an environment.
+    ///
+    /// This environment is passed by `&mut` and exists solely for the deprecated
+    /// API.
+    #[cfg(feature = "deprecated")]
+    #[doc(hidden)]
+    pub struct WithUnsafeMutableEnv;
+
+    #[cfg(feature = "deprecated")]
+    impl HostFunctionKind for WithUnsafeMutableEnv {}
+
     /// An empty struct to help Rust typing to determine
     /// when a `HostFunction` does not have an environment.
     pub struct WithoutEnv;
@@ -1187,6 +1248,52 @@ mod inner {
                 Rets: WasmTypeList,
                 RetsAsResult: IntoResult<Rets>,
                 Env: Sized,
+                Func: Fn(&Env, $( $x , )*) -> RetsAsResult + Send + 'static,
+            {
+                #[allow(non_snake_case)]
+                fn function_body_ptr(self) -> *const VMFunctionBody {
+                    /// This is a function that wraps the real host
+                    /// function. Its address will be used inside the
+                    /// runtime.
+                    extern fn func_wrapper<$( $x, )* Rets, RetsAsResult, Env, Func>( env: &Env, $( $x: $x::Native, )* ) -> Rets::CStruct
+                    where
+                        $( $x: FromToNativeWasmType, )*
+                        Rets: WasmTypeList,
+                        RetsAsResult: IntoResult<Rets>,
+                        Env: Sized,
+                        Func: Fn(&Env, $( $x ),* ) -> RetsAsResult + 'static
+                    {
+                        let func: &Func = unsafe { &*(&() as *const () as *const Func) };
+
+                        let result = panic::catch_unwind(AssertUnwindSafe(|| {
+                            func(env, $( FromToNativeWasmType::from_native($x) ),* ).into_result()
+                        }));
+
+                        match result {
+                            Ok(Ok(result)) => return result.into_c_struct(),
+                            Ok(Err(trap)) => unsafe { raise_user_trap(Box::new(trap)) },
+                            Err(panic) => unsafe { resume_panic(panic) },
+                        }
+                    }
+
+                    func_wrapper::< $( $x, )* Rets, RetsAsResult, Env, Self > as *const VMFunctionBody
+                }
+            }
+
+            // Implement `HostFunction` for a function that has the same arity than the tuple.
+            // This specific function has an environment.
+            #[doc(hidden)]
+            #[cfg(feature = "deprecated")]
+            #[allow(unused_parens)]
+            impl< $( $x, )* Rets, RetsAsResult, Env, Func >
+                HostFunction<( $( $x ),* ), Rets, WithUnsafeMutableEnv, Env>
+            for
+                Func
+            where
+                $( $x: FromToNativeWasmType, )*
+                Rets: WasmTypeList,
+                RetsAsResult: IntoResult<Rets>,
+                Env: UnsafeMutableEnv,
                 Func: Fn(&mut Env, $( $x , )*) -> RetsAsResult + Send + 'static,
             {
                 #[allow(non_snake_case)]
@@ -1218,7 +1325,6 @@ mod inner {
                     func_wrapper::< $( $x, )* Rets, RetsAsResult, Env, Self > as *const VMFunctionBody
                 }
             }
-
         };
     }
 

diff --git a/lib/api/src/externals/mod.rs b/lib/api/src/externals/mod.rs
@@ -6,6 +6,9 @@ mod table;
 pub use self::function::{
     FromToNativeWasmType, Function, HostFunction, WasmTypeList, WithEnv, WithoutEnv,
 };
+
+#[cfg(feature = "deprecated")]
+pub use self::function::{UnsafeMutableEnv, WithUnsafeMutableEnv};
 pub use self::global::Global;
 pub use self::memory::Memory;
 pub use self::table::Table;

diff --git a/lib/api/src/lib.rs b/lib/api/src/lib.rs
@@ -46,6 +46,8 @@ pub mod internals {
     //! `wasmer-vm`. Please don't use any of this types directly, as
     //! they might change frequently or be removed in the future.
 
+    #[cfg(feature = "deprecated")]
+    pub use crate::externals::{UnsafeMutableEnv, WithUnsafeMutableEnv};
     pub use crate::externals::{WithEnv, WithoutEnv};
 }
 

diff --git a/lib/api/tests/externals.rs b/lib/api/tests/externals.rs
@@ -211,33 +211,33 @@ fn function_new_env() -> Result<()> {
     #[derive(Clone)]
     struct MyEnv {};
     let my_env = MyEnv {};
-    let function = Function::new_native_with_env(&store, my_env.clone(), |_env: &mut MyEnv| {});
+    let function = Function::new_native_with_env(&store, my_env.clone(), |_env: &MyEnv| {});
     assert_eq!(function.ty().clone(), FunctionType::new(vec![], vec![]));
     let function =
-        Function::new_native_with_env(&store, my_env.clone(), |_env: &mut MyEnv, _a: i32| {});
+        Function::new_native_with_env(&store, my_env.clone(), |_env: &MyEnv, _a: i32| {});
     assert_eq!(
         function.ty().clone(),
         FunctionType::new(vec![Type::I32], vec![])
     );
     let function = Function::new_native_with_env(
         &store,
         my_env.clone(),
-        |_env: &mut MyEnv, _a: i32, _b: i64, _c: f32, _d: f64| {},
+        |_env: &MyEnv, _a: i32, _b: i64, _c: f32, _d: f64| {},
     );
     assert_eq!(
         function.ty().clone(),
         FunctionType::new(vec![Type::I32, Type::I64, Type::F32, Type::F64], vec![])
     );
     let function =
-        Function::new_native_with_env(&store, my_env.clone(), |_env: &mut MyEnv| -> i32 { 1 });
+        Function::new_native_with_env(&store, my_env.clone(), |_env: &MyEnv| -> i32 { 1 });
     assert_eq!(
         function.ty().clone(),
         FunctionType::new(vec![], vec![Type::I32])
     );
     let function = Function::new_native_with_env(
         &store,
         my_env.clone(),
-        |_env: &mut MyEnv| -> (i32, i64, f32, f64) { (1, 2, 3.0, 4.0) },
+        |_env: &MyEnv| -> (i32, i64, f32, f64) { (1, 2, 3.0, 4.0) },
     );
     assert_eq!(
         function.ty().clone(),
@@ -279,39 +279,39 @@ fn function_new_dynamic_env() -> Result<()> {
         &store,
         &function_type,
         my_env.clone(),
-        |_env: &mut MyEnv, _values: &[Value]| unimplemented!(),
+        |_env: &MyEnv, _values: &[Value]| unimplemented!(),
     );
     assert_eq!(function.ty().clone(), function_type);
     let function_type = FunctionType::new(vec![Type::I32], vec![]);
     let function = Function::new_with_env(
         &store,
         &function_type,
         my_env.clone(),
-        |_env: &mut MyEnv, _values: &[Value]| unimplemented!(),
+        |_env: &MyEnv, _values: &[Value]| unimplemented!(),
     );
     assert_eq!(function.ty().clone(), function_type);
     let function_type = FunctionType::new(vec![Type::I32, Type::I64, Type::F32, Type::F64], vec![]);
     let function = Function::new_with_env(
         &store,
         &function_type,
         my_env.clone(),
-        |_env: &mut MyEnv, _values: &[Value]| unimplemented!(),
+        |_env: &MyEnv, _values: &[Value]| unimplemented!(),
     );
     assert_eq!(function.ty().clone(), function_type);
     let function_type = FunctionType::new(vec![], vec![Type::I32]);
     let function = Function::new_with_env(
         &store,
         &function_type,
         my_env.clone(),
-        |_env: &mut MyEnv, _values: &[Value]| unimplemented!(),
+        |_env: &MyEnv, _values: &[Value]| unimplemented!(),
     );
     assert_eq!(function.ty().clone(), function_type);
     let function_type = FunctionType::new(vec![], vec![Type::I32, Type::I64, Type::F32, Type::F64]);
     let function = Function::new_with_env(
         &store,
         &function_type,
         my_env.clone(),
-        |_env: &mut MyEnv, _values: &[Value]| unimplemented!(),
+        |_env: &MyEnv, _values: &[Value]| unimplemented!(),
     );
     assert_eq!(function.ty().clone(), function_type);
     Ok(())

diff --git a/lib/c-api/src/deprecated/import/mod.rs b/lib/c-api/src/deprecated/import/mod.rs
@@ -45,10 +45,10 @@ pub(crate) struct CAPIImportObject {
     /// but the new/current API does not. So we store them here to pass them to the Instance
     /// to allow functions to access this data for backwards compatibilty.
     pub(crate) imported_memories: Vec<*mut Memory>,
-    /// List of pointers to `LegacyEnv`s used to patch imported functions to be able to
+    /// List of pointers to `UnsafeMutableEnv`s used to patch imported functions to be able to
     /// pass a the "vmctx" as the first argument.
     /// Needed here because of extending import objects.
-    pub(crate) instance_pointers_to_update: Vec<NonNull<LegacyEnv>>,
+    pub(crate) instance_pointers_to_update: Vec<NonNull<UnsafeMutableEnv>>,
 }
 
 #[repr(C)]
@@ -399,7 +399,7 @@ pub unsafe extern "C" fn wasmer_import_object_imports_destroy(
                 let function_wrapper: Box<FunctionWrapper> =
                     Box::from_raw(import.value.func as *mut _);
                 let _: Box<Function> = Box::from_raw(function_wrapper.func.as_ptr());
-                let _: Box<LegacyEnv> = Box::from_raw(function_wrapper.legacy_env.as_ptr());
+                let _: Box<UnsafeMutableEnv> = Box::from_raw(function_wrapper.legacy_env.as_ptr());
             }
             wasmer_import_export_kind::WASM_GLOBAL => {
                 let _: Box<Global> = Box::from_raw(import.value.global as *mut _);
@@ -635,25 +635,25 @@ pub unsafe extern "C" fn wasmer_import_func_params_arity(
 
 /// struct used to pass in context to functions (which must be back-patched)
 #[derive(Debug, Default)]
-pub(crate) struct LegacyEnv {
+pub(crate) struct UnsafeMutableEnv {
     pub(crate) instance_ptr: Option<NonNull<CAPIInstance>>,
 }
 
-impl LegacyEnv {
+impl UnsafeMutableEnv {
     pub(crate) fn ctx_ptr(&self) -> *mut CAPIInstance {
         self.instance_ptr
             .map(|p| p.as_ptr())
             .unwrap_or(ptr::null_mut())
     }
 }
 
-/// struct used to hold on to `LegacyEnv` pointer as well as the function.
-/// we need to do this to initialize the context ptr inside of `LegacyEnv` when
+/// struct used to hold on to `UnsafeMutableEnv` pointer as well as the function.
+/// we need to do this to initialize the context ptr inside of `UnsafeMutableEnv` when
 /// instantiating the module.
 #[derive(Debug)]
 pub(crate) struct FunctionWrapper {
     pub(crate) func: NonNull<Function>,
-    pub(crate) legacy_env: NonNull<LegacyEnv>,
+    pub(crate) legacy_env: NonNull<UnsafeMutableEnv>,
 }
 
 /// Creates new host function, aka imported function. `func` is a
@@ -690,7 +690,7 @@ pub unsafe extern "C" fn wasmer_import_func_new(
 
     let store = get_global_store();
 
-    let env_ptr = Box::into_raw(Box::new(LegacyEnv::default()));
+    let env_ptr = Box::into_raw(Box::new(UnsafeMutableEnv::default()));
 
     let func = Function::new_with_env(store, &func_type, &mut *env_ptr, move |env, args| {
         use libffi::high::call::{call, Arg};