Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasi-common: cannot use file handles for WasiCtxBuilder::std[in|out|err] #1735

Closed
peterhuene opened this issue May 21, 2020 · 5 comments
Closed
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime

Comments

@peterhuene
Copy link
Member

peterhuene commented May 21, 2020

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

@peterhuene peterhuene added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label May 21, 2020
peterhuene added a commit to peterhuene/wasmtime that referenced this issue May 21, 2020
A regression in bytecodealliance#1561 caused "invalid argument" errors when `stdin`, `stdout`,
and `stderr` are given a file handle.

This was missed in Wasmtime CI because previously only a pipe handle was being
used.

It was caught in the .NET implementation CI because it uses file handles for
its WASI tests.

Fixes bytecodealliance#1735.
@kubkon
Copy link
Member

kubkon commented May 21, 2020

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!

@kubkon
Copy link
Member

kubkon commented May 21, 2020

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

@peterhuene
Copy link
Member Author

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 peterhuene@21b0d6e. It does incur an extra stat to see what type of handle it is, though.

@peterhuene
Copy link
Member Author

peterhuene commented May 21, 2020

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.

ericflo pushed a commit to ericflo/wasmtime that referenced this issue May 28, 2020
A regression in bytecodealliance#1561 caused "invalid argument" errors when `stdin`, `stdout`,
and `stderr` are given a file handle.

This was missed in Wasmtime CI because previously only a pipe handle was being
used.

It was caught in the .NET implementation CI because it uses file handles for
its WASI tests.

Fixes bytecodealliance#1735.
@kubkon
Copy link
Member

kubkon commented Jun 9, 2020

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

@kubkon kubkon closed this as completed Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime
Projects
None yet
Development

No branches or pull requests

2 participants