Merge #1949

1949: test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs r=Hywan a=Hywan # Description This PR improve test coverage of the C API. ~It's a work in progress. I've paused this work because we need to focus on something else, but one of the two test cases that have been added reveals an important bug.~ The `wasm_frame_vec_t`, `wasm_functype_vec_t`, `wasm_globaltype_vec_t`, `wasm_memorytype_vec_t` and `wasm_tabletype_vec_t` are incorrectly defined. Indeed, in `wasm.h` they are all defined with `WASM_DECLARE_TYPE` (except for `wasm_frame_vec_t`, but it's exactly the same), which declares the vectors as `WAM_DECLARE_VEC(name, *)`. This `*` is important. It means that we _must not_ declare thoses types with our own `wasm_declare_vec!` macro, but rather with `wasm_declare_boxed_vec!`. ~Fixing this bug impact many places in the code. I didn't have time for the moment, but it's probably not a big deal!~ This PR fixes those bugs with e10ad51, 75ceb0d, a3c7a2d, a0c34fe. Bonus for `wasm_frame_t` that was also incorrectly implemented, the fix lands in 4abf6f8 and is possible thanks to 43026e6, with a side effect of simplifying the code in 940dea7. While continuing to improve the test coverage, another bug has been found. In case of a boxed vector, `wasm_$name_vec_delete` was crashing if the given vector was uninitialized. Basically the following code was crashing: ``` wasm_exporttype_vec_t vector; wasm_exporttype_vec_new_uninitialized(&vector, 3); wasm_exporttype_vec_delete(&vector); ``` This is now fixed in 8aa0822, and tested in 96169de. The PR adds 34 test cases, and has found+fixes 6 bugs. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Ivan Enderlin <ivan@mnt.io>
wasmerio · Dec 18, 2020 · 625e110 · 625e110
2 parents 8e34363 + b9afb6e
commit 625e110
Show file tree

Hide file tree

Showing 7 changed files with 182 additions and 69 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,9 @@
 
 ### Fixed
 
+- [#1949](https://github.com/wasmerio/wasmer/pull/1949) `wasm_<type>_vec_delete` functions no longer crash when the given vector is uninitialized, in the Wasmer C API
+- [#1949](https://github.com/wasmerio/wasmer/pull/1949) The `wasm_frame_vec_t`, `wasm_functype_vec_t`, `wasm_globaltype_vec_t`, `wasm_memorytype_vec_t`, and `wasm_tabletype_vec_t` are now boxed vectors in the Wasmer C API
+
 ## 1.0.0-beta2 - 2020-12-16
 
 ### Added

diff --git a/lib/c-api/src/wasm_c_api/macros.rs b/lib/c-api/src/wasm_c_api/macros.rs
@@ -3,8 +3,31 @@
 macro_rules! wasm_declare_vec_inner {
     ($name:ident) => {
         paste::paste! {
-            /// Creates an empty vector of
-            #[doc = "Creates an empty vector of [`wasm_" $name "_t`]."]
+            #[doc = "Creates an empty vector of [`wasm_" $name "_t`].
+
+# Example
+
+```rust
+# use inline_c::assert_c;
+# fn main() {
+#    (assert_c! {
+# #include \"tests/wasmer_wasm.h\"
+#
+int main() {
+    // Creates an empty vector of `wasm_" $name "_t`.
+    wasm_" $name "_vec_t vector;
+    wasm_" $name "_vec_new_empty(&vector);
+
+    // Check that it is empty.
+    assert(vector.size == 0);
+
+    // Free it.
+    wasm_" $name "_vec_delete(&vector);
+}
+#    })
+#    .success();
+# }
+```"]
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_new_empty>](out: *mut [<wasm_ $name _vec_t>]) {
                 // TODO: actually implement this
@@ -19,7 +42,37 @@ macro_rules! wasm_declare_vec_inner {
 macro_rules! wasm_declare_vec {
     ($name:ident) => {
         paste::paste! {
-            #[doc = "Represents of a vector of [`wasm_" $name "_t`]."]
+            #[doc = "Represents a vector of `wasm_" $name "_t`.
+
+Read the documentation of [`wasm_" $name "_t`] to see more concrete examples.
+
+# Example
+
+```rust
+# use inline_c::assert_c;
+# fn main() {
+#    (assert_c! {
+# #include \"tests/wasmer_wasm.h\"
+#
+int main() {
+    // Create a vector of 2 `wasm_" $name "_t`.
+    wasm_" $name "_t x;
+    wasm_" $name "_t y;
+    wasm_" $name "_t* items[2] = {&x, &y};
+
+    wasm_" $name "_vec_t vector;
+    wasm_" $name "_vec_new(&vector, 2, (wasm_" $name "_t*) items);
+
+    // Check that it contains 2 items.
+    assert(vector.size == 2);
+
+    // Free it.
+    wasm_" $name "_vec_delete(&vector);
+}
+#    })
+#    .success();
+# }
+```"]
             #[derive(Debug)]
             #[repr(C)]
             pub struct [<wasm_ $name _vec_t>] {
@@ -54,6 +107,7 @@ macro_rules! wasm_declare_vec {
                         .into_boxed_slice();
                     let data = copied_data.as_mut_ptr();
                     ::std::mem::forget(copied_data);
+
                     Self {
                         size,
                         data,
@@ -84,31 +138,68 @@ macro_rules! wasm_declare_vec {
             }
 
             // TODO: investigate possible memory leak on `init` (owned pointer)
-            #[doc = "Creates a new vector of [`wasm_" $name "_t`]."]
+            #[doc = "Creates a new vector of [`wasm_" $name "_t`].
+
+# Example
+
+See the [`wasm_" $name "_vec_t`] type to get an example."]
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_new>](out: *mut [<wasm_ $name _vec_t>], length: usize, init: *mut [<wasm_ $name _t>]) {
                 let mut bytes: Vec<[<wasm_ $name _t>]> = Vec::with_capacity(length);
+
                 for i in 0..length {
                     bytes.push(::std::ptr::read(init.add(i)));
                 }
+
                 let pointer = bytes.as_mut_ptr();
                 debug_assert!(bytes.len() == bytes.capacity());
+
                 (*out).data = pointer;
                 (*out).size = length;
                 ::std::mem::forget(bytes);
             }
 
-            #[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`]."]
+            #[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`].
+
+# Example
+
+```rust
+# use inline_c::assert_c;
+# fn main() {
+#    (assert_c! {
+# #include \"tests/wasmer_wasm.h\"
+#
+int main() {
+    // Creates an empty vector of `wasm_" $name "_t`.
+    wasm_" $name "_vec_t vector;
+    wasm_" $name "_vec_new_uninitialized(&vector, 3);
+
+    // Check that it contains 3 items.
+    assert(vector.size == 3);
+
+    // Free it.
+    wasm_" $name "_vec_delete(&vector);
+}
+#    })
+#    .success();
+# }
+```"]
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_new_uninitialized>](out: *mut [<wasm_ $name _vec_t>], length: usize) {
                 let mut bytes: Vec<[<wasm_ $name _t>]> = Vec::with_capacity(length);
                 let pointer = bytes.as_mut_ptr();
+
                 (*out).data = pointer;
                 (*out).size = length;
+
                 ::std::mem::forget(bytes);
             }
 
-            #[doc = "Deletes a vector of [`wasm_" $name "_t`]."]
+            #[doc = "Deletes a vector of [`wasm_" $name "_t`].
+
+# Example
+
+See the [`wasm_" $name "_vec_t`] type to get an example."]
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_delete>](ptr: Option<&mut [<wasm_ $name _vec_t>]>) {
                 if let Some(vec) = ptr {
@@ -130,7 +221,9 @@ macro_rules! wasm_declare_vec {
 macro_rules! wasm_declare_boxed_vec {
     ($name:ident) => {
         paste::paste! {
-            #[doc = "Represents of a vector of [`wasm_" $name "_t`]."]
+            #[doc = "Represents a vector of `wasm_" $name "_t`.
+
+Read the documentation of [`wasm_" $name "_t`] to see more concrete examples."]
             #[derive(Debug)]
             #[repr(C)]
             pub struct [<wasm_ $name _vec_t>] {
@@ -153,6 +246,27 @@ macro_rules! wasm_declare_boxed_vec {
                 }
             }
 
+            impl<'a, T: Into<[<wasm_ $name _t>]> + Clone> From<&'a [T]> for [<wasm_ $name _vec_t>] {
+                fn from(other: &'a [T]) -> Self {
+                    let size = other.len();
+                    let mut copied_data = other
+                        .iter()
+                        .cloned()
+                        .map(Into::into)
+                        .map(Box::new)
+                        .map(Box::into_raw)
+                        .collect::<Vec<*mut [<wasm_ $name _t>]>>()
+                        .into_boxed_slice();
+                    let data = copied_data.as_mut_ptr();
+                    ::std::mem::forget(copied_data);
+
+                    Self {
+                        size,
+                        data,
+                    }
+                }
+            }
+
             // TODO: do this properly
             impl [<wasm_ $name _vec_t>] {
                 pub unsafe fn into_slice(&self) -> Option<&[Box<[<wasm_ $name _t>]>]>{
@@ -171,35 +285,71 @@ macro_rules! wasm_declare_boxed_vec {
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_new>](out: *mut [<wasm_ $name _vec_t>], length: usize, init: *const *mut [<wasm_ $name _t>]) {
                 let mut bytes: Vec<*mut [<wasm_ $name _t>]> = Vec::with_capacity(length);
+
                 for i in 0..length {
                     bytes.push(*init.add(i));
                 }
+
                 let pointer = bytes.as_mut_ptr();
                 debug_assert!(bytes.len() == bytes.capacity());
+
                 (*out).data = pointer;
                 (*out).size = length;
+
                 ::std::mem::forget(bytes);
             }
 
-            #[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`]."]
+            #[doc = "Creates a new uninitialized vector of [`wasm_" $name "_t`].
+
+# Example
+
+```rust
+# use inline_c::assert_c;
+# fn main() {
+#    (assert_c! {
+# #include \"tests/wasmer_wasm.h\"
+#
+int main() {
+    // Creates an empty vector of `wasm_" $name "_t`.
+    wasm_" $name "_vec_t vector;
+    wasm_" $name "_vec_new_uninitialized(&vector, 3);
+
+    // Check that it contains 3 items.
+    assert(vector.size == 3);
+
+    // Free it.
+    wasm_" $name "_vec_delete(&vector);
+}
+#    })
+#    .success();
+# }
+```"]
             #[no_mangle]
             pub unsafe extern "C" fn [<wasm_ $name _vec_new_uninitialized>](out: *mut [<wasm_ $name _vec_t>], length: usize) {
-                let mut bytes: Vec<*mut [<wasm_ $name _t>]> = Vec::with_capacity(length);
+                let mut bytes: Vec<*mut [<wasm_ $name _t>]> = vec![::std::ptr::null_mut(); length];
                 let pointer = bytes.as_mut_ptr();
+
                 (*out).data = pointer;
                 (*out).size = length;
+
                 ::std::mem::forget(bytes);
             }
 
-            #[doc = "Deletes a vector of [`wasm_" $name "_t`]."]
+            #[doc = "Deletes a vector of [`wasm_" $name "_t`].
+
+# Example
+
+See the [`wasm_" $name "_vec_t`] type to get an example."]
             #[no_mangle]
-            pub unsafe extern "C" fn [<wasm_ $name _vec_delete>](ptr: *mut [<wasm_ $name _vec_t>]) {
-                let vec = &mut *ptr;
-                if !vec.data.is_null() {
-                    let data: Vec<*mut [<wasm_ $name _t>]> = Vec::from_raw_parts(vec.data, vec.size, vec.size);
-                    let _data: Vec<Box<[<wasm_ $name _t>]>> = ::std::mem::transmute(data);
-                    vec.data = ::std::ptr::null_mut();
-                    vec.size = 0;
+            pub unsafe extern "C" fn [<wasm_ $name _vec_delete>](ptr: Option<&mut [<wasm_ $name _vec_t>]>) {
+                if let Some(vec) = ptr {
+                    if !vec.data.is_null() {
+                        let data: Vec<*mut [<wasm_ $name _t>]> = Vec::from_raw_parts(vec.data, vec.size, vec.size);
+                        let _data: Vec<Option<Box<[<wasm_ $name _t>]>>> = ::std::mem::transmute(data);
+
+                        vec.data = ::std::ptr::null_mut();
+                        vec.size = 0;
+                    }
                 }
             }
         }
@@ -222,7 +372,6 @@ macro_rules! wasm_declare_ref_base {
             }
 
             // TODO: finish this...
-
         }
     };
 }

diff --git a/lib/c-api/src/wasm_c_api/types/frame.rs b/lib/c-api/src/wasm_c_api/types/frame.rs
@@ -48,4 +48,4 @@ pub unsafe extern "C" fn wasm_frame_module_offset(frame: &wasm_frame_t) -> usize
     frame.info.module_offset()
 }
 
-wasm_declare_vec!(frame);
+wasm_declare_boxed_vec!(frame);
diff --git a/lib/c-api/src/wasm_c_api/types/function.rs b/lib/c-api/src/wasm_c_api/types/function.rs
@@ -1,7 +1,4 @@
-use super::{
-    wasm_externtype_t, wasm_valtype_t, wasm_valtype_vec_delete, wasm_valtype_vec_t, WasmExternType,
-};
-use std::mem;
+use super::{wasm_externtype_t, wasm_valtype_vec_delete, wasm_valtype_vec_t, WasmExternType};
 use wasmer::{ExternType, FunctionType, ValType};
 
 #[derive(Debug)]
@@ -13,44 +10,8 @@ pub(crate) struct WasmFunctionType {
 
 impl WasmFunctionType {
     pub(crate) fn new(function_type: FunctionType) -> Self {
-        let params = {
-            let mut valtypes = function_type
-                .params()
-                .iter()
-                .cloned()
-                .map(Into::into)
-                .map(Box::new)
-                .map(Box::into_raw)
-                .collect::<Vec<*mut wasm_valtype_t>>();
-
-            let valtypes_vec = Box::new(wasm_valtype_vec_t {
-                size: valtypes.len(),
-                data: valtypes.as_mut_ptr(),
-            });
-
-            mem::forget(valtypes);
-
-            valtypes_vec
-        };
-        let results = {
-            let mut valtypes = function_type
-                .results()
-                .iter()
-                .cloned()
-                .map(Into::into)
-                .map(Box::new)
-                .map(Box::into_raw)
-                .collect::<Vec<*mut wasm_valtype_t>>();
-
-            let valtypes_vec = Box::new(wasm_valtype_vec_t {
-                size: valtypes.len(),
-                data: valtypes.as_mut_ptr(),
-            });
-
-            mem::forget(valtypes);
-
-            valtypes_vec
-        };
+        let params: Box<wasm_valtype_vec_t> = Box::new(function_type.params().into());
+        let results: Box<wasm_valtype_vec_t> = Box::new(function_type.results().into());
 
         Self {
             function_type,
@@ -90,12 +51,12 @@ impl wasm_functype_t {
     }
 }
 
-wasm_declare_vec!(functype);
+wasm_declare_boxed_vec!(functype);
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_functype_new(
-    params: Option<Box<wasm_valtype_vec_t>>,
-    results: Option<Box<wasm_valtype_vec_t>>,
+    params: Option<&mut wasm_valtype_vec_t>,
+    results: Option<&mut wasm_valtype_vec_t>,
 ) -> Option<Box<wasm_functype_t>> {
     let params = params?;
     let results = results?;
@@ -111,8 +72,8 @@ pub unsafe extern "C" fn wasm_functype_new(
         .map(|val| val.as_ref().into())
         .collect::<Vec<_>>();
 
-    wasm_valtype_vec_delete(Box::into_raw(params));
-    wasm_valtype_vec_delete(Box::into_raw(results));
+    wasm_valtype_vec_delete(Some(params));
+    wasm_valtype_vec_delete(Some(results));
 
     Some(Box::new(wasm_functype_t::new(FunctionType::new(
         params_as_valtype,

diff --git a/lib/c-api/src/wasm_c_api/types/global.rs b/lib/c-api/src/wasm_c_api/types/global.rs
@@ -46,7 +46,7 @@ impl wasm_globaltype_t {
     }
 }
 
-wasm_declare_vec!(globaltype);
+wasm_declare_boxed_vec!(globaltype);
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_globaltype_new(

diff --git a/lib/c-api/src/wasm_c_api/types/memory.rs b/lib/c-api/src/wasm_c_api/types/memory.rs
@@ -48,7 +48,7 @@ impl wasm_memorytype_t {
     }
 }
 
-wasm_declare_vec!(memorytype);
+wasm_declare_boxed_vec!(memorytype);
 
 #[no_mangle]
 pub unsafe extern "C" fn wasm_memorytype_new(limits: &wasm_limits_t) -> Box<wasm_memorytype_t> {