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

fix(c-api) Trap's messages are always null terminated #2444

Merged
merged 4 commits into from
Jun 25, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jun 25, 2021

Description

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

It fixes #2439.

Review

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

`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`.
@Hywan Hywan marked this pull request as ready for review June 25, 2021 11:26
@Hywan Hywan self-assigned this Jun 25, 2021
@Hywan Hywan added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Jun 25, 2021
@Hywan
Copy link
Contributor Author

Hywan commented Jun 25, 2021

bors try

bors bot added a commit that referenced this pull request Jun 25, 2021
@bors
Copy link
Contributor

bors bot commented Jun 25, 2021

@syrusakbary
Copy link
Member

Merging directly as bors already passed

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.

Trap messages get an extra null terminator
2 participants