Preview 2 filesystem: track open_mode instead of reporting the permis…

…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]>
bytecodealliance · Feb 13, 2024 · dc9ad8e · dc9ad8e
1 parent 04c03b3
commit dc9ad8e
Show file tree

Hide file tree

Showing 6 changed files with 239 additions and 110 deletions.
diff --git a/crates/test-programs/src/bin/preview1_fd_filestat_set.rs b/crates/test-programs/src/bin/preview1_fd_filestat_set.rs
@@ -1,9 +1,10 @@
 use std::{env, process, time::Duration};
-use test_programs::preview1::{assert_fs_time_eq, open_scratch_directory, TestConfig};
+use test_programs::preview1::{
+    assert_errno, assert_fs_time_eq, open_scratch_directory, TestConfig,
+};
 
-unsafe fn test_fd_filestat_set(dir_fd: wasi::Fd) {
-    let cfg = TestConfig::from_env();
-    // Create a file in the scratch directory.
+unsafe fn test_fd_filestat_set_size_rw(dir_fd: wasi::Fd) {
+    // Create a file in the scratch directory, opened read/write
     let file_fd = wasi::path_open(
         dir_fd,
         0,
@@ -29,6 +30,56 @@ unsafe fn test_fd_filestat_set(dir_fd: wasi::Fd) {
     let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat 2");
     assert_eq!(stat.size, 100, "file size should be 100");
 
+    wasi::fd_close(file_fd).expect("failed to close fd");
+    wasi::path_unlink_file(dir_fd, "file").expect("failed to remove file");
+}
+
+unsafe fn test_fd_filestat_set_size_ro(dir_fd: wasi::Fd) {
+    // Create a file in the scratch directory. Creating a file implies opening it for writing, so
+    // we have to close and re-open read-only to observe read-only behavior.
+    let file_fd = wasi::path_open(dir_fd, 0, "file", wasi::OFLAGS_CREAT, 0, 0, 0)
+        .expect("failed to create file");
+    wasi::fd_close(file_fd).expect("failed to close fd");
+
+    // Open the created file read-only
+    let file_fd = wasi::path_open(dir_fd, 0, "file", 0, wasi::RIGHTS_FD_READ, 0, 0)
+        .expect("failed to create file");
+
+    // Check file size
+    let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat");
+    assert_eq!(stat.size, 0, "file size should be 0");
+
+    // Check fd_filestat_set_size on a file opened read-only fails with EINVAL, like ftruncate is defined to do on posix
+    assert_errno!(
+        wasi::fd_filestat_set_size(file_fd, 100)
+            .expect_err("fd_filestat_set_size should error when file is opened read-only"),
+        windows => wasi::ERRNO_ACCES,
+        wasi::ERRNO_INVAL
+    );
+
+    let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat 2");
+    assert_eq!(stat.size, 0, "file size should remain 0");
+
+    wasi::fd_close(file_fd).expect("failed to close fd");
+    wasi::path_unlink_file(dir_fd, "file").expect("failed to remove file");
+}
+
+unsafe fn test_fd_filestat_set_times(dir_fd: wasi::Fd, rights: wasi::Rights) {
+    let cfg = TestConfig::from_env();
+
+    // Create a file in the scratch directory. OFLAGS_CREAT implies opening for writing, so we will
+    // close it and re-open with the desired rights (FD_READ for read only, FD_READ | FD_WRITE for
+    // readwrite)
+    let file_fd = wasi::path_open(dir_fd, 0, "file", wasi::OFLAGS_CREAT, 0, 0, 0)
+        .expect("failed to create file");
+    wasi::fd_close(file_fd).expect("failed to close fd");
+
+    // Open the file with the rights given.
+    let file_fd =
+        wasi::path_open(dir_fd, 0, "file", 0, rights, 0, 0).expect("failed to create file");
+
+    let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat 2");
+
     // Check fd_filestat_set_times
     let old_atim = Duration::from_nanos(stat.atim);
     let new_mtim = Duration::from_nanos(stat.mtim) - cfg.fs_time_precision() * 2;
@@ -41,7 +92,7 @@ unsafe fn test_fd_filestat_set(dir_fd: wasi::Fd) {
     .expect("fd_filestat_set_times");
 
     let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat 3");
-    assert_eq!(stat.size, 100, "file size should remain unchanged at 100");
+    assert_eq!(stat.size, 0, "file size should remain unchanged at 0");
 
     // Support accuracy up to at least 1ms
     assert_fs_time_eq!(
@@ -59,7 +110,7 @@ unsafe fn test_fd_filestat_set(dir_fd: wasi::Fd) {
     // assert_eq!(status, wasi::EINVAL, "ATIM & ATIM_NOW can't both be set");
 
     wasi::fd_close(file_fd).expect("failed to close fd");
-    wasi::path_unlink_file(dir_fd, "file").expect("failed to remove dir");
+    wasi::path_unlink_file(dir_fd, "file").expect("failed to remove file");
 }
 fn main() {
     let mut args = env::args();
@@ -81,5 +132,18 @@ fn main() {
     };
 
     // Run the tests.
-    unsafe { test_fd_filestat_set(dir_fd) }
+
+    unsafe { test_fd_filestat_set_size_rw(dir_fd) }
+    unsafe { test_fd_filestat_set_size_ro(dir_fd) }
+
+    // The fd_filestat_set_times function should behave the same whether the file
+    // 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.
+    if test_programs::preview1::config().support_dangling_filesystem() {
+        // Guarding to run on non-windows filesystems. Windows rejects set-times on read-only
+        // files.
+        unsafe { test_fd_filestat_set_times(dir_fd, wasi::RIGHTS_FD_READ) }
+    }
+    unsafe { test_fd_filestat_set_times(dir_fd, wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE) }
 }
diff --git a/crates/test-programs/src/bin/preview1_path_link.rs b/crates/test-programs/src/bin/preview1_path_link.rs
@@ -1,89 +1,82 @@
 use std::{env, process};
 use test_programs::preview1::{assert_errno, config, create_file, open_scratch_directory};
 
-unsafe fn create_or_open(dir_fd: wasi::Fd, name: &str, flags: wasi::Oflags) -> wasi::Fd {
-    let file_fd = wasi::path_open(dir_fd, 0, name, flags, 0, 0, 0)
-        .unwrap_or_else(|_| panic!("opening '{}'", name));
-    assert!(
-        file_fd > libc::STDERR_FILENO as wasi::Fd,
-        "file descriptor range check",
-    );
-    file_fd
-}
-
-unsafe fn open_link(dir_fd: wasi::Fd, name: &str) -> wasi::Fd {
-    let file_fd = wasi::path_open(dir_fd, 0, name, 0, 0, 0, 0)
-        .unwrap_or_else(|_| panic!("opening a link '{}'", name));
-    assert!(
-        file_fd > libc::STDERR_FILENO as wasi::Fd,
-        "file descriptor range check",
-    );
-    file_fd
-}
-
-// This is temporary until `wasi` implements `Debug` and `PartialEq` for
-// `wasi::Filestat`.
-fn filestats_assert_eq(left: wasi::Filestat, right: wasi::Filestat) {
-    assert_eq!(left.dev, right.dev, "dev should be equal");
-    assert_eq!(left.ino, right.ino, "ino should be equal");
-    assert_eq!(left.atim, right.atim, "atim should be equal");
-    assert_eq!(left.ctim, right.ctim, "ctim should be equal");
-    assert_eq!(left.mtim, right.mtim, "mtim should be equal");
-    assert_eq!(left.size, right.size, "size should be equal");
-    assert_eq!(left.nlink, right.nlink, "nlink should be equal");
-    assert_eq!(left.filetype, right.filetype, "filetype should be equal");
+// These are all macro-rules so the panic line number shows us where
+// things went wrong.
+
+macro_rules! filestats_assert_eq {
+    ($left:ident, $right:ident) => {
+        assert_eq!($left.dev, $right.dev, "dev should be equal");
+        assert_eq!($left.ino, $right.ino, "ino should be equal");
+        assert_eq!($left.atim, $right.atim, "atim should be equal");
+        assert_eq!($left.ctim, $right.ctim, "ctim should be equal");
+        assert_eq!($left.mtim, $right.mtim, "mtim should be equal");
+        assert_eq!($left.size, $right.size, "size should be equal");
+        assert_eq!($left.nlink, $right.nlink, "nlink should be equal");
+        assert_eq!($left.filetype, $right.filetype, "filetype should be equal");
+    };
 }
 
-// This is temporary until `wasi` implements `Debug` and `PartialEq` for
-// `wasi::Fdstat`.
-fn fdstats_assert_eq(left: wasi::Fdstat, right: wasi::Fdstat) {
-    assert_eq!(left.fs_flags, right.fs_flags, "fs_flags should be equal");
-    assert_eq!(
-        left.fs_filetype, right.fs_filetype,
-        "fs_filetype should be equal"
-    );
-    assert_eq!(
-        left.fs_rights_base, right.fs_rights_base,
-        "fs_rights_base should be equal"
-    );
-    assert_eq!(
-        left.fs_rights_inheriting, right.fs_rights_inheriting,
-        "fs_rights_inheriting should be equal"
-    );
+macro_rules! fdstats_assert_eq {
+    ($left:ident, $right:ident) => {
+        assert_eq!($left.fs_flags, $right.fs_flags, "fs_flags should be equal");
+        assert_eq!(
+            $left.fs_filetype, $right.fs_filetype,
+            "fs_filetype should be equal"
+        );
+        assert_eq!(
+            $left.fs_rights_base, $right.fs_rights_base,
+            "fs_rights_base should be equal"
+        );
+        assert_eq!(
+            $left.fs_rights_inheriting, $right.fs_rights_inheriting,
+            "fs_rights_inheriting should be equal"
+        );
+    };
 }
 
-unsafe fn check_rights(orig_fd: wasi::Fd, link_fd: wasi::Fd) {
-    // Compare Filestats
-    let orig_filestat = wasi::fd_filestat_get(orig_fd).expect("reading filestat of the source");
-    let link_filestat = wasi::fd_filestat_get(link_fd).expect("reading filestat of the link");
-    filestats_assert_eq(orig_filestat, link_filestat);
-
-    // Compare Fdstats
-    let orig_fdstat = wasi::fd_fdstat_get(orig_fd).expect("reading fdstat of the source");
-    let link_fdstat = wasi::fd_fdstat_get(link_fd).expect("reading fdstat of the link");
-    fdstats_assert_eq(orig_fdstat, link_fdstat);
+macro_rules! check_rights {
+    ($orig_fd:ident, $link_fd:ident) => {
+        let orig_filestat =
+            wasi::fd_filestat_get($orig_fd).expect("reading filestat of the source");
+        let link_filestat = wasi::fd_filestat_get($link_fd).expect("reading filestat of the link");
+        filestats_assert_eq!(orig_filestat, link_filestat);
+
+        // Compare Fdstats
+        let orig_fdstat = wasi::fd_fdstat_get($orig_fd).expect("reading fdstat of the source");
+        let link_fdstat = wasi::fd_fdstat_get($link_fd).expect("reading fdstat of the link");
+        fdstats_assert_eq!(orig_fdstat, link_fdstat);
+    };
 }
-
 // Extra calls of fd_close are needed for Windows, which will not remove
 // the directory until all handles are closed.
 unsafe fn test_path_link(dir_fd: wasi::Fd) {
     // Create a file
-    let file_fd = create_or_open(dir_fd, "file", wasi::OFLAGS_CREAT);
+    let create_fd =
+        wasi::path_open(dir_fd, 0, "file", wasi::OFLAGS_CREAT, 0, 0, 0).expect("create file");
+    wasi::fd_close(create_fd).unwrap();
+
+    // Open a fresh descriptor to the file. We won't have a write right that was implied by OFLAGS_CREAT
+    // above.
+    let file_fd = wasi::path_open(dir_fd, 0, "file", 0, 0, 0, 0).expect("open file");
 
     // Create a link in the same directory and compare rights
     wasi::path_link(dir_fd, 0, "file", dir_fd, "link")
         .expect("creating a link in the same directory");
-    let link_fd = open_link(dir_fd, "link");
-    check_rights(file_fd, link_fd);
+
+    let link_fd = wasi::path_open(dir_fd, 0, "link", 0, 0, 0, 0).expect("open link");
+
+    check_rights!(file_fd, link_fd);
     wasi::fd_close(link_fd).expect("Closing link_fd"); // needed for Windows
     wasi::path_unlink_file(dir_fd, "link").expect("removing a link");
 
     // Create a link in a different directory and compare rights
     wasi::path_create_directory(dir_fd, "subdir").expect("creating a subdirectory");
-    let subdir_fd = create_or_open(dir_fd, "subdir", wasi::OFLAGS_DIRECTORY);
+    let subdir_fd = wasi::path_open(dir_fd, 0, "subdir", wasi::OFLAGS_DIRECTORY, 0, 0, 0)
+        .expect("open subdir directory");
     wasi::path_link(dir_fd, 0, "file", subdir_fd, "link").expect("creating a link in subdirectory");
-    let link_fd = open_link(subdir_fd, "link");
-    check_rights(file_fd, link_fd);
+    let link_fd = wasi::path_open(subdir_fd, 0, "link", 0, 0, 0, 0).expect("open link in subdir");
+    check_rights!(file_fd, link_fd);
     wasi::fd_close(link_fd).expect("Closing link_fd"); // needed for Windows
     wasi::path_unlink_file(subdir_fd, "link").expect("removing a link");
     wasi::fd_close(subdir_fd).expect("Closing subdir_fd"); // needed for Windows
@@ -118,7 +111,8 @@ unsafe fn test_path_link(dir_fd: wasi::Fd) {
 
     // Create a link to a directory
     wasi::path_create_directory(dir_fd, "subdir").expect("creating a subdirectory");
-    let subdir_fd = create_or_open(dir_fd, "subdir", wasi::OFLAGS_DIRECTORY);
+    let subdir_fd = wasi::path_open(dir_fd, 0, "subdir", wasi::OFLAGS_DIRECTORY, 0, 0, 0)
+        .expect("open new descriptor to subdir");
 
     assert_errno!(
         wasi::path_link(dir_fd, 0, "subdir", dir_fd, "link")

diff --git a/crates/test-programs/src/bin/preview1_path_open_read_write.rs b/crates/test-programs/src/bin/preview1_path_open_read_write.rs
@@ -30,10 +30,14 @@ unsafe fn test_path_open_read_write(dir_fd: wasi::Fd) {
         buf: write_buffer.as_ptr(),
         buf_len: write_buffer.len(),
     };
+    // PERM is only the failure on windows under wasmtime-wasi. wasi-common
+    // fails on windows with BADF, so we cant use the `windows =>` syntax
+    // because that doesnt support alternatives like the agnostic syntax does.
     assert_errno!(
         wasi::fd_write(f_readonly, &[ciovec])
             .err()
             .expect("read of writeonly fails"),
+        wasi::ERRNO_PERM,
         wasi::ERRNO_BADF
     );
 
@@ -53,10 +57,12 @@ unsafe fn test_path_open_read_write(dir_fd: wasi::Fd) {
         "writeonly has write right"
     );
 
+    // See above for description of PERM
     assert_errno!(
         wasi::fd_read(f_writeonly, &[iovec])
             .err()
             .expect("read of writeonly fails"),
+        wasi::ERRNO_PERM,
         wasi::ERRNO_BADF
     );
     let bytes_written = wasi::fd_write(f_writeonly, &[ciovec]).expect("write to writeonly");

diff --git a/crates/wasi/src/preview2/ctx.rs b/crates/wasi/src/preview2/ctx.rs
@@ -3,7 +3,7 @@ use crate::preview2::{
         host::{monotonic_clock, wall_clock},
         HostMonotonicClock, HostWallClock,
     },
-    filesystem::Dir,
+    filesystem::{Dir, OpenMode},
     network::{SocketAddrCheck, SocketAddrUse},
     pipe, random, stdio,
     stdio::{StdinStream, StdoutStream},
@@ -144,8 +144,17 @@ impl WasiCtxBuilder {
         file_perms: FilePerms,
         path: impl AsRef<str>,
     ) -> &mut Self {
-        self.preopens
-            .push((Dir::new(dir, perms, file_perms), path.as_ref().to_owned()));
+        let mut open_mode = OpenMode::empty();
+        if perms.contains(DirPerms::READ) {
+            open_mode |= OpenMode::READ;
+        }
+        if perms.contains(DirPerms::MUTATE) {
+            open_mode |= OpenMode::WRITE;
+        }
+        self.preopens.push((
+            Dir::new(dir, perms, file_perms, open_mode),
+            path.as_ref().to_owned(),
+        ));
         self
     }