Skip to content

Commit

Permalink
Merge #2070
Browse files Browse the repository at this point in the history
2070: fix(c-api) Don't drain the entire captured stream at first read r=Hywan a=Hywan

# Description

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 `..` (as we do) 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.

This patch also updates the `test-wasi` integration test. Previously,
`stdout` wasn't captured, so the loop using `wasi_env_read_stdout` was
doing nothing. Now we have the following output:

```
Initializing...
Loading binary...
Compiling module...
Setting up WASI...
Instantiating module...
Extracting export...
found 2 exports
Calling export...
Evaluating "function greet(name) { return JSON.stringify('Hello, ' + name); }; print(greet('World'));"
WASI Stdout: `"Hello, World"
`
Shutting down...
Done.
```

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Ivan Enderlin <[email protected]>
  • Loading branch information
bors[bot] and Hywan authored Jan 28, 2021
2 parents 3d6e4ad + 2df343d commit aea76b8
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
- [#2058](https://github.com/wasmerio/wasmer/pull/2058) Expose WASI versions to C correctly.
- [#2044](https://github.com/wasmerio/wasmer/pull/2044) Do not build C headers on docs.rs.

Expand Down
13 changes: 9 additions & 4 deletions lib/c-api/src/wasm_c_api/wasi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -275,12 +276,16 @@ pub unsafe extern "C" fn wasi_env_read_stderr(

fn read_inner(wasi_file: &mut Box<dyn WasiFile>, inner_buffer: &mut [u8]) -> isize {
if let Some(oc) = wasi_file.downcast_mut::<capture_files::OutputCapturer>() {
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
}
Expand Down
38 changes: 29 additions & 9 deletions lib/c-api/tests/test-wasi.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit aea76b8

Please sign in to comment.