Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasm_name_new_from_string should not have +1 since rust strings are not null terminated. #2131

Closed
craigsteyn opened this issue Aug 13, 2020 · 2 comments · Fixed by #2579
Closed
Assignees
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@craigsteyn
Copy link

wasm_name_t nameWrong;
wasm_name_new_from_string(&nameWrong, "hello");
wasm_name_t nameRight;
wasm_name_new(&nameRight, strlen("hello"), "hello");

first one didnt work when adding to the linker.
second one did.
Issue was caused by wasm_name_new_from_string adding to the length.

@craigsteyn craigsteyn added the bug Incorrect behavior in the current implementation that needs fixing label Aug 13, 2020
@alexcrichton
Copy link
Member

Sorry for the delay in getting to this, but thanks for the report! This is a discrepancy with the upstream C API in https://github.com/WebAssembly/wasm-c-api. The problem is that some string lengths include the nul terminator and some do not. Most Wasmtime APIs do not take into account the nul terminator, except for those that the wasm-c-api specifies should do so. Depending on the constructor you use you get a +1 or not as well.

It's pretty confusing, but at this time the upstream C API needs to change to make the situation better I believe.

@peterhuene
Copy link
Member

peterhuene commented Aug 31, 2020

WebAssembly/wasm-c-api#151 was made to help distinguish the fact that this function (renamed to wasm_name_new_from_string_nt) will add a null terminator to the string, which makes it unsuitable for a use such as resolving module imports.

We should update our API to match the new name, at which time wasm_name_new_from_string should start working.

@peterhuene peterhuene self-assigned this Jan 13, 2021
peterhuene added a commit to peterhuene/wasmtime that referenced this issue Jan 13, 2021
This commit updates the WebAssembly C API submodule (for `wasm.h`) to the
latest commit out of master.

This fixes the behavior of `wasm_name_new_from_string` such that it no longer
copies the null character into the name, which caused unexpected failures when
using the Wasmtime linker as imports wouldn't resolve when the null was
present.

Along with this change were breaking changes to `wasm_func_call`, the host
callback signatures, and `wasm_instance_new` to take a vector type instead of a
pointer to an unsized array.

As a result, Wasmtime language bindings based on the C API will need to be
updated once this change is pulled in.

Fixes bytecodealliance#2211.
Fixes bytecodealliance#2131.
alexcrichton pushed a commit that referenced this issue Jan 14, 2021
* Update WebAssembly C API submodule to latest commit.

This commit updates the WebAssembly C API submodule (for `wasm.h`) to the
latest commit out of master.

This fixes the behavior of `wasm_name_new_from_string` such that it no longer
copies the null character into the name, which caused unexpected failures when
using the Wasmtime linker as imports wouldn't resolve when the null was
present.

Along with this change were breaking changes to `wasm_func_call`, the host
callback signatures, and `wasm_instance_new` to take a vector type instead of a
pointer to an unsized array.

As a result, Wasmtime language bindings based on the C API will need to be
updated once this change is pulled in.

Fixes #2211.
Fixes #2131.

* Update Doxygen comments for wasm.h changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants