WebAssembly / wasm-c-api

Wasm C API prototype
Apache License 2.0
534 stars 77 forks source link

wasm_name_t clarification #149

Closed AlexEne closed 4 years ago

AlexEne commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.)

binji commented 4 years ago

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