From eea75e7862786ca30ffcca32648776ece45f16e0 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 25 Jun 2021 11:42:04 +0200 Subject: [PATCH 1/4] 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 a18a2a80a9f..53f628716ea 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(); + } +} From 9354f9bd51a7c4599fbe64eca03347bc9978442b Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 25 Jun 2021 11:52:31 +0200 Subject: [PATCH 2/4] feat(engine) Simplify code. --- lib/engine/src/trap/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/engine/src/trap/error.rs b/lib/engine/src/trap/error.rs index 08535c1488c..9615dfe2590 100644 --- a/lib/engine/src/trap/error.rs +++ b/lib/engine/src/trap/error.rs @@ -158,7 +158,7 @@ impl RuntimeError { /// Returns a reference the `message` stored in `Trap`. pub fn message(&self) -> String { - format!("{}", self.inner.source) + self.inner.source.to_string() } /// Returns a list of function frames in WebAssembly code that led to this From a4349f8719a4799e2879d533e1b3d84711e1d6c6 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 25 Jun 2021 13:26:02 +0200 Subject: [PATCH 3/4] doc(changelog) Add #2444. --- lib/c-api/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/c-api/CHANGELOG.md b/lib/c-api/CHANGELOG.md index dc920dcdda9..0d622fe4ec7 100644 --- a/lib/c-api/CHANGELOG.md +++ b/lib/c-api/CHANGELOG.md @@ -8,6 +8,11 @@ Looking for changes to the Wasmer CLI and the Rust API? See our [Primary Changel ## **[Unreleased]** +### Fixed +- [#2444](https://github.com/wasmerio/wasmer/pull/2444) Trap's messages are always null terminated. + +## 2.0.0 - 2020/06/16 + ## 2.0.0-rc1 - 2020/06/02 ### Added From d63ffcd78f7a2de6203ba10e40d614662583cbbe Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 25 Jun 2021 13:38:26 +0200 Subject: [PATCH 4/4] doc(c-api) Write documentation for the `trap` module. --- lib/c-api/src/wasm_c_api/instance.rs | 2 +- lib/c-api/src/wasm_c_api/mod.rs | 44 ++++++++++++++++++++++ lib/c-api/src/wasm_c_api/trap.rs | 55 ++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/lib/c-api/src/wasm_c_api/instance.rs b/lib/c-api/src/wasm_c_api/instance.rs index 7c742eb5ed5..9e845c5b8f2 100644 --- a/lib/c-api/src/wasm_c_api/instance.rs +++ b/lib/c-api/src/wasm_c_api/instance.rs @@ -87,7 +87,7 @@ pub unsafe extern "C" fn wasm_instance_new( /// /// # Example /// -/// See `wasm_instance_new`. +/// See [`wasm_instance_new`]. #[no_mangle] pub unsafe extern "C" fn wasm_instance_delete(_instance: Option>) {} diff --git a/lib/c-api/src/wasm_c_api/mod.rs b/lib/c-api/src/wasm_c_api/mod.rs index a2fc5ff3dd9..29870b9edb7 100644 --- a/lib/c-api/src/wasm_c_api/mod.rs +++ b/lib/c-api/src/wasm_c_api/mod.rs @@ -224,6 +224,50 @@ pub mod module; /// cbindgen:ignore pub mod store; +/// A trap represents an error which stores trace message with +/// backtrace. +/// +/// # Example +/// +/// ```rust +/// # use inline_c::assert_c; +/// # fn main() { +/// # (assert_c! { +/// # #include "tests/wasmer.h" +/// # +/// int main() { +/// // Create an engine and a store. +/// wasm_engine_t* engine = wasm_engine_new(); +/// wasm_store_t* store = wasm_store_new(engine); +/// +/// // Create the trap message. +/// wasm_message_t message; +/// wasm_name_new_from_string_nt(&message, "foobar"); +/// +/// // Create the trap with its message. +/// // The backtrace will be generated automatically. +/// wasm_trap_t* trap = wasm_trap_new(store, &message); +/// assert(trap); +/// +/// wasm_name_delete(&message); +/// +/// // Do something with the trap. +/// +/// // Free everything. +/// wasm_trap_delete(trap); +/// wasm_store_delete(store); +/// wasm_engine_delete(engine); +/// +/// return 0; +/// } +/// # }) +/// # .success(); +/// # } +/// ``` +/// +/// Usually, a trap is returned from a host function (an imported +/// function). +/// /// cbindgen:ignore pub mod trap; diff --git a/lib/c-api/src/wasm_c_api/trap.rs b/lib/c-api/src/wasm_c_api/trap.rs index 53f628716ea..96f1835bd21 100644 --- a/lib/c-api/src/wasm_c_api/trap.rs +++ b/lib/c-api/src/wasm_c_api/trap.rs @@ -19,6 +19,10 @@ impl From for wasm_trap_t { /// /// Be careful, the message is typed with `wasm_message_t` which /// represents a null-terminated string. +/// +/// # Example +/// +/// See the module's documentation for a complete example. #[no_mangle] pub unsafe extern "C" fn wasm_trap_new( _store: &mut wasm_store_t, @@ -53,9 +57,56 @@ pub unsafe extern "C" fn wasm_trap_new( Some(Box::new(trap)) } +/// Deletes a trap. +/// +/// # Example +/// +/// See the module's documentation for a complete example. #[no_mangle] pub unsafe extern "C" fn wasm_trap_delete(_trap: Option>) {} +/// Gets the message attached to the trap. +/// +/// # Example +/// +/// ```rust +/// # use inline_c::assert_c; +/// # fn main() { +/// # (assert_c! { +/// # #include "tests/wasmer.h" +/// # +/// int main() { +/// // Create an engine and a store. +/// wasm_engine_t* engine = wasm_engine_new(); +/// wasm_store_t* store = wasm_store_new(engine); +/// +/// // Create the trap message. +/// wasm_message_t message; +/// wasm_name_new_from_string_nt(&message, "foobar"); +/// +/// // Create the trap with its message. +/// // The backtrace will be generated automatically. +/// wasm_trap_t* trap = wasm_trap_new(store, &message); +/// assert(trap); +/// +/// // Get the trap's message back. +/// wasm_message_t retrieved_message; +/// wasm_trap_message(trap, &retrieved_message); +/// assert(retrieved_message.size == message.size); +/// +/// // Free everything. +/// wasm_name_delete(&message); +/// wasm_name_delete(&retrieved_message); +/// wasm_trap_delete(trap); +/// wasm_store_delete(store); +/// wasm_engine_delete(engine); +/// +/// return 0; +/// } +/// # }) +/// # .success(); +/// # } +/// ``` #[no_mangle] pub unsafe extern "C" fn wasm_trap_message( trap: &wasm_trap_t, @@ -72,11 +123,13 @@ pub unsafe extern "C" fn wasm_trap_message( out.data = byte_vec.data; } +/// Gets the origin frame attached to the trap. #[no_mangle] pub unsafe extern "C" fn wasm_trap_origin(trap: &wasm_trap_t) -> Option> { trap.inner.trace().first().map(Into::into).map(Box::new) } +/// Gets the trace (as a list of frames) attached to the trap. #[no_mangle] pub unsafe extern "C" fn wasm_trap_trace( trap: &wasm_trap_t, @@ -115,6 +168,7 @@ mod tests { assert(retrieved_message.size == 7); wasm_name_delete(&original_message); + wasm_name_delete(&retrieved_message); wasm_trap_delete(trap); wasm_store_delete(store); wasm_engine_delete(engine); @@ -146,6 +200,7 @@ mod tests { assert(retrieved_message.size == 7); wasm_name_delete(&original_message); + wasm_name_delete(&retrieved_message); wasm_trap_delete(trap); wasm_store_delete(store); wasm_engine_delete(engine);