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_t clarification #149

Closed
AlexEne opened this issue Jul 27, 2020 · 3 comments
Closed

wasm_name_t clarification #149

AlexEne opened this issue Jul 27, 2020 · 3 comments

Comments

@AlexEne
Copy link

AlexEne commented Jul 27, 2020

Hello,

I stumbled into this problem when using the C-API with WASMTIME.
wasmname_t can be created with wasm_name_new_from_string. This function is implemented like so:

static inline void wasm_name_new_from_string(
  own wasm_name_t* out, const char* s
) {
  wasm_name_new(out, strlen(s) + 1, s);
}

It copies the terminating '\0', as you can observe from the +1.
It is unclear to me if this is intentional or not (it seems like a good idea to copy the null if what's on the other side is a C++ engine).
However, if what's on the other side is a rust implementation that creates a &str from bytes, it creates the string array like this:
By passing "env" as a parameter you get ['e', 'n', 'v', '\0'].

Rust strings and string slices (&str) are not null terminated, they have a pointer and a size. And this is where that null terminator becomes a problem, as any comparison would fail due to the extra character.

So my question is: Is this by design? Could we change it to not pick up the null at the end?

@peterhuene
Copy link

Also, given that this only appears in an example for creating a wasm_message_t used in wasm_trap_new, should this function be named wasm_message_new_from_string instead?

From the comment in the header where wasm_message_t is typedef'd, wasm_message_t is null terminated so it makes sense that this function would copy the null terminator for that use case.

For wasm_name_t uses in Wasmtime, we're expecting the underlying byte vec length to not include a null terminator (as it would be unnecessary). Therefore a wasm_name_t created by wasm_name_new_from_string will result in the null terminator being treated as a character to compare against or display.

@rossberg
Copy link
Member

rossberg commented Aug 6, 2020

Indeed, the distinction was a bit fuzzy. So in #151, I changed the existing name_new_from_string to not include NUL and introduced a separate function name_new_from_string_nt that does. Same in the C++ API.

Edit: To clarify, regular names are not supposed to be null-terminated. (Maybe that's a bad idea for message strings, too, but that's perhaps a different question.)

@rossberg rossberg closed this as completed Aug 6, 2020
@binji
Copy link
Member

binji commented Aug 6, 2020

lgtm. Small bikeshed, I don't think nt is very clear, I'd prefer something like cstr or 'stringz` I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants