fix Wasi rights system to work with wasi-libc#6463
Merged
Conversation
github-actions
bot
added
cranelift
pchickey
force-pushed
the
pch/wasi_read_write_rights
branch
from
827cf5e to
5b3c019
Compare
alexcrichton
approved these changes
May 26, 2023
Subscribe to Label Actioncc @kubkon DetailsThis issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasi"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
pchickey
force-pushed
the
pch/wasi_read_write_rights
branch
2 times, most recently
from
e7831fe to
0bd2695
Compare
added 11 commits
May 30, 2023 15:18
implementations such as wasi-libc use these as a mask for any rights they request for a new fd when invoking path_open
…am creation which will mean editing the wit.
…n error-code in order to fail appropriately when a file is not open for reading or writing.
I appreciate his work on this stuff in the past but he hasnt been around for a few years now
since file perms is now more accurately the file's access mode, a file open for writing was having problems here. in reality i dont think it makes sense to restrict this based on the perms. if a file is opened, you should be able to stat it. this fixes the host adapter's path_link test, which was broken by prior changes. additionally, this fix lets the path_open_dirfd_not_dir test pass.
in order to work with wasi-testsuite, it needs to be possible to path_open(dirfd, ".", ...) with the same rights reported in the fdstat of that dirfd. When we report the Rights::all() set, both FD_READ and FD_WRITE are set in the base rights, which results in unix rejecting an openat2(dirfd, ".", O_RDWR) with EISDIR. By not having the FD_READ and FD_WRITE rights present in the base rights, the open syscall defaults to O_RDONLY, which is the only access mode allowed for opening directories.
pchickey
force-pushed
the
pch/wasi_read_write_rights
branch
from
eb3a07f to
e063785
Compare
sunfishcode
approved these changes
May 30, 2023
dhil
pushed a commit
to dhil/wasmtime
that referenced
this pull request
Jun 5, 2023
* wasi-common: need FileEntry to track the mode with which a file was opened
* add a test for read, write access modes in path_open
* a directory needs to report full set of rights in fdstat
implementations such as wasi-libc use these as a mask for any rights
they request for a new fd when invoking path_open
* preview2: mask file perms. and we really need to enforce them on stream creation
which will mean editing the wit.
* filesystem.wit: make {read, write, append}-via-stream fallible with an error-code
in order to fail appropriately when a file is not open for reading or
writing.
* filesystem.wit: update comments
* preview2 impls: fix error handling for {read,write,append}_via_stream
* wasi-common: normalize wrong access mode error on windows to badf
* remove kubkob from labeler
I appreciate his work on this stuff in the past but he hasnt been around
for a few years now
* preview 2 filesystem: allow stat on any opened fd
since file perms is now more accurately the file's access mode, a file
open for writing was having problems here.
in reality i dont think it makes sense to restrict this based on the
perms. if a file is opened, you should be able to stat it.
this fixes the host adapter's path_link test, which was broken by prior
changes. additionally, this fix lets the path_open_dirfd_not_dir test
pass.
* fix the directory base & inheriting rights
in order to work with wasi-testsuite, it needs to be possible to
path_open(dirfd, ".", ...) with the same rights reported in the
fdstat of that dirfd. When we report the Rights::all() set, both
FD_READ and FD_WRITE are set in the base rights, which results in
unix rejecting an openat2(dirfd, ".", O_RDWR) with EISDIR.
By not having the FD_READ and FD_WRITE rights present in the base
rights, the open syscall defaults to O_RDONLY, which is the only
access mode allowed for opening directories.
* path_open of a directory with read and write succeeds on windows
|
The bug that this fixed has regressed in certain ways. Reproduction here: WebAssembly/wasi-libc#415 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an upstreaming of #6462
#6265 introduced a regression with programs using wasi-libc, reported at WebAssembly/wasi-libc#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:
Preview 2's wit definition was unintentionally (per @sunfishcode) missing a way for
filesystem.{read,write,append}-via-streamto return anerror-codewhen 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.