pchickey opened PR #6463 from bytecodealliance:pch/wasi_read_write_rights
to bytecodealliance:main
:
Based on #6390
This is an upstreaming of #6462
https://github.com/bytecodealliance/wasmtime/pull/6265 introduced a regression with programs using wasi-libc, reported at https://github.com/WebAssembly/wasi-libc/issues/415.
Wasi-libc read the rights of the base directory (using fd_fdstat_get) and used those to mask the rights requested to path_open. In 6265, I changed the behavior of fdstat_get to always report and empty set of rights. This means that Wasi-libc will always pass an empty set of rights to path_open, which is a problem because the FD_READ and FD_WRITE rights are how path_open determines if a descriptor is to be opened for reading, writing, or both.
The fix is as follows:
directories always return the full set of rights in fd_fdstat_get.
we record the access mode that a file is opened with, and use that to set the FD_READ and FD_WRITE bits in fs_rights_base for a file's fd_fdstat_get.A test demonstrates the behavior of the fdstat rights bits, and that opening for reading, writing, or reading and writing behaves correctly when calling fd_read and fd_write
- This PR is just for the release-9.0.0 branch, I will work on upstreaming it to main but the situation there is slightly more complex because the test also needs to pass under the preview 2 implementation.
Preview 2's wit definition was unintentionally (per @sunfishcode) missing a way for
filesystem.{read,write,append}-via-stream
to return anerror-code
when a descriptor cannot be used to create such a stream. I added this to the wit definition in order to make the preview 2 behavior match the legacy implementation.
pchickey edited PR #6463:
Based on #6390
This is an upstreaming of #6462
https://github.com/bytecodealliance/wasmtime/pull/6265 introduced a regression with programs using wasi-libc, reported at https://github.com/WebAssembly/wasi-libc/issues/415.
Wasi-libc read the rights of the base directory (using fd_fdstat_get) and used those to mask the rights requested to path_open. In 6265, I changed the behavior of fdstat_get to always report and empty set of rights. This means that Wasi-libc will always pass an empty set of rights to path_open, which is a problem because the FD_READ and FD_WRITE rights are how path_open determines if a descriptor is to be opened for reading, writing, or both.
The fix is as follows:
- directories always return the full set of rights in fd_fdstat_get.
- legacy implementation records the access mode that a file is opened with, and use that to set the FD_READ and FD_WRITE bits in fs_rights_base for a file's fd_fdstat_get. The preview 2 implementation already did this.
- A test demonstrates the behavior of the fdstat rights bits, and that opening for reading, writing, or reading and writing behaves correctly when calling fd_read and fd_write
Preview 2's wit definition was unintentionally (per @sunfishcode) missing a way for
filesystem.{read,write,append}-via-stream
to return anerror-code
when a descriptor cannot be used to create such a stream. I added this to the wit definition in order to make the preview 2 behavior match the legacy implementation.
pchickey edited PR #6463:
Based on #6390 because I needed to edit wits - will land after that merges.
This is an upstreaming of #6462
https://github.com/bytecodealliance/wasmtime/pull/6265 introduced a regression with programs using wasi-libc, reported at https://github.com/WebAssembly/wasi-libc/issues/415.
Wasi-libc read the rights of the base directory (using fd_fdstat_get) and used those to mask the rights requested to path_open. In 6265, I changed the behavior of fdstat_get to always report and empty set of rights. This means that Wasi-libc will always pass an empty set of rights to path_open, which is a problem because the FD_READ and FD_WRITE rights are how path_open determines if a descriptor is to be opened for reading, writing, or both.
The fix is as follows:
- directories always return the full set of rights in fd_fdstat_get.
- legacy implementation records the access mode that a file is opened with, and use that to set the FD_READ and FD_WRITE bits in fs_rights_base for a file's fd_fdstat_get. The preview 2 implementation already did this.
- A test demonstrates the behavior of the fdstat rights bits, and that opening for reading, writing, or reading and writing behaves correctly when calling fd_read and fd_write
Preview 2's wit definition was unintentionally (per @sunfishcode) missing a way for
filesystem.{read,write,append}-via-stream
to return anerror-code
when a descriptor cannot be used to create such a stream. I added this to the wit definition in order to make the preview 2 behavior match the legacy implementation.
pchickey updated PR #6463.
pchickey edited PR #6463:
This is an upstreaming of #6462
https://github.com/bytecodealliance/wasmtime/pull/6265 introduced a regression with programs using wasi-libc, reported at https://github.com/WebAssembly/wasi-libc/issues/415.
Wasi-libc read the rights of the base directory (using fd_fdstat_get) and used those to mask the rights requested to path_open. In 6265, I changed the behavior of fdstat_get to always report and empty set of rights. This means that Wasi-libc will always pass an empty set of rights to path_open, which is a problem because the FD_READ and FD_WRITE rights are how path_open determines if a descriptor is to be opened for reading, writing, or both.
The fix is as follows:
- directories always return the full set of rights in fd_fdstat_get.
- legacy implementation records the access mode that a file is opened with, and use that to set the FD_READ and FD_WRITE bits in fs_rights_base for a file's fd_fdstat_get. The preview 2 implementation already did this.
- A test demonstrates the behavior of the fdstat rights bits, and that opening for reading, writing, or reading and writing behaves correctly when calling fd_read and fd_write
Preview 2's wit definition was unintentionally (per @sunfishcode) missing a way for
filesystem.{read,write,append}-via-stream
to return anerror-code
when a descriptor cannot be used to create such a stream. I added this to the wit definition in order to make the preview 2 behavior match the legacy implementation.
pchickey has marked PR #6463 as ready for review.
pchickey requested itsrainy for a review on PR #6463.
pchickey requested wasmtime-core-reviewers for a review on PR #6463.
pchickey requested wasmtime-default-reviewers for a review on PR #6463.
pchickey updated PR #6463.
alexcrichton submitted PR review.
pchickey has enabled auto merge for PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey updated PR #6463.
pchickey requested sunfishcode for a review on PR #6463.
sunfishcode submitted PR review.
pchickey merged PR #6463.
squeek502 commented on PR #6463:
The bug that this fixed has regressed in certain ways. Reproduction here: https://github.com/WebAssembly/wasi-libc/issues/415#issuecomment-2244254211
Last updated: Dec 23 2024 at 13:07 UTC