Skip to content

Commit

Permalink
fix(c-api) Trap's messages are always null terminated.
Browse files Browse the repository at this point in the history
`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`.
  • Loading branch information
Hywan committed Jun 25, 2021
1 parent 6773109 commit eea75e7
Showing 1 changed file with 97 additions and 4 deletions.
101 changes: 97 additions & 4 deletions lib/c-api/src/wasm_c_api/trap.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::store::wasm_store_t;
use super::types::{wasm_byte_vec_t, wasm_frame_t, wasm_frame_vec_t, wasm_message_t};
use std::str;
use std::ffi::CString;
use wasmer::RuntimeError;

// opaque type which is a `RuntimeError`
Expand All @@ -15,14 +15,39 @@ impl From<RuntimeError> for wasm_trap_t {
}
}

/// Create a new trap message.
///
/// Be careful, the message is typed with `wasm_message_t` which
/// represents a null-terminated string.
#[no_mangle]
pub unsafe extern "C" fn wasm_trap_new(
_store: &mut wasm_store_t,
message: &wasm_message_t,
) -> Option<Box<wasm_trap_t>> {
let message_bytes = message.into_slice()?;
let message_str = c_try!(str::from_utf8(message_bytes));
let runtime_error = RuntimeError::new(message_str);

// The trap message is typed with `wasm_message_t` which is a
// typeref to `wasm_name_t` with the exception that it's a
// null-terminated string. `RuntimeError` must contain a valid
// Rust `String` that doesn't contain a null byte. We must ensure
// this behavior.
let runtime_error = match CString::new(message_bytes) {
// The string is well-formed and doesn't contain a nul byte.
Ok(cstring) => RuntimeError::new(cstring.into_string().ok()?),

// The string is well-formed but is nul-terminated. Let's
// create a `String` which is null-terminated too.
Err(nul_error) if nul_error.nul_position() + 1 == message_bytes.len() => {
let mut vec = nul_error.into_vec();
vec.pop();

RuntimeError::new(String::from_utf8(vec).ok()?)
}

// The string not well-formed.
Err(_) => return None,
};

let trap = runtime_error.into();

Some(Box::new(trap))
Expand All @@ -39,7 +64,8 @@ pub unsafe extern "C" fn wasm_trap_message(
) {
let message = trap.inner.message();
let mut byte_vec = message.into_bytes();
byte_vec.push(0); // append NUL
byte_vec.push(0);

let byte_vec: wasm_byte_vec_t = byte_vec.into();

out.size = byte_vec.size;
Expand All @@ -63,3 +89,70 @@ pub unsafe extern "C" fn wasm_trap_trace(
out.size = frame_vec.size;
out.data = frame_vec.data;
}

#[cfg(test)]
mod tests {
use inline_c::assert_c;

#[test]
fn test_trap_message_null_terminated() {
(assert_c! {
#include "tests/wasmer.h"

int main() {
wasm_engine_t* engine = wasm_engine_new();
wasm_store_t* store = wasm_store_new(engine);

wasm_message_t original_message;
wasm_name_new_from_string_nt(&original_message, "foobar");
assert(original_message.size == 7); // 6 for `foobar` + 1 for nul byte.

wasm_trap_t* trap = wasm_trap_new(store, &original_message);
assert(trap);

wasm_message_t retrieved_message;
wasm_trap_message(trap, &retrieved_message);
assert(retrieved_message.size == 7);

wasm_name_delete(&original_message);
wasm_trap_delete(trap);
wasm_store_delete(store);
wasm_engine_delete(engine);

return 0;
}
})
.success();
}

#[test]
fn test_trap_message_not_null_terminated() {
(assert_c! {
#include "tests/wasmer.h"

int main() {
wasm_engine_t* engine = wasm_engine_new();
wasm_store_t* store = wasm_store_new(engine);

wasm_message_t original_message;
wasm_name_new_from_string(&original_message, "foobar");
assert(original_message.size == 6); // 6 for `foobar` + 0 for nul byte.

wasm_trap_t* trap = wasm_trap_new(store, &original_message);
assert(trap);

wasm_message_t retrieved_message;
wasm_trap_message(trap, &retrieved_message);
assert(retrieved_message.size == 7);

wasm_name_delete(&original_message);
wasm_trap_delete(trap);
wasm_store_delete(store);
wasm_engine_delete(engine);

return 0;
}
})
.success();
}
}

0 comments on commit eea75e7

Please sign in to comment.