From 8516d07c62ca65968cdb976789795f4bf4f3a27a Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Tue, 23 Mar 2021 13:28:59 -0700 Subject: [PATCH 1/5] Fix memory leak in C API in some uses of `wasm_name_t` Converting from `String` means that the type should auto-destruct to make sure we free that memory. The memory can still be freed manually though. We need thorough review for the changes were the `owned_wasm_name_t` is coming from user code. Is it guaranteed that `wasm_name_t` coming from the user always uses a host allocation? I don't think so, in which case, we have to find some way to handle transfer of ownership where it's unclear who owns it... --- lib/c-api/src/wasm_c_api/types/export.rs | 10 +++--- lib/c-api/src/wasm_c_api/types/import.rs | 18 +++++------ lib/c-api/src/wasm_c_api/types/mod.rs | 39 +++++++++++++++++++++-- lib/c-api/src/wasm_c_api/unstable/wasi.rs | 13 +++++--- 4 files changed, 58 insertions(+), 22 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/types/export.rs b/lib/c-api/src/wasm_c_api/types/export.rs index 6a78805cb..a1088ed11 100644 --- a/lib/c-api/src/wasm_c_api/types/export.rs +++ b/lib/c-api/src/wasm_c_api/types/export.rs @@ -1,10 +1,10 @@ -use super::{wasm_externtype_t, wasm_name_t}; +use super::{owned_wasm_name_t, wasm_externtype_t, wasm_name_t}; use wasmer::ExportType; #[allow(non_camel_case_types)] #[derive(Clone)] pub struct wasm_exporttype_t { - name: Box, + name: Box, extern_type: Box, } @@ -12,7 +12,7 @@ wasm_declare_boxed_vec!(exporttype); #[no_mangle] pub extern "C" fn wasm_exporttype_new( - name: Option>, + name: Option>, extern_type: Option>, ) -> Option> { Some(Box::new(wasm_exporttype_t { @@ -23,7 +23,7 @@ pub extern "C" fn wasm_exporttype_new( #[no_mangle] pub extern "C" fn wasm_exporttype_name(export_type: &wasm_exporttype_t) -> &wasm_name_t { - export_type.name.as_ref() + export_type.name.as_ref().as_ref() } #[no_mangle] @@ -42,7 +42,7 @@ impl From for wasm_exporttype_t { impl From<&ExportType> for wasm_exporttype_t { fn from(other: &ExportType) -> Self { - let name: Box = Box::new(other.name().to_string().into()); + let name: Box = Box::new(other.name().to_string().into()); let extern_type: Box = Box::new(other.ty().into()); wasm_exporttype_t { name, extern_type } diff --git a/lib/c-api/src/wasm_c_api/types/import.rs b/lib/c-api/src/wasm_c_api/types/import.rs index 9d060584d..79d75ff99 100644 --- a/lib/c-api/src/wasm_c_api/types/import.rs +++ b/lib/c-api/src/wasm_c_api/types/import.rs @@ -1,11 +1,11 @@ -use super::{wasm_externtype_t, wasm_name_t}; +use super::{owned_wasm_name_t, wasm_externtype_t, wasm_name_t}; use wasmer::ImportType; #[allow(non_camel_case_types)] #[derive(Clone)] pub struct wasm_importtype_t { - module: Box, - name: Box, + module: Box, + name: Box, extern_type: Box, } @@ -13,8 +13,8 @@ wasm_declare_boxed_vec!(importtype); #[no_mangle] pub extern "C" fn wasm_importtype_new( - module: Option>, - name: Option>, + module: Option>, + name: Option>, extern_type: Option>, ) -> Option> { Some(Box::new(wasm_importtype_t { @@ -26,12 +26,12 @@ pub extern "C" fn wasm_importtype_new( #[no_mangle] pub extern "C" fn wasm_importtype_module(import_type: &wasm_importtype_t) -> &wasm_name_t { - import_type.module.as_ref() + import_type.module.as_ref().as_ref() } #[no_mangle] pub extern "C" fn wasm_importtype_name(import_type: &wasm_importtype_t) -> &wasm_name_t { - import_type.name.as_ref() + import_type.name.as_ref().as_ref() } #[no_mangle] @@ -50,8 +50,8 @@ impl From for wasm_importtype_t { impl From<&ImportType> for wasm_importtype_t { fn from(other: &ImportType) -> Self { - let module: Box = Box::new(other.module().to_string().into()); - let name: Box = Box::new(other.name().to_string().into()); + let module: Box = Box::new(other.module().to_string().into()); + let name: Box = Box::new(other.name().to_string().into()); let extern_type: Box = Box::new(other.ty().into()); wasm_importtype_t { diff --git a/lib/c-api/src/wasm_c_api/types/mod.rs b/lib/c-api/src/wasm_c_api/types/mod.rs index c022ba17e..a1f7c5158 100644 --- a/lib/c-api/src/wasm_c_api/types/mod.rs +++ b/lib/c-api/src/wasm_c_api/types/mod.rs @@ -28,16 +28,49 @@ wasm_declare_vec!(byte); #[allow(non_camel_case_types)] pub type wasm_name_t = wasm_byte_vec_t; -impl From for wasm_name_t { +impl AsRef for wasm_name_t { + fn as_ref(&self) -> &wasm_name_t { + &self + } +} + +/// An owned version of `wasm_name_t`. +/// +/// Assumes that data is either valid host-owned or null. +// NOTE: `wasm_name_t` already does a deep copy, so we just derive `Clone` here. +#[derive(Debug, Clone)] +#[repr(transparent)] +#[allow(non_camel_case_types)] +pub struct owned_wasm_name_t(wasm_name_t); + +impl Drop for owned_wasm_name_t { + fn drop(&mut self) { + if !self.0.data.is_null() { + let _v = unsafe { Vec::from_raw_parts(self.0.data, self.0.size, self.0.size) }; + self.0.data = std::ptr::null_mut(); + self.0.size = 0; + } + // why can't we call this function? + //unsafe { crate::wasm_c_api::macros::wasm_byte_vec_delete(Some(self.0)) } + } +} + +impl AsRef for owned_wasm_name_t { + fn as_ref(&self) -> &wasm_name_t { + &self.0 + } +} + +impl From for owned_wasm_name_t { fn from(string: String) -> Self { let mut boxed_str: Box = string.into_boxed_str(); let data = boxed_str.as_mut_ptr(); let size = boxed_str.bytes().len(); - let wasm_name = Self { data, size }; + let wasm_name = wasm_name_t { data, size }; Box::leak(boxed_str); - wasm_name + Self(wasm_name) } } diff --git a/lib/c-api/src/wasm_c_api/unstable/wasi.rs b/lib/c-api/src/wasm_c_api/unstable/wasi.rs index 8eb0a5fa2..fdd842020 100644 --- a/lib/c-api/src/wasm_c_api/unstable/wasi.rs +++ b/lib/c-api/src/wasm_c_api/unstable/wasi.rs @@ -2,7 +2,10 @@ //! API. use super::super::{ - externals::wasm_extern_t, module::wasm_module_t, store::wasm_store_t, types::wasm_name_t, + externals::wasm_extern_t, + module::wasm_module_t, + store::wasm_store_t, + types::{owned_wasm_name_t, wasm_name_t}, wasi::wasi_env_t, }; use crate::error::CApiError; @@ -19,8 +22,8 @@ use wasmer_wasi::{generate_import_object_from_env, get_wasi_version}; #[allow(non_camel_case_types)] #[derive(Clone)] pub struct wasmer_named_extern_t { - module: Box, - name: Box, + module: Box, + name: Box, r#extern: Box, } @@ -118,7 +121,7 @@ mod __cbindgen_hack__ { pub extern "C" fn wasmer_named_extern_module( named_extern: Option<&wasmer_named_extern_t>, ) -> Option<&wasm_name_t> { - Some(named_extern?.module.as_ref()) + Some(named_extern?.module.as_ref().as_ref()) } /// Non-standard function to get the name of a `wasmer_named_extern_t`. @@ -128,7 +131,7 @@ pub extern "C" fn wasmer_named_extern_module( pub extern "C" fn wasmer_named_extern_name( named_extern: Option<&wasmer_named_extern_t>, ) -> Option<&wasm_name_t> { - Some(named_extern?.name.as_ref()) + Some(named_extern?.name.as_ref().as_ref()) } /// Non-standard function to get the wrapped extern of a From 769707a51d2a85753745984cca1d1e77d853a079 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 25 Mar 2021 10:55:26 -0700 Subject: [PATCH 2/5] Explicitly convert to `owned_wasm_name_t` --- lib/c-api/src/wasm_c_api/types/import.rs | 23 +++++++++++++++-------- lib/c-api/src/wasm_c_api/types/mod.rs | 14 ++++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/types/import.rs b/lib/c-api/src/wasm_c_api/types/import.rs index 79d75ff99..d29395ebf 100644 --- a/lib/c-api/src/wasm_c_api/types/import.rs +++ b/lib/c-api/src/wasm_c_api/types/import.rs @@ -3,9 +3,10 @@ use wasmer::ImportType; #[allow(non_camel_case_types)] #[derive(Clone)] +#[repr(C)] pub struct wasm_importtype_t { - module: Box, - name: Box, + module: owned_wasm_name_t, + name: owned_wasm_name_t, extern_type: Box, } @@ -13,13 +14,19 @@ wasm_declare_boxed_vec!(importtype); #[no_mangle] pub extern "C" fn wasm_importtype_new( - module: Option>, - name: Option>, + module: Option<&wasm_name_t>, + name: Option<&wasm_name_t>, extern_type: Option>, ) -> Option> { + let (module, name) = unsafe { + ( + owned_wasm_name_t::new(module?), + owned_wasm_name_t::new(name?), + ) + }; Some(Box::new(wasm_importtype_t { - name: name?, - module: module?, + name, + module, extern_type: extern_type?, })) } @@ -50,8 +57,8 @@ impl From for wasm_importtype_t { impl From<&ImportType> for wasm_importtype_t { fn from(other: &ImportType) -> Self { - let module: Box = Box::new(other.module().to_string().into()); - let name: Box = Box::new(other.name().to_string().into()); + let module: owned_wasm_name_t = other.module().to_string().into(); + let name: owned_wasm_name_t = other.name().to_string().into(); let extern_type: Box = Box::new(other.ty().into()); wasm_importtype_t { diff --git a/lib/c-api/src/wasm_c_api/types/mod.rs b/lib/c-api/src/wasm_c_api/types/mod.rs index a1f7c5158..541747045 100644 --- a/lib/c-api/src/wasm_c_api/types/mod.rs +++ b/lib/c-api/src/wasm_c_api/types/mod.rs @@ -43,6 +43,20 @@ impl AsRef for wasm_name_t { #[allow(non_camel_case_types)] pub struct owned_wasm_name_t(wasm_name_t); +impl owned_wasm_name_t { + /// Take ownership of some `wasm_name_t` + /// + /// # Safety + /// You must ensure that the data pointed to by `wasm_name_t` is valid and + /// that it is not owned by anyone else. + pub unsafe fn new(name: &wasm_name_t) -> Self { + Self(wasm_name_t { + size: name.size, + data: name.data, + }) + } +} + impl Drop for owned_wasm_name_t { fn drop(&mut self) { if !self.0.data.is_null() { From e3eb1b6ad1f26bd509e28160a779652ab50dd385 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 25 Mar 2021 12:46:43 -0700 Subject: [PATCH 3/5] Explicitly convert to `owned_wasm_name_t` in export and wasi too --- lib/c-api/src/wasm_c_api/types/export.rs | 11 ++++++----- lib/c-api/src/wasm_c_api/unstable/wasi.rs | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/types/export.rs b/lib/c-api/src/wasm_c_api/types/export.rs index a1088ed11..a2d94d3c0 100644 --- a/lib/c-api/src/wasm_c_api/types/export.rs +++ b/lib/c-api/src/wasm_c_api/types/export.rs @@ -4,7 +4,7 @@ use wasmer::ExportType; #[allow(non_camel_case_types)] #[derive(Clone)] pub struct wasm_exporttype_t { - name: Box, + name: owned_wasm_name_t, extern_type: Box, } @@ -12,18 +12,19 @@ wasm_declare_boxed_vec!(exporttype); #[no_mangle] pub extern "C" fn wasm_exporttype_new( - name: Option>, + name: Option<&wasm_name_t>, extern_type: Option>, ) -> Option> { + let name = unsafe { owned_wasm_name_t::new(name?) }; Some(Box::new(wasm_exporttype_t { - name: name?, + name, extern_type: extern_type?, })) } #[no_mangle] pub extern "C" fn wasm_exporttype_name(export_type: &wasm_exporttype_t) -> &wasm_name_t { - export_type.name.as_ref().as_ref() + export_type.name.as_ref() } #[no_mangle] @@ -42,7 +43,7 @@ impl From for wasm_exporttype_t { impl From<&ExportType> for wasm_exporttype_t { fn from(other: &ExportType) -> Self { - let name: Box = Box::new(other.name().to_string().into()); + let name: owned_wasm_name_t = other.name().to_string().into(); let extern_type: Box = Box::new(other.ty().into()); wasm_exporttype_t { name, extern_type } diff --git a/lib/c-api/src/wasm_c_api/unstable/wasi.rs b/lib/c-api/src/wasm_c_api/unstable/wasi.rs index fdd842020..66361b6f7 100644 --- a/lib/c-api/src/wasm_c_api/unstable/wasi.rs +++ b/lib/c-api/src/wasm_c_api/unstable/wasi.rs @@ -22,8 +22,8 @@ use wasmer_wasi::{generate_import_object_from_env, get_wasi_version}; #[allow(non_camel_case_types)] #[derive(Clone)] pub struct wasmer_named_extern_t { - module: Box, - name: Box, + module: owned_wasm_name_t, + name: owned_wasm_name_t, r#extern: Box, } @@ -182,8 +182,8 @@ fn wasi_get_unordered_imports_inner( *imports = import_object .into_iter() .map(|((module, name), export)| { - let module = Box::new(module.into()); - let name = Box::new(name.into()); + let module = module.into(); + let name = name.into(); let extern_inner = Extern::from_vm_export(store, export); Box::new(wasmer_named_extern_t { From 7ef7bf0c481374d3300459e147175a94ee5d1d99 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 25 Mar 2021 12:55:45 -0700 Subject: [PATCH 4/5] Simplify `as_ref()` code due to lack of Box --- lib/c-api/src/wasm_c_api/types/import.rs | 4 ++-- lib/c-api/src/wasm_c_api/unstable/wasi.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/types/import.rs b/lib/c-api/src/wasm_c_api/types/import.rs index d29395ebf..8e2e5361c 100644 --- a/lib/c-api/src/wasm_c_api/types/import.rs +++ b/lib/c-api/src/wasm_c_api/types/import.rs @@ -33,12 +33,12 @@ pub extern "C" fn wasm_importtype_new( #[no_mangle] pub extern "C" fn wasm_importtype_module(import_type: &wasm_importtype_t) -> &wasm_name_t { - import_type.module.as_ref().as_ref() + import_type.module.as_ref() } #[no_mangle] pub extern "C" fn wasm_importtype_name(import_type: &wasm_importtype_t) -> &wasm_name_t { - import_type.name.as_ref().as_ref() + import_type.name.as_ref() } #[no_mangle] diff --git a/lib/c-api/src/wasm_c_api/unstable/wasi.rs b/lib/c-api/src/wasm_c_api/unstable/wasi.rs index 66361b6f7..8fce17709 100644 --- a/lib/c-api/src/wasm_c_api/unstable/wasi.rs +++ b/lib/c-api/src/wasm_c_api/unstable/wasi.rs @@ -121,7 +121,7 @@ mod __cbindgen_hack__ { pub extern "C" fn wasmer_named_extern_module( named_extern: Option<&wasmer_named_extern_t>, ) -> Option<&wasm_name_t> { - Some(named_extern?.module.as_ref().as_ref()) + Some(named_extern?.module.as_ref()) } /// Non-standard function to get the name of a `wasmer_named_extern_t`. @@ -131,7 +131,7 @@ pub extern "C" fn wasmer_named_extern_module( pub extern "C" fn wasmer_named_extern_name( named_extern: Option<&wasmer_named_extern_t>, ) -> Option<&wasm_name_t> { - Some(named_extern?.name.as_ref().as_ref()) + Some(named_extern?.name.as_ref()) } /// Non-standard function to get the wrapped extern of a From 681dab78d18790edb95607538007724f165f7e6a Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Fri, 26 Mar 2021 08:02:07 -0700 Subject: [PATCH 5/5] Document Wasm C API memory leak fix in changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b9226707..a932aace7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ - [#2149](https://github.com/wasmerio/wasmer/pull/2144) `wasmer-engine-native` looks for clang-11 instead of clang-10. ### Fixed +- [#2210](https://github.com/wasmerio/wasmer/pull/2210) Fix a memory leak in the Wasm C API in the strings used to identify imports and exports coming from user code. - [#2108](https://github.com/wasmerio/wasmer/pull/2108) The Object Native Engine generates code that now compiles correctly with C++. - [#2125](https://github.com/wasmerio/wasmer/pull/2125) Fix RUSTSEC-2021-0023. - [#2155](https://github.com/wasmerio/wasmer/pull/2155) Fix the implementation of shift and rotate in the LLVM compiler.