From ef1328e1d7f1a2f07391d7330c4e003d4650d78a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 28 Jan 2021 10:28:49 +0100 Subject: [PATCH 1/3] fix(c-api) Don't drain the entire captured stream when reading a small range. We use `VecDeque::drain` to read the captured stream, zipped with the given buffer. We could expect that only the yielded items from the `drain` will be removed, but actually no. Reading [the documentation](https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.drain): > Note 1: The element `range` is removed even if the iterator is not > consumed until the end. So by using a range like `..` will drain the entire captured stream, whatever we read from it. Said differently, if the given buffer length is smaller than the captured stream, the first read will drain the entire captured stream. This patch fixes the problem by specifying a better range: `..min(inner_buffer.len(), oc.buffer.len())`. With this new range, it's actually useless to increment `num_bytes_written`, we already know ahead of time the amount of bytes we are going to read. Consequently, the patch simplifies this code a little bit more. --- lib/c-api/src/wasm_c_api/wasi/mod.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/c-api/src/wasm_c_api/wasi/mod.rs b/lib/c-api/src/wasm_c_api/wasi/mod.rs index 67e217fc97d..2c6fee8441c 100644 --- a/lib/c-api/src/wasm_c_api/wasi/mod.rs +++ b/lib/c-api/src/wasm_c_api/wasi/mod.rs @@ -13,6 +13,7 @@ use super::{ // required due to really weird Rust resolution rules for macros // https://github.com/rust-lang/rust/issues/57966 use crate::error::{update_last_error, CApiError}; +use std::cmp::min; use std::convert::TryFrom; use std::ffi::CStr; use std::os::raw::c_char; @@ -275,12 +276,16 @@ pub unsafe extern "C" fn wasi_env_read_stderr( fn read_inner(wasi_file: &mut Box, inner_buffer: &mut [u8]) -> isize { if let Some(oc) = wasi_file.downcast_mut::() { - let mut num_bytes_written = 0; - for (address, value) in inner_buffer.iter_mut().zip(oc.buffer.drain(..)) { + let total_to_read = min(inner_buffer.len(), oc.buffer.len()); + + for (address, value) in inner_buffer + .iter_mut() + .zip(oc.buffer.drain(..total_to_read)) + { *address = value; - num_bytes_written += 1; } - num_bytes_written + + total_to_read as isize } else { -1 } From 1bbec4519f2960dfa3886930dde5fa273c9dbda7 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 28 Jan 2021 10:44:53 +0100 Subject: [PATCH 2/3] doc(changelog) Add #2070. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06af7a1a893..6a72ffb0a73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - [#2056](https://github.com/wasmerio/wasmer/pull/2056) Change back to depend on the `enumset` crate instead of `wasmer_enumset` ### Fixed +- [#2070](https://github.com/wasmerio/wasmer/pull/2070) Do not drain the entire captured stream at first read with `wasi_env_read_stdout` or `_stderr` in the C API. - [#2044](https://github.com/wasmerio/wasmer/pull/2044) Do not build C headers on docs.rs. ## 1.0.1 - 2021-01-12 From 2df343dc7079989765d9dbb86e0354543f61df47 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 28 Jan 2021 11:25:42 +0100 Subject: [PATCH 3/3] test(c-api) Test WASI captured stdout API. --- lib/c-api/tests/test-wasi.c | 38 ++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/c-api/tests/test-wasi.c b/lib/c-api/tests/test-wasi.c index 60f89185c2f..3403e8996b7 100644 --- a/lib/c-api/tests/test-wasi.c +++ b/lib/c-api/tests/test-wasi.c @@ -58,6 +58,7 @@ int main(int argc, const char* argv[]) { const char* js_string = "function greet(name) { return JSON.stringify('Hello, ' + name); }; print(greet('World'));"; wasi_config_arg(config, "--eval"); wasi_config_arg(config, js_string); + wasi_config_capture_stdout(config); wasi_env_t* wasi_env = wasi_env_new(config); if (!wasi_env) { @@ -125,16 +126,35 @@ int main(int argc, const char* argv[]) { return 1; } - char buffer[BUF_SIZE] = { 0 }; - size_t result = BUF_SIZE; - for (size_t i = 0; - // TODO: this code is too clever, make the control flow more obvious here - result == BUF_SIZE && - (result = wasi_env_read_stdout(wasi_env, buffer, BUF_SIZE)); - ++i) { - printf("%.*s", BUF_SIZE, buffer); + { + FILE *memory_stream; + char* stdout; + size_t stdout_size = 0; + + memory_stream = open_memstream(&stdout, &stdout_size); + + if (NULL == memory_stream) { + printf("> Error creating a memory stream.\n"); + return 1; + } + + char buffer[BUF_SIZE] = { 0 }; + size_t data_read_size = BUF_SIZE; + + do { + data_read_size = wasi_env_read_stdout(wasi_env, buffer, BUF_SIZE); + + if (data_read_size > 0) { + stdout_size += data_read_size; + fwrite(buffer, sizeof(char), data_read_size, memory_stream); + } + } while (BUF_SIZE == data_read_size); + + fclose(memory_stream); + + printf("WASI Stdout: `%.*s`\n", (int) stdout_size, stdout); } - printf("\n"); + wasm_extern_vec_delete(&exports); wasm_extern_vec_delete(&imports);