Preview 2 filesystem: track open_mode instead of reporting the permissions by pchickey · Pull Request #7876 · bytecodealliance/wasmtime

pchickey · 2024-02-06T01:11:43Z

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.

don't mask FilePerms with the DescriptorFlags (oflags) in path_open
File and Dir now track their open_mode (represented using FilePerms)
and report them in DescriptorFlags

sunfishcode · 2024-02-06T23:58:37Z

crates/test-programs/src/bin/preview1_fd_filestat_set.rs

+    // 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) }


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.

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.

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.

pchickey · 2024-02-09T00:24:49Z

I believe the test now shows that the implementations all behave as desired.

alexcrichton

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?

crates/wasi/src/preview2/ctx.rs

crates/wasi/src/preview2/filesystem.rs

whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <telliott@fastly.com>

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 <telliott@fastly.com>

brettcannon · 2024-02-10T01:19:48Z

I just finished running CPython's test suite and this fixed the issue I found and didn't cause any new ones!

prtest:full

…esting that

…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 <telliott@fastly.com> * 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 <telliott@fastly.com> * 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 <telliott@fastly.com>

* 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 <telliott@fastly.com> * 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 <telliott@fastly.com> * 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 <telliott@fastly.com> * 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 <telliott@fastly.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com>

pchickey requested a review from a team as a code owner February 6, 2024 01:11

pchickey requested review from alexcrichton and sunfishcode and removed request for a team February 6, 2024 01:11

pchickey mentioned this pull request Feb 6, 2024

utime() does not accept a file descriptor under preview 2 (as found by Python) #7829

Closed

github-actions bot added the wasi Issues pertaining to WASI label Feb 6, 2024

sunfishcode reviewed Feb 6, 2024

View reviewed changes

pchickey requested a review from sunfishcode February 9, 2024 00:25

alexcrichton approved these changes Feb 9, 2024

View reviewed changes

crates/wasi/src/preview2/ctx.rs Outdated Show resolved Hide resolved

crates/wasi/src/preview2/filesystem.rs Outdated Show resolved Hide resolved

pchickey and others added 3 commits February 9, 2024 15:26

fd_filestat_set test: assert that a file descriptor behaves the same...

9184be2

whether opened readonly or readwrite. This test now reproduces the behavior reported in #7829 Co-authored-by: Trevor Elliott <telliott@fastly.com>

pchickey force-pushed the pch/preview2_fs_dont_mask_perms branch from 7587c94 to 4eb836a Compare February 10, 2024 00:32

pchickey added this pull request to the merge queue Feb 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 10, 2024

pchickey and others added 4 commits February 11, 2024 10:41

different error on windows i guess?

ecf8ecc

prtest:full

apparently set-times of read-only is rejected on windows. just skip t…

848b67b

…esting that

path open read write: another alternate error code for windows

9be9282

wasi-common actually gives a badf, so accept either

87a0b62

pchickey force-pushed the pch/preview2_fs_dont_mask_perms branch from b7323b4 to 87a0b62 Compare February 12, 2024 20:44

this case too

c3de888

pchickey added this pull request to the merge queue Feb 13, 2024

Merged via the queue into main with commit dc9ad8e Feb 13, 2024

pchickey deleted the pch/preview2_fs_dont_mask_perms branch February 13, 2024 00:52

pchickey mentioned this pull request Feb 13, 2024

Backport to release-18: three WASI PRs #7928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preview 2 filesystem: track open_mode instead of reporting the permissions#7876

Preview 2 filesystem: track open_mode instead of reporting the permissions#7876
pchickey merged 8 commits intomainfrom
pch/preview2_fs_dont_mask_perms

pchickey commented Feb 6, 2024

Uh oh!

sunfishcode Feb 6, 2024 •

edited

Loading

Uh oh!

pchickey Feb 7, 2024 •

edited

Loading

Uh oh!

pchickey Feb 9, 2024

Uh oh!

pchickey commented Feb 9, 2024 •

edited

Loading

Uh oh!

alexcrichton left a comment

Uh oh!

Uh oh!

Uh oh!

brettcannon commented Feb 10, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

pchickey commented Feb 6, 2024

Uh oh!

sunfishcode Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchickey Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchickey Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

pchickey commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brettcannon commented Feb 10, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sunfishcode Feb 6, 2024 •

edited

Loading

pchickey Feb 7, 2024 •

edited

Loading

pchickey commented Feb 9, 2024 •

edited

Loading