test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

Hywan · 2020-12-17T10:41:29Z

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

Add a short description of the the change to the CHANGELOG.md file

In `wasm.h`, `wasm_functype_t` is implemented with `WASM_DECLARE_TYPE`, so with `WASM_DECLARE_VEC(functype, *)`. This `*` means the C struct for the vector is defined: ```c struct wasm_functype_vec_t { size_t size; wasm_functype_t** data; } ``` The way we implement `wasm_functype_vec_t` in Rust is with the `wasm_declare_vec!` macro. And it is wrong. We must use `wasm_declared_boxed_vec!`.

In `wasm.h`, `wasm_globaltype_t` is implemented with `WASM_DECLARE_TYPE`, so with `WASM_DECLARE_VEC(globaltype, *)`. This `*` means the C struct for the vector is defined: ```c struct wasm_globaltype_vec_t { size_t size; wasm_globaltype_t** data; } ``` The way we implement `wasm_globaltype_vec_t` in Rust is with the `wasm_declare_vec!` macro. And it is wrong. We must use `wasm_declared_boxed_vec!`.

In `wasm.h`, `wasm_memorytype_t` is implemented with `WASM_DECLARE_TYPE`, so with `WASM_DECLARE_VEC(memorytype, *)`. This `*` means the C struct for the vector is defined: ```c struct wasm_memorytype_vec_t { size_t size; wasm_memorytype_t** data; } ``` The way we implement `wasm_memorytype_vec_t` in Rust is with the `wasm_declare_vec!` macro. And it is wrong. We must use `wasm_declared_boxed_vec!`.

In `wasm.h`, `wasm_tabletype_t` is implemented with `WASM_DECLARE_TYPE`, so with `WASM_DECLARE_VEC(tabletype, *)`. This `*` means the C struct for the vector is defined: ```c struct wasm_tabletype_vec_t { size_t size; wasm_tabletype_t** data; } ``` The way we implement `wasm_tabletype_vec_t` in Rust is with the `wasm_declare_vec!` macro. And it is wrong. We must use `wasm_declared_boxed_vec!`.

In `wasm.h`, `wasm_frame_t` is implemented with `WASM_DECLARE_TYPE`, so with `WASM_DECLARE_VEC(frame, *)`. This `*` means the C struct for the vector is defined: ```c struct wasm_frame_vec_t { size_t size; wasm_frame_t** data; } ``` The way we implement `wasm_frame_vec_t` in Rust is with the `wasm_declare_vec!` macro. And it is wrong. We must use `wasm_declared_boxed_vec!`.

In case of a boxed vector, `wasm_$name_vec_delete` now checks that the vec is correctly initialized (by checking the first item only) because transmuting `Vec<*mut T>` to `Vec<Box<T>>`, otherwise it will crash.

…prove doc.

Hywan · 2020-12-17T14:05:59Z

lib/c-api/src/wasm_c_api/macros.rs

@@ -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>] {


Such implementation exists for regular vecs. Now it exists for boxed vecs too.

Hywan · 2020-12-17T14:06:47Z

lib/c-api/src/wasm_c_api/macros.rs

+                    // If the vector has been initialized (we check
+                    // only the first item), we can transmute items to
+                    // `Box`es.
+                    if vec.size > 0 && !data[0].is_null() {
+                        let _data: Vec<Box<[<wasm_ $name _t>]>> = ::std::mem::transmute(data);
+                    }


This changes allows to delete an uninitialized boxed vec. Should we check for all items or checking the first is safe enough?

For handling uninitialized, I think we just have to make a representation of uninit that makes sense... I think the code as-is will break in real code (for example partially initialized vectors. I'll get back to you with more details after finishing the review and looking at the code locally

Hywan · 2020-12-17T14:11:45Z

bors try

bors · 2020-12-17T14:35:31Z

try

Build failed:

Test on macos-x64

Hywan · 2020-12-17T15:57:18Z

Too bad, it's working on aarch64-apple-darwin, but not on x86_64…

Hywan · 2020-12-17T16:11:49Z

Following Vec::with_capacity, we are redirected to RawVec::with_capacity. If we continue the lead, we reach RawVec::with_capacity_in:

https://github.com/rust-lang/rust/blob/e261649593cf9c2707f7b30a61c46c4469c67ebb/library/alloc/src/raw_vec.rs#L128-L130

Notice the difference with RawVec::with_capacity_zeroed_in:

https://github.com/rust-lang/rust/blob/e261649593cf9c2707f7b30a61c46c4469c67ebb/library/alloc/src/raw_vec.rs#L135-L137

Ideally, the wasm_$name_vec_new_uninitialized should zeroed the items, for safety reasons. Otherwise, this change 8aa0822 will not work because we have no guarantee that the items are zeroed. This API is public for RawVec; it is only used by slice::new_zeroed_slice, which is unstable and behind a feature flag.

We could probably just used vec![ptr::null(); len]: yes the vector will allocate memory immediately and everytime, but they are all aimed to be filled one day.

lib/c-api/src/wasm_c_api/macros.rs

MarkMcCaskey · 2020-12-17T17:33:32Z

lib/c-api/src/wasm_c_api/macros.rs

+                    // If the vector has been initialized (we check
+                    // only the first item), we can transmute items to
+                    // `Box`es.
+                    if vec.size > 0 && !data[0].is_null() {
+                        let _data: Vec<Box<[<wasm_ $name _t>]>> = ::std::mem::transmute(data);
+                    }


For handling uninitialized, I think we just have to make a representation of uninit that makes sense... I think the code as-is will break in real code (for example partially initialized vectors. I'll get back to you with more details after finishing the review and looking at the code locally

MarkMcCaskey · 2020-12-17T17:38:30Z

Too bad, it's working on aarch64-apple-darwin, but not on x86_64…

How do I reproduce the failure? Locally make test-capi-cranelift-jit is passing on x86_64 macos

…n<&mut T>`. This was already the case for regular vec. This patch applies the same pattern for boxed vec. See wasmerio@deec77d.

It's a safer way to handle partially uninitialized boxed vector, since it protects against based deletion for every item.

Hywan · 2020-12-18T10:44:33Z

To handle partially uninitialized boxed vector, I propose to transmute the Vec<*mut T> to Vec<Option<Box<T>>> so that it handles safely all items individually. The corresponding commit is a68a1e6.

This patch updates how `wasm_$name_vec_new_uninitialized` creates the vector. The vector is now fully allocated with `null` pointer for each item, instead of having an empty vector with the initial capacity set to `length`. That way, we are sure the vector is zeroed correctly.

Hywan · 2020-12-18T10:48:47Z

bors try

bors · 2020-12-18T11:18:37Z

try

Build succeeded:

MarkMcCaskey

I have some other small comments but it overlaps with #1944, and I can change them there!

MarkMcCaskey · 2020-12-18T16:23:48Z

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

+    wasm_valtype_vec_delete(Some(params.as_mut()));
+    wasm_valtype_vec_delete(Some(results.as_mut()));
+
+    mem::forget(params);
+    mem::forget(results);


Ideally, we'd have Drop implemented and it'd do this for us

Hywan · 2020-12-18T16:36:09Z

bors r+

bors · 2020-12-18T17:05:18Z

Build succeeded:

1953: Get `multi.c` working in the Wasm C API r=MarkMcCaskey a=MarkMcCaskey Gets the `multi.c` example working. This PR has significant overlap with #1949 due to changes in vec / boxed vec. Changes in this PR: - impl `Clone` more often, implement `*_copy` functions in terms of `Clone` - clean up boxed vec behavior - add `*_vec_copy` functions for boxed and unboxed versions - fix a typo in `wasm-c-api/examples/multi.c`; fixed in the upstream Wasm C API here: WebAssembly/wasm-c-api#163 - additional misc improvements # Review - [ ] 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]>

test+doc(c-api) Continue to improve test coverage of the C API.

a4effaa

Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests 📚 documentation Do you like to read? labels Dec 17, 2020

Hywan self-assigned this Dec 17, 2020

Hywan added 9 commits December 17, 2020 13:51

feat(c-api) Implement From<&[T]> for wasm_$name_vec_t for boxed vec.

43026e6

feat(c-api) Simplify code by using new conversion implementations.

940dea7

feat(c-api) wasm_$name_vec_delete checks the vec is initialized.

8aa0822

In case of a boxed vector, `wasm_$name_vec_delete` now checks that the vec is correctly initialized (by checking the first item only) because transmuting `Vec<*mut T>` to `Vec<Box<T>>`, otherwise it will crash.

test+doc(c-api) Improve test coverage for the macros module, and im…

96169de

…prove doc.

Hywan changed the title ~~test+doc(c-api) Continue to improve test coverage of the C API~~ test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vec Dec 17, 2020

Hywan changed the title ~~test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vec~~ test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs Dec 17, 2020

Hywan commented Dec 17, 2020

View reviewed changes

Hywan marked this pull request as ready for review December 17, 2020 14:07

Hywan requested review from jubianchi and MarkMcCaskey as code owners December 17, 2020 14:07

Hywan added 2 commits December 17, 2020 15:08

Merge branch 'master' into doc-c-api-2

eb03f18

doc(changelog) Add wasmerio#1949.

2853bd4

bors bot added a commit that referenced this pull request Dec 17, 2020

Try #1949:

0aaeece

Merge branch 'master' into doc-c-api-2

272b9d1

MarkMcCaskey reviewed Dec 17, 2020

View reviewed changes

MarkMcCaskey mentioned this pull request Dec 17, 2020

Get multi.c working in the Wasm C API #1953

Merged

1 task

Hywan added 3 commits December 18, 2020 10:45

feat(c-api) wasm_$name_vec_delete for boxed vec now takes an `Optio…

51fe219

…n<&mut T>`. This was already the case for regular vec. This patch applies the same pattern for boxed vec. See wasmerio@deec77d.

feat(c-api) Transmute boxed vecs to Vec<Option<Box<T>>> when deleting.

a68a1e6

It's a safer way to handle partially uninitialized boxed vector, since it protects against based deletion for every item.

fix(c-api) UPdate wasm_functype_new according to previous commit.

40fa9c0

bors bot added a commit that referenced this pull request Dec 18, 2020

Try #1949:

2755b14

MarkMcCaskey approved these changes Dec 18, 2020

View reviewed changes

fix(c-api) Remove Box from wasm_functype_new.

b9afb6e

bors bot merged commit 625e110 into wasmerio:master Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

Hywan commented Dec 17, 2020 •

edited

Loading

Hywan Dec 17, 2020

Hywan Dec 17, 2020

MarkMcCaskey Dec 17, 2020

Hywan commented Dec 17, 2020

bors bot commented Dec 17, 2020

Hywan commented Dec 17, 2020

Hywan commented Dec 17, 2020

MarkMcCaskey Dec 17, 2020

MarkMcCaskey commented Dec 17, 2020

Hywan commented Dec 18, 2020

Hywan commented Dec 18, 2020

bors bot commented Dec 18, 2020

MarkMcCaskey left a comment

MarkMcCaskey Dec 18, 2020

Hywan commented Dec 18, 2020

bors bot commented Dec 18, 2020

test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

test+doc(c-api) Fix vecs that must be boxed vecs + don't transmute uninitialized boxed vecs #1949

Conversation

Hywan commented Dec 17, 2020 • edited Loading

Description

Review

Hywan Dec 17, 2020

Choose a reason for hiding this comment

Hywan Dec 17, 2020

Choose a reason for hiding this comment

MarkMcCaskey Dec 17, 2020

Choose a reason for hiding this comment

Hywan commented Dec 17, 2020

bors bot commented Dec 17, 2020

try

Hywan commented Dec 17, 2020

Hywan commented Dec 17, 2020

MarkMcCaskey Dec 17, 2020

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 17, 2020

Hywan commented Dec 18, 2020

Hywan commented Dec 18, 2020

bors bot commented Dec 18, 2020

try

MarkMcCaskey left a comment

Choose a reason for hiding this comment

MarkMcCaskey Dec 18, 2020

Choose a reason for hiding this comment

Hywan commented Dec 18, 2020

bors bot commented Dec 18, 2020

Hywan commented Dec 17, 2020 •

edited

Loading