From 29d50a55cabc87ad0c772e5ac02eaceea1dc47a3 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 24 Nov 2020 12:40:51 -0800 Subject: [PATCH 1/5] Disable closures as host functions for now + docs + tests --- CHANGELOG.md | 1 + lib/api/src/externals/function.rs | 24 ++++++ tests/compilers/native_functions.rs | 112 ++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71c945933bf..a42806eda50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ ### Fixed +- [#1841](https://github.com/wasmerio/wasmer/pull/1841) We will now panic when attempting to use a function with a captured env as a host function. Previously this would silently do the wrong thing. See [#1840](https://github.com/wasmerio/wasmer/pull/1840) for info about Wasmer's support of closures as host functions. - [#1764](https://github.com/wasmerio/wasmer/pull/1764) Fix bug in WASI `path_rename` allowing renamed files to be 1 directory below a preopened directory. ## 1.0.0-alpha5 - 2020-11-06 diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index b65462e3af3..7ba6ac58653 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -51,6 +51,11 @@ pub enum FunctionDefinition { /// during execution of the function. /// /// Spec: https://webassembly.github.io/spec/core/exec/runtime.html#function-instances +/// +/// # Panics +/// - Closures (functions with captured environments) are not currently supported. +/// Attempting to create a `Function` with one will result in a panic. +/// [Closures as host functions tracking issue](https://github.com/wasmerio/wasmer/issues/1840) #[derive(Clone, PartialEq)] pub struct Function { pub(crate) store: Store, @@ -79,6 +84,9 @@ impl Function { where F: Fn(&[Val]) -> Result, RuntimeError> + 'static, { + if std::mem::size_of::() != 0 { + Self::closures_unsupported_panic(); + } let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithoutEnv { func: Box::new(func), function_type: ty.clone(), @@ -130,6 +138,9 @@ impl Function { F: Fn(&Env, &[Val]) -> Result, RuntimeError> + 'static, Env: Sized + 'static, { + if std::mem::size_of::() != 0 { + Self::closures_unsupported_panic(); + } let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithEnv { env: Box::new(env), func: Box::new(func), @@ -180,6 +191,9 @@ impl Function { Rets: WasmTypeList, Env: Sized + 'static, { + if std::mem::size_of::() != 0 { + Self::closures_unsupported_panic(); + } let function = inner::Function::::new(func); let address = function.address() as *const VMFunctionBody; let vmctx = VMFunctionEnvironment { @@ -229,6 +243,9 @@ impl Function { Rets: WasmTypeList, Env: Sized + 'static, { + if std::mem::size_of::() != 0 { + Self::closures_unsupported_panic(); + } let function = inner::Function::::new(func); let address = function.address(); @@ -275,6 +292,9 @@ impl Function { Rets: WasmTypeList, Env: UnsafeMutableEnv + 'static, { + if std::mem::size_of::() != 0 { + Self::closures_unsupported_panic(); + } let function = inner::Function::::new(func); let address = function.address(); @@ -620,6 +640,10 @@ impl Function { self.definition.clone(), )) } + + fn closures_unsupported_panic() -> ! { + unimplemented!("Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840") + } } impl<'a> Exportable<'a> for Function { diff --git a/tests/compilers/native_functions.rs b/tests/compilers/native_functions.rs index cd2600eac1f..66085327eb8 100644 --- a/tests/compilers/native_functions.rs +++ b/tests/compilers/native_functions.rs @@ -78,6 +78,118 @@ fn native_function_works_for_wasm() -> Result<()> { Ok(()) } +#[test] +#[should_panic( + expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" +)] +fn host_function_closure_panics() { + let store = get_store(false); + let state = 3; + let ty = FunctionType::new(vec![], vec![]); + Function::new(&store, &ty, move |_args| { + println!("{}", state); + Ok(vec![]) + }); +} + +#[test] +#[should_panic( + expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" +)] +fn env_host_function_closure_panics() { + let store = get_store(false); + let state = 3; + let ty = FunctionType::new(vec![], vec![]); + let env = 4; + Function::new_with_env(&store, &ty, env, move |_env, _args| { + println!("{}", state); + Ok(vec![]) + }); +} + +#[test] +#[should_panic( + expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" +)] +fn native_host_function_closure_panics() { + let store = get_store(false); + let state = 3; + Function::new_native(&store, move |_: i32| { + println!("{}", state); + }); +} + +#[test] +#[should_panic( + expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" +)] +fn native_with_env_host_function_closure_panics() { + let store = get_store(false); + let state = 3; + let env = 4; + Function::new_native_with_env(&store, env, move |_env: &i32, _: i32| { + println!("{}", state); + }); +} + +#[test] +fn lambdas_with_no_env_work() -> Result<()> { + let store = get_store(false); + let wat = r#"(module + (func $multiply1 (import "env" "multiply1") (param i32 i32) (result i32)) + (func $multiply2 (import "env" "multiply2") (param i32 i32) (result i32)) + (func $multiply3 (import "env" "multiply3") (param i32 i32) (result i32)) + (func $multiply4 (import "env" "multiply4") (param i32 i32) (result i32)) + + (func (export "test") (param i32 i32 i32 i32 i32) (result i32) + (call $multiply4 + (call $multiply3 + (call $multiply2 + (call $multiply1 + (local.get 0) + (local.get 1)) + (local.get 2)) + (local.get 3)) + (local.get 4))) +)"#; + let module = Module::new(&store, wat).unwrap(); + + let ty = FunctionType::new(vec![Type::I32, Type::I32], vec![Type::I32]); + let env = 10; + let import_object = imports! { + "env" => { + "multiply1" => Function::new(&store, &ty, |args| { + if let (Value::I32(v1), Value::I32(v2)) = (&args[0], &args[1]) { + Ok(vec![Value::I32(v1 * v2)]) + } else { + panic!("Invalid arguments"); + } + }), + "multiply2" => Function::new_with_env(&store, &ty, env, |&env, args| { + if let (Value::I32(v1), Value::I32(v2)) = (&args[0], &args[1]) { + Ok(vec![Value::I32(v1 * v2 * env)]) + } else { + panic!("Invalid arguments"); + } + }), + "multiply3" => Function::new_native(&store, |arg1: i32, arg2: i32| -> i32 + {arg1 * arg2 }), + "multiply4" => Function::new_native_with_env(&store, env, |&env: &i32, arg1: i32, arg2: i32| -> i32 + {arg1 * arg2 * env }), + }, + }; + + let instance = Instance::new(&module, &import_object)?; + + let test: NativeFunc<(i32, i32, i32, i32, i32), i32> = + instance.exports.get_native_function("test")?; + + let result = test.call(2, 3, 4, 5, 6)?; + let manually_computed_result = 6 * (5 * (4 * (3 * 2) * 10)) * 10; + assert_eq!(result, manually_computed_result); + Ok(()) +} + // The native ABI for functions fails when defining a function natively in // macos (Darwin) with the Apple Silicon ARM chip // TODO: Cranelift should have a good ABI for the ABI From 82fc0080ba6380a13dc2bdaea907cdf250f4d762 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 24 Nov 2020 13:15:38 -0800 Subject: [PATCH 2/5] Allow closures with non-native host functions --- lib/api/src/externals/function.rs | 13 +++------ tests/compilers/native_functions.rs | 44 ++++++----------------------- 2 files changed, 12 insertions(+), 45 deletions(-) diff --git a/lib/api/src/externals/function.rs b/lib/api/src/externals/function.rs index 7ba6ac58653..0142fec4ad5 100644 --- a/lib/api/src/externals/function.rs +++ b/lib/api/src/externals/function.rs @@ -53,8 +53,9 @@ pub enum FunctionDefinition { /// Spec: https://webassembly.github.io/spec/core/exec/runtime.html#function-instances /// /// # Panics -/// - Closures (functions with captured environments) are not currently supported. -/// Attempting to create a `Function` with one will result in a panic. +/// - Closures (functions with captured environments) are not currently supported +/// with native functions. Attempting to create a native `Function` with one will +/// result in a panic. /// [Closures as host functions tracking issue](https://github.com/wasmerio/wasmer/issues/1840) #[derive(Clone, PartialEq)] pub struct Function { @@ -84,9 +85,6 @@ impl Function { where F: Fn(&[Val]) -> Result, RuntimeError> + 'static, { - if std::mem::size_of::() != 0 { - Self::closures_unsupported_panic(); - } let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithoutEnv { func: Box::new(func), function_type: ty.clone(), @@ -138,9 +136,6 @@ impl Function { F: Fn(&Env, &[Val]) -> Result, RuntimeError> + 'static, Env: Sized + 'static, { - if std::mem::size_of::() != 0 { - Self::closures_unsupported_panic(); - } let dynamic_ctx = VMDynamicFunctionContext::from_context(VMDynamicFunctionWithEnv { env: Box::new(env), func: Box::new(func), @@ -642,7 +637,7 @@ impl Function { } fn closures_unsupported_panic() -> ! { - unimplemented!("Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840") + unimplemented!("Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840") } } diff --git a/tests/compilers/native_functions.rs b/tests/compilers/native_functions.rs index 66085327eb8..5c7d22b81b2 100644 --- a/tests/compilers/native_functions.rs +++ b/tests/compilers/native_functions.rs @@ -80,36 +80,7 @@ fn native_function_works_for_wasm() -> Result<()> { #[test] #[should_panic( - expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" -)] -fn host_function_closure_panics() { - let store = get_store(false); - let state = 3; - let ty = FunctionType::new(vec![], vec![]); - Function::new(&store, &ty, move |_args| { - println!("{}", state); - Ok(vec![]) - }); -} - -#[test] -#[should_panic( - expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" -)] -fn env_host_function_closure_panics() { - let store = get_store(false); - let state = 3; - let ty = FunctionType::new(vec![], vec![]); - let env = 4; - Function::new_with_env(&store, &ty, env, move |_env, _args| { - println!("{}", state); - Ok(vec![]) - }); -} - -#[test] -#[should_panic( - expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" + expected = "Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840" )] fn native_host_function_closure_panics() { let store = get_store(false); @@ -121,7 +92,7 @@ fn native_host_function_closure_panics() { #[test] #[should_panic( - expected = "Closures (functions with captured environments) are currently unsupported. See: https://github.com/wasmerio/wasmer/issues/1840" + expected = "Closures (functions with captured environments) are currently unsupported with native functions. See: https://github.com/wasmerio/wasmer/issues/1840" )] fn native_with_env_host_function_closure_panics() { let store = get_store(false); @@ -156,18 +127,19 @@ fn lambdas_with_no_env_work() -> Result<()> { let ty = FunctionType::new(vec![Type::I32, Type::I32], vec![Type::I32]); let env = 10; + let captured_by_closure = 20; let import_object = imports! { "env" => { - "multiply1" => Function::new(&store, &ty, |args| { + "multiply1" => Function::new(&store, &ty, move |args| { if let (Value::I32(v1), Value::I32(v2)) = (&args[0], &args[1]) { - Ok(vec![Value::I32(v1 * v2)]) + Ok(vec![Value::I32(v1 * v2 * captured_by_closure)]) } else { panic!("Invalid arguments"); } }), - "multiply2" => Function::new_with_env(&store, &ty, env, |&env, args| { + "multiply2" => Function::new_with_env(&store, &ty, env, move |&env, args| { if let (Value::I32(v1), Value::I32(v2)) = (&args[0], &args[1]) { - Ok(vec![Value::I32(v1 * v2 * env)]) + Ok(vec![Value::I32(v1 * v2 * captured_by_closure * env)]) } else { panic!("Invalid arguments"); } @@ -185,7 +157,7 @@ fn lambdas_with_no_env_work() -> Result<()> { instance.exports.get_native_function("test")?; let result = test.call(2, 3, 4, 5, 6)?; - let manually_computed_result = 6 * (5 * (4 * (3 * 2) * 10)) * 10; + let manually_computed_result = 6 * (5 * (4 * (3 * 2 * 20) * 10 * 20)) * 10; assert_eq!(result, manually_computed_result); Ok(()) } From 081af6428eaced9b51294268d4e5e86e217f2242 Mon Sep 17 00:00:00 2001 From: Syrus Akbary Date: Mon, 30 Nov 2020 14:09:13 -0500 Subject: [PATCH 3/5] Update tests/compilers/native_functions.rs Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> --- tests/compilers/native_functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/compilers/native_functions.rs b/tests/compilers/native_functions.rs index 5c7d22b81b2..91eac643513 100644 --- a/tests/compilers/native_functions.rs +++ b/tests/compilers/native_functions.rs @@ -104,7 +104,7 @@ fn native_with_env_host_function_closure_panics() { } #[test] -fn lambdas_with_no_env_work() -> Result<()> { +fn non_native_functions_and_closures_with_no_env_work() -> Result<()> { let store = get_store(false); let wat = r#"(module (func $multiply1 (import "env" "multiply1") (param i32 i32) (result i32)) From f583263f1da9b6c5529972f34bf72692062ce9ad Mon Sep 17 00:00:00 2001 From: Syrus Akbary Date: Mon, 30 Nov 2020 14:09:22 -0500 Subject: [PATCH 4/5] Update CHANGELOG.md Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a42806eda50..2ea497fe669 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ ### Fixed -- [#1841](https://github.com/wasmerio/wasmer/pull/1841) We will now panic when attempting to use a function with a captured env as a host function. Previously this would silently do the wrong thing. See [#1840](https://github.com/wasmerio/wasmer/pull/1840) for info about Wasmer's support of closures as host functions. +- [#1841](https://github.com/wasmerio/wasmer/pull/1841) We will now panic when attempting to use a native function with a captured env as a host function. Previously this would silently do the wrong thing. See [#1840](https://github.com/wasmerio/wasmer/pull/1840) for info about Wasmer's support of closures as host functions. - [#1764](https://github.com/wasmerio/wasmer/pull/1764) Fix bug in WASI `path_rename` allowing renamed files to be 1 directory below a preopened directory. ## 1.0.0-alpha5 - 2020-11-06 From e64adea1d42a911fd1280e3f371bf1d32a1911f5 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Mon, 30 Nov 2020 15:11:36 -0800 Subject: [PATCH 5/5] Fix test by changing doc comment in deprecated api func macro --- lib/deprecated/runtime-core/src/functional_api.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/deprecated/runtime-core/src/functional_api.rs b/lib/deprecated/runtime-core/src/functional_api.rs index 793991b2168..0b47e023a16 100644 --- a/lib/deprecated/runtime-core/src/functional_api.rs +++ b/lib/deprecated/runtime-core/src/functional_api.rs @@ -78,9 +78,7 @@ pub fn validate(bytes: &[u8]) -> bool { /// /// # Usage /// -/// Function pointers or closures are supported. Closures can capture -/// their environment (with `move`). The first parameter is of kind -/// `vm::Ctx`. +/// Function pointers are supported. The first parameter is of kind `vm::Ctx`. /// /// ``` /// # use wasmer_runtime_core::{imports, func, vm}; @@ -90,15 +88,12 @@ pub fn validate(bytes: &[u8]) -> bool { /// n /// } /// -/// let i = 7; -/// /// let import_object = imports! { /// "env" => { /// "foo" => func!(func), -/// // A closure with a captured environment. -/// "bar" => func!(move |_: &mut vm::Ctx, n: i32| -> i32 { -/// n + i -/// }), +/// "bar" => func!(|_: &mut vm::Ctx, n: i32| -> i32 { +/// n + 7 +/// }), /// }, /// }; /// ```