Stream: git-wasmtime

Topic: wasmtime / Issue #1735 wasi-common: cannot use file handl...


view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 01:24):

peterhuene opened Issue #1735:

It appears that #1561 regressed WasiCtxBuilder::stdin, WasiCtxBuilder::stdout, and WasiCtxBuilder::stderr because it is using OsOther which will error with invalid argument.

cc @kubkon.

This caused a CI failure in the .NET implementation that I hadn't had time to look into the last few days.

Unfortunately, there's a test coverage hole in Wasmtime CI where only a pipe handle was used for these functions, which is why the regression wasn't caught (OsOther works fine for pipes).

The fix should probably try an OsFile first and then a OsOther. We should also close the test hole.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 01:24):

peterhuene labeled Issue #1735:

It appears that #1561 regressed WasiCtxBuilder::stdin, WasiCtxBuilder::stdout, and WasiCtxBuilder::stderr because it is using OsOther which will error with invalid argument.

cc @kubkon.

This caused a CI failure in the .NET implementation that I hadn't had time to look into the last few days.

Unfortunately, there's a test coverage hole in Wasmtime CI where only a pipe handle was used for these functions, which is why the regression wasn't caught (OsOther works fine for pipes).

The fix should probably try an OsFile first and then a OsOther. We should also close the test hole.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 01:25):

peterhuene edited Issue #1735:

It appears that #1561 regressed WasiCtxBuilder::stdin, WasiCtxBuilder::stdout, and WasiCtxBuilder::stderr because it is using OsOther which will error with invalid argument when given a file handle.

cc @kubkon.

This caused a CI failure in the .NET implementation that I hadn't had time to look into the last few days.

Unfortunately, there's a test coverage hole in Wasmtime CI where only a pipe handle was used for these functions, which is why the regression wasn't caught (OsOther works fine for pipes).

The fix should probably try an OsFile first and then a OsOther. We should also close the test hole.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 01:40):

peterhuene edited Issue #1735:

It appears that #1561 regressed WasiCtxBuilder::stdin, WasiCtxBuilder::stdout, and WasiCtxBuilder::stderr because it is using OsOther which will error with invalid argument when given a file handle.

cc @kubkon.

This caused a CI failure in the .NET implementation that I hadn't had time to look into the last few days.

Unfortunately, there's a test coverage hole in Wasmtime CI where only a pipe handle was used for these functions, which is why the regression wasn't caught (OsOther works fine for pipes).

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 05:49):

kubkon commented on Issue #1735:

Oh shoot, it seems that my refactoring regressed a lot more than expected, sorry about that! I’ll have a look ASAP and also will try figure out an additional test. Thanks for the report @peterhuene!

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 18:35):

kubkon commented on Issue #1735:

Just out of curiosity, would #1600 fix this issue for you?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 19:59):

peterhuene commented on Issue #1735:

From a quick glance, I don't think it would, as the .NET API goes through the WASI C API, which remains using OsOther.

I was thinking of a more specific fix to where we currently handle PendingEntry::OsHandle to support either a file handle or an other handle, ala https://github.com/peterhuene/wasmtime/commit/21b0d6e27ccf5d19eee43f921f7898e960f005a0. It does incur an extra stat to see what type of handle it is, though.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 20:03):

peterhuene commented on Issue #1735:

Breaking change to the Rust API aside, I think if we changed those to OsFile, your approach would work. Th C API is only accepting paths to files.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2020 at 20:03):

peterhuene edited a comment on Issue #1735:

Breaking change to the Rust API aside, I think if we changed those to OsFile, your approach would work. The C API is only accepting paths to files.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2020 at 18:20):

kubkon commented on Issue #1735:

Since #1600 landed, I’m closing this one. Feel free to reopen it though if you feel it’s not been fully fixed @peterhuene!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2020 at 18:20):

kubkon closed Issue #1735:

It appears that #1561 regressed WasiCtxBuilder::stdin, WasiCtxBuilder::stdout, and WasiCtxBuilder::stderr because it is using OsOther which will error with invalid argument when given a file handle.

cc @kubkon.

This caused a CI failure in the .NET implementation that I hadn't had time to look into the last few days.

Unfortunately, there's a test coverage hole in Wasmtime CI where only a pipe handle was used for these functions, which is why the regression wasn't caught (OsOther works fine for pipes).


Last updated: Dec 23 2024 at 12:05 UTC