Merge #2210

2210: Fix memory leak in C API in some uses of `wasm_name_t` r=MarkMcCaskey a=MarkMcCaskey

This PR fixes a bug relating to `wasm_importtype_t` leaking memory, particularly on the path where `String` is converted into `wasm_name_t`. It fixes this by introducing a new type, `owned_wasm_name_t`, which is identical to `wasm_name_t` but has RAII and calls the destructor if it goes out of scope.

I'm not confident that the use of `owned_wasm_name_t` on the FFI boundary is correct though, we'll have to thoroughly review that before shipping this PR.

# Review

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


Co-authored-by: Mark McCaskey <mark@wasmer.io>

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.

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<wasm_name_t>,
+    name: owned_wasm_name_t,
     extern_type: Box<wasm_externtype_t>,
 }
 
@@ -12,11 +12,12 @@ wasm_declare_boxed_vec!(exporttype);
 
 #[no_mangle]
 pub extern "C" fn wasm_exporttype_new(
-    name: Option<Box<wasm_name_t>>,
+    name: Option<&wasm_name_t>,
     extern_type: Option<Box<wasm_externtype_t>>,
 ) -> Option<Box<wasm_exporttype_t>> {
+    let name = unsafe { owned_wasm_name_t::new(name?) };
     Some(Box::new(wasm_exporttype_t {
-        name: name?,
+        name,
         extern_type: extern_type?,
     }))
 }
@@ -42,7 +43,7 @@ impl From<ExportType> for wasm_exporttype_t {
 
 impl From<&ExportType> for wasm_exporttype_t {
     fn from(other: &ExportType) -> Self {
-        let name: Box<wasm_name_t> = Box::new(other.name().to_string().into());
+        let name: owned_wasm_name_t = other.name().to_string().into();
         let extern_type: Box<wasm_externtype_t> = Box::new(other.ty().into());
 
         wasm_exporttype_t { name, extern_type }

lib/c-api/src/wasm_c_api/types/import.rs

@@ -1,11 +1,12 @@
-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)]
+#[repr(C)]
 pub struct wasm_importtype_t {
-    module: Box<wasm_name_t>,
+    module: owned_wasm_name_t,
-    name: Box<wasm_name_t>,
+    name: owned_wasm_name_t,
     extern_type: Box<wasm_externtype_t>,
 }
 
@@ -13,13 +14,19 @@ wasm_declare_boxed_vec!(importtype);
 
 #[no_mangle]
 pub extern "C" fn wasm_importtype_new(
-    module: Option<Box<wasm_name_t>>,
+    module: Option<&wasm_name_t>,
-    name: Option<Box<wasm_name_t>>,
+    name: Option<&wasm_name_t>,
     extern_type: Option<Box<wasm_externtype_t>>,
 ) -> Option<Box<wasm_importtype_t>> {
+    let (module, name) = unsafe {
+        (
+            owned_wasm_name_t::new(module?),
+            owned_wasm_name_t::new(name?),
+        )
+    };
     Some(Box::new(wasm_importtype_t {
-        name: name?,
+        name,
-        module: module?,
+        module,
         extern_type: extern_type?,
     }))
 }
@@ -50,8 +57,8 @@ impl From<ImportType> for wasm_importtype_t {
 
 impl From<&ImportType> for wasm_importtype_t {
     fn from(other: &ImportType) -> Self {
-        let module: Box<wasm_name_t> = Box::new(other.module().to_string().into());
+        let module: owned_wasm_name_t = other.module().to_string().into();
-        let name: Box<wasm_name_t> = Box::new(other.name().to_string().into());
+        let name: owned_wasm_name_t = other.name().to_string().into();
         let extern_type: Box<wasm_externtype_t> = Box::new(other.ty().into());
 
         wasm_importtype_t {

lib/c-api/src/wasm_c_api/types/mod.rs

@@ -28,16 +28,63 @@ wasm_declare_vec!(byte);
 #[allow(non_camel_case_types)]
 pub type wasm_name_t = wasm_byte_vec_t;
 
-impl From<String> for wasm_name_t {
+impl AsRef<wasm_name_t> 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 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() {
+            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<wasm_name_t> for owned_wasm_name_t {
+    fn as_ref(&self) -> &wasm_name_t {
+        &self.0
+    }
+}
+
+impl From<String> for owned_wasm_name_t {
     fn from(string: String) -> Self {
         let mut boxed_str: Box<str> = 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)
     }
 }
 

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<wasm_name_t>,
+    module: owned_wasm_name_t,
-    name: Box<wasm_name_t>,
+    name: owned_wasm_name_t,
     r#extern: Box<wasm_extern_t>,
 }
 
@@ -179,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 module = module.into();
-            let name = Box::new(name.into());
+            let name = name.into();
             let extern_inner = Extern::from_vm_export(store, export);
 
             Box::new(wasmer_named_extern_t {