Stream: git-wasmtime

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

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.

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.

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.

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).

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!

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?

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.

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.

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.

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!

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: Nov 22 2024 at 17:03 UTC