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

Include a NUL byte in the message returned by wasm_trap_message(). #1947

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

nlewycky
Copy link
Contributor

No description provided.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I assumed that strings wouldn't be nul-terminated because we pass back a pointer and a length; what was the motivating reason for this change?

@nlewycky
Copy link
Contributor Author

wasm.h has this to say:

typedef wasm_name_t wasm_message_t;  // null terminated

so I'd guess that wasm_message_t should be NUL terminated but wasm_name_t may not be.

In practice it came up in wasm-c-api/examples/start.cc which prints out a string by pointer:

  std::cout << "> " << trap->message().get() << std::endl;

Since it's using a pointer with no length, operator<< calls strlen() to find the end of the string which in turn reads off the end.

@MarkMcCaskey
Copy link
Contributor

That's good to know! I'd expect this to be a more widespread issue, affecting everywhere message is used

@nlewycky
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

@bors bors bot merged commit bc0ba32 into master Dec 16, 2020
@bors bors bot deleted the feature/trap-message-nul branch December 16, 2020 21:48
@Hywan
Copy link
Contributor

Hywan commented Dec 17, 2020

👍

@Hywan Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Dec 17, 2020
@Hywan
Copy link
Contributor

Hywan commented Jun 25, 2021

This PR was actually incorrect. wasm_trap_t works on wasm_message_t which is supposed to always contain a null-terminated string. But wasm_trap_t uses RuntimeError which is a valid Rust string that must not contain a null byte. In the current code at the time of writing, RuntimeError receives a null terminated string (which is arguably incorrect), a wasm_trap_message always add a new null byte at the end of the string. It results in a doubly-null-terminated string. This is incorrect :-).

Hywan added a commit to Hywan/wasmer that referenced this pull request Jun 25, 2021
`wasm_trap_new` expects a `wasm_message_t`. It's a type alias to
`wasm_name_t` with the exception that it represents a null-terminated
string.

When calling `wasm_trap_new`, no check was present to ensure the
string was well-formed. That's a first issue. But in the best
scenario, the string was correctly formed and was
null-terminated. This string was transformed to a Rust `String` —with
the null byte!— and passed to `RuntimeError`.

Then in `wasm_trap_message`, another null byte was pushed at the end
of the message. It's been introduced in
wasmerio#1947. It results in a
doubly-null-terminated string, which is incorrect.

This patch does the following:

1. It checks that the string given to `wasm_trap_new` contains a
   null-terminated string or not, and will act accordingly. Note that
   it's possible to pass a non-null-terminated string, and it will
   still work because this detail is vicious. The idea is to get a
   well-formed `RuntimeError` in anycase.

  * If no null byte is found, the string is passed to `RuntimeError`
    as a valid Rust string,

  * If a null byte is found at the end of the string, a new string is
    passed to `RuntimeError` but without the final null byte,

  * If a null byte is found but not at the end, it's considered as an
    error,

  * If the string contains invalid UTF-8 bytes, it's considered as an
    error.

2. It updates `wasm_trap_message` to always add a null byte at the end
   of the returned owned string.

3. It adds test cases when passing a null-terminated or a
   non-null-terminated string to `wasm_trap_new` and to compare the
   results to `wasm_trap_message`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants