From eea75e7862786ca30ffcca32648776ece45f16e0 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 25 Jun 2021 11:42:04 +0200 Subject: [PATCH] fix(c-api) Trap's messages are always null terminated. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 https://github.com/wasmerio/wasmer/pull/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`. --- lib/c-api/src/wasm_c_api/trap.rs | 101 +++++++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 4 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/trap.rs b/lib/c-api/src/wasm_c_api/trap.rs index a18a2a80a..53f628716 100644 --- a/lib/c-api/src/wasm_c_api/trap.rs +++ b/lib/c-api/src/wasm_c_api/trap.rs @@ -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` @@ -15,14 +15,39 @@ impl From 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> { 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)) @@ -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; @@ -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(); + } +}