Readdir doesn't correctly handle capacity updates

[RReverser]

Try to compile this sample code to WASI:

fn main() -> std::io::Result<()> {
	for entry in std::fs::read_dir("/")? {
		println!("{}", entry?.path().display());
	}
	Ok(())
}

Then, choose a large-ish folder - e.g. I created a temporary directory with 50 files named 0...49.

Run the produced Wasm with Wasmtime with that directory mapped to /:

$ wasmtime temp.wasm --mapdir /::$PWD/temp-dir
/1
/2
/49
thread 'main' panicked at 'range end index 1214 out of range for slice of length 128', library/std/src/sys/wasi/fs.rs:164:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: failed to run main module `temp.wasm`

Caused by:
    0: failed to invoke command default
    1: wasm trap: unreachable
       wasm backtrace:
           0: 0xd581 - <unknown>!__rust_start_panic
           1: 0xd223 - <unknown>!rust_panic
           2: 0xcdfb - <unknown>!std::panicking::rust_panic_with_hook::h7412819944345424
           3: 0xc36e - <unknown>!std::panicking::begin_panic_handler::{{closure}}::h4738c0049ce98322
           4: 0xc2af - <unknown>!std::sys_common::backtrace::__rust_end_short_backtrace::h8bb6e3f06234953f
           5: 0xcc9f - <unknown>!rust_begin_unwind
           6: 0x12d4e - <unknown>!core::panicking::panic_fmt::h904ce09f3cb14707
           7: 0x12432 - <unknown>!core::slice::index::slice_end_index_len_fail::hcdd59b2bc02fd78c
           8: 0x9bd9 - <unknown>!<std::sys::wasi::fs::ReadDir as core::iter::traits::iterator::Iterator>::next::h53073ad4dd8c7879
           9: 0x96ff - <unknown>!<std::fs::ReadDir as core::iter::traits::iterator::Iterator>::next::h35e43cb49f6132cb
          10:  0x9de - <unknown>!temp::main::h0ae35bb072c9f0ef
          11: 0x1c90 - <unknown>!core::ops::function::FnOnce::call_once::hf1fda840e003cdf0
          12: 0x2820 - <unknown>!std::sys_common::backtrace::__rust_begin_short_backtrace::hf0b5aa48499a3256
          13: 0x3e5b - <unknown>!std::rt::lang_start::{{closure}}::h6eaa3cdacd789dba
          14: 0xd334 - <unknown>!std::rt::lang_start_internal::h0e1571f3e9f07dad
          15: 0x3dfc - <unknown>!std::rt::lang_start::h382cc3264c9b1456
          16: 0x1483 - <unknown>!__original_main
          17:  0x527 - <unknown>!_start
       note: run with `WASMTIME_BACKTRACE_DETAILS=1` environment variable to display more information

Same code against the same folder works fine with Wasmer, suggesting it's an environment issue not Rust stdlib issue:

/0
/1
/10
/11
/12
/13
/14
/15
/16
/17
/18
/19
/2
/20
/21
/22
/23
/24
/25
/26
/27
/28
/29
/3
/30
/31
/32
/33
/34
/35
/36
/37
/38
/39
/4
/40
/41
/42
/43
/44
/45
/46
/47
/48
/49
/5
/6
/7
/8
/9

The relevant code on the Rust side also hasn't been updated in 2 years, also confirming it's likely a runtime environment issue: https://github.com/rust-lang/rust/blob/643a79af3d5a31fa87c8a4c9d7f8bc4ebe2add4b/library/std/src/sys/wasi/fs.rs#L164

[github-actions]

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

• kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[peterhuene]

The bug is indeed in Wasmtime's WASI implementation.

Our fd_readdir implementation is returning more bytes used than capacity of the given buffer in this case. This is because it incorrectly fails to truncate the length of the entry name as it does not take into account the space taken by the dirent structure when calculating how many bytes of the name it can write to the buffer.

I have a fix.

[peterhuene]

@RReverser would you mind trying the fix in #2620 and verify it addresses this issue? Thanks!

[RReverser]

As long as the sample above works, I trust it's fine.

[RReverser]

I'd suggest to add it as a regression test to the PR though.

[peterhuene]

Agreed that there should be a regression test here, but it is a little difficult to mock up currently as it isn't directly related to the number of directory entries, just that the (unstable order of) entries being returned doesn't neatly fit in the buffer given to fd_readdir.

The WASI test programs use a real file system underneath, so we might be able to create a repro test relying on filesystem-specific behavior, perhaps.

The reason I wanted to confirm it addresses what you're seeing is that 1214 (from your panic above) is a much larger offset than what I'd expect returned when your file names are all really short, but that might be explained by the stdlib impl not expecting a "length used" returned from fd_readdir to be greater than the size of the buffer that was given.

[peterhuene]

Actually, there's some virtualized filesystem support in the WASI test-programs I'm completely unfamiliar with. I'll see if we can use this to properly test this for regression.

[peterhuene]

Hmm, it appears that fd_readdir WASI test program might have a workaround for the bug:

let sl = slice::from_raw_parts(buf.as_ptr(), min(BUF_LEN, bufused));

It should be asserting that bufused <= BUF_LEN.

[pchickey]

The virtualized filesystem support currently only works for files, not directories.

I'm nearly ready to land @sunfishcode's and my lengthy rewrite of wasi-common, and will need to backport the fix for this into my branch. The virtualized filesystem support in the rewrite is much better and we should be able to synthesize this test, or even fuzz this interface, since we keep on getting it wrong!

[RReverser]

I might also suggest checking out WebAssembly/WASI#9 (comment) thread where @caspervonb has been working on a shared WASI testsuite. It seems like Wasmtime might benefit from running the same tests, and, OTOH, the testsuite could include examples like this one with large-ish directories (should be just a matter of adding more files to the fixtures folder).

[pchickey]

Agreed, we'll be contributing the wasmtime tests to that suite and getting the rest of it run as part of wasmtime's CI soon :)