Preview 2 filesystem: track open_mode instead of reporting the permissions #7876
Conversation
This file contains 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
pchickey
requested review from
alexcrichton and
sunfishcode
and removed request for
a team
sunfishcode
reviewed
Feb 6, 2024
| // descriptor is open for read-only or read-write, because the underlying | ||
| // permissions of the file determine whether or not the filestat can be | ||
| // set or not, not than the open mode. | ||
| unsafe { test_fd_filestat_set(dir_fd, wasi::RIGHTS_FD_READ) } |
There was a problem hiding this comment.
This doesn't seem right. If we open the file with just RIGHTS_FD_READ, we shuldn't be able to do fd_filestat_set_size on it. That would be analogous in POSIX to opening a file with O_READ and calling ftruncate, which is defined to fail.
There was a problem hiding this comment.
That was what I thought too but I didnt chase it down in the ftruncate case. From the python test suite I found that futimes is expected to succeed even when a file is read-only. So, I'll have to change this test, as well as set-len to check the open mode and reject size changes when not open for writing (fail with EINVAL, which I prefer here to EBADF) but permit set-times.
There was a problem hiding this comment.
This problem ended up being down to how the test was written. wasi::OFLAGS_CREAT implies that the WASI impl open the file with the O_RRONLY or O_RDWR in posix, so we were never actually opening a file just for reading in this test. I changed the test to create the file, then open it under the requested mode, and it behaves as expected.
|
I believe the test now shows that the implementations all behave as desired. |
alexcrichton
approved these changes
Feb 9, 2024
There was a problem hiding this comment.
Seems reasonable to me but I'm finding it difficult to follow what each of the sets of permissions are so I'm not 100% sure. Could you expand a bit on how this fixes #7829? I'm having a tough time connecting permissions and how that'd fail something fd-based vs path-based.
Also #7829 mentions errno 63 and ENOSR but I suspect that was something Python-specific or something like that?
whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <[email protected]>
we create and open the file with separate descriptors because the creation descriptor will always imply writing, whereas the rest do not. there were a number of auxillary functions in here that were obscuring what was going on, and making errors harder to read, so i changed some assertions to use macro-rules so the panic message had the relevant line number, and i inlined the creation / opening functions.
… it n DescriptorFlags FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY. We no longer mask FilePerms with the DescriptorFlags (oflags) in open_at - instead, track what open mode is requested, and see if the file and directory permissions permit it. Additionally, File and Dir now track their open_mode (represented using OpenMode, which just like FilePerms is just a read and write bit), and report that in DescriptorFlags. Previously, we reported the FilePerms or DirPerms in the DescriptorFlags, which was totally wrong. Co-authored-by: Trevor Elliott <[email protected]>
pchickey
force-pushed
the
pch/preview2_fs_dont_mask_perms
branch
from
7587c94 to
4eb836a
Compare
|
I just finished running CPython's test suite and this fixed the issue I found and didn't cause any new ones! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
pchickey
force-pushed
the
pch/preview2_fs_dont_mask_perms
branch
from
b7323b4 to
87a0b62
Compare
pchickey
pushed a commit
that referenced
this pull request
Feb 13, 2024
…sions (#7876) * fd_filestat_set test: assert that a file descriptor behaves the same... whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <[email protected]> * preview1_path_link test: refactor; create and open file separately we create and open the file with separate descriptors because the creation descriptor will always imply writing, whereas the rest do not. there were a number of auxillary functions in here that were obscuring what was going on, and making errors harder to read, so i changed some assertions to use macro-rules so the panic message had the relevant line number, and i inlined the creation / opening functions. * preview2 filesystem: track open mode instead of reporting permissions it n DescriptorFlags FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY. We no longer mask FilePerms with the DescriptorFlags (oflags) in open_at - instead, track what open mode is requested, and see if the file and directory permissions permit it. Additionally, File and Dir now track their open_mode (represented using OpenMode, which just like FilePerms is just a read and write bit), and report that in DescriptorFlags. Previously, we reported the FilePerms or DirPerms in the DescriptorFlags, which was totally wrong. Co-authored-by: Trevor Elliott <[email protected]> * different error on windows i guess? prtest:full * apparently set-times of read-only is rejected on windows. just skip testing that * path open read write: another alternate error code for windows * wasi-common actually gives a badf, so accept either * this case too --------- Co-authored-by: Trevor Elliott <[email protected]>
pchickey
pushed a commit
that referenced
this pull request
Feb 13, 2024
* Preview 2 filesystem: track open_mode instead of reporting the permissions (#7876) * fd_filestat_set test: assert that a file descriptor behaves the same... whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <[email protected]> * preview1_path_link test: refactor; create and open file separately we create and open the file with separate descriptors because the creation descriptor will always imply writing, whereas the rest do not. there were a number of auxillary functions in here that were obscuring what was going on, and making errors harder to read, so i changed some assertions to use macro-rules so the panic message had the relevant line number, and i inlined the creation / opening functions. * preview2 filesystem: track open mode instead of reporting permissions it n DescriptorFlags FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY. We no longer mask FilePerms with the DescriptorFlags (oflags) in open_at - instead, track what open mode is requested, and see if the file and directory permissions permit it. Additionally, File and Dir now track their open_mode (represented using OpenMode, which just like FilePerms is just a read and write bit), and report that in DescriptorFlags. Previously, we reported the FilePerms or DirPerms in the DescriptorFlags, which was totally wrong. Co-authored-by: Trevor Elliott <[email protected]> * different error on windows i guess? prtest:full * apparently set-times of read-only is rejected on windows. just skip testing that * path open read write: another alternate error code for windows * wasi-common actually gives a badf, so accept either * this case too --------- Co-authored-by: Trevor Elliott <[email protected]> * Return correct fs errors on the proxy adapter (#7926) This commit fixes an issue with the proxy adapter where if the proxy program attempted to look at prestat items, which wasi-libc always does on startup, it would invoke `fd_prestat_get` and receive `ERRNO_NOTSUP` in response (the standard response when using the `cfg_filesystem_available!` macro). This error code is unexpected by wasi-libc and causes wasi-libc to abort. The PR here is to instead return `ERRNO_BADF` which is indeed expected by wasi-libc and is recognized as meaning "that prestat isn't available". * Make wasi-common self-contained, deprecate exports from wasmtime-wasi (#7881) * WIP: try to make wasi-common self contained. * rebase: cargo.lock * remove all dependencies between wasi-common and wasmtime-wasi * use wasi-common directly throughout tests, benches, examples, cli run * wasi-threads: use wasi-common's maybe_exit_on_error in spawned thread not a very modular design, but at this point wasi-common and wasi-threads are forever wed * fix wasmtime's docs * re-introduce wasmtime-wasi's exports of wasi-common definitions behind deprecated * factor out determining i32 process exit code and remove libc dep because rustix provides the same constant * commands/run: inline the logic about aborting on trap since this is the sole place in the codebase its used * Add high-level summary to wasi-common's top-level doc comment. * c-api: fix use of wasi_cap_std_sync => wasi_common::sync, wasmtime_wasi => wasi_common * fix tokio example * think better of combining downcast and masking into one method * fix references to wasmtime_wasi in docs prtest:full * benches: use wasi-common * cfg-if around use of rustix::process because that doesnt exist on windows * wasi-common: include tests, caught by verify-publish * fix another bench * exit requires wasmtime dep. caught by verify-publish. --------- Co-authored-by: Trevor Elliott <[email protected]> Co-authored-by: Alex Crichton <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
First update the fd_filestat_set test to assert that the filestat_set operations on a file descriptor behaves the same whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829. wasi-common passes this changed test, but wasmtime-wasi::preview2 does not, showing that this is a regression between the two implementations.
Then, in preview2 implementation, track open_mode for a File and Dir, and use that instead of reporting permissions in DescriptorFlags.
FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY.
and report them in DescriptorFlags