Remove system-interface from wasmtime-wasi (#13109)

* Remove `system-interface` from `wasi-common`

This commit removes the `system-interface` dependency from
`wasi-common`, reimplementing necessary functionality within the crate
itself as appropriate. The goal of this commit is to trim our dependency
tree. The `system-interface` crate has not received an update in over a
year and continues to pull in an older `rustix` dependency for example.
Additionally I've personally found it confusing and surprising in the
past to trace through all the layers of abstractions from
`wasmtime-wasi` to the OS, and I'd like to start slimming this down to
be more local within Wasmtime rather than depending on a tree of crates.

The `system-interface` crate is a relatively thin wrapper around
`cap-std`-style crates providing a platform-agnostic API. This sometimes
fits what WASI wants, and sometimes doesn't. For example all
reads/writes to files within WASI currently require that Wasmtime
maintains a file cursor itself meaning that the underlying OS file
cursor doesn't actually matter. Reads and writes through the
`system-interface` abstraction, however, keep the cursor up-to-date to
have the same semantics across Unix and Windows which differ in the
behavior of the underlying syscalls. This is unnecessarily adds overhead
to Wasmtime's implementation of these APIs where they're otherwise not
required.

Effectively `system-interface` is not receiving much maintenance (old
dependency on `rustix` has persisted for ~1 year), its an extra layer of
abstraction to debug when issues arise, and its abstractions are not
always the best fit for WASI's semantics meaning that it can add
performance overhead. The replacement of inlining implementations within
Wasmtime is not too too costly and, personally, I view as worth it.

I'll note that this doesn't delete the `system-interface` crate
entirely. That would require removing it from `wasi-common` as well,
which is the subject of #13108.

* Portability fixes

prtest:full

* More portability fixes

* Allow some more errors on Windows

* Try to fix test on Windows

* Fix test issue

* Limit pwritev2 to linux

Author: alexcrichton

Cargo.lock

@@ -125,7 +125,9 @@ unsafe fn test_file_long_write(dir_fd: wasip1::Fd, filename: &str, blocking_mode
         }],
     );
     assert!(
-        res == Err(wasip1::ERRNO_BADF) || res == Err(wasip1::ERRNO_PERM),
+        res == Err(wasip1::ERRNO_BADF)
+            || res == Err(wasip1::ERRNO_PERM)
+            || res == Err(wasip1::ERRNO_ACCES),
         "bad result {res:?}"
     )
 }

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

@@ -51,7 +51,8 @@ unsafe fn test_path_open_read_write(dir_fd: wasip1::Fd, blocking_mode: BlockingM
             .err()
             .expect("read of writeonly fails"),
         wasip1::ERRNO_PERM,
-        wasip1::ERRNO_BADF
+        wasip1::ERRNO_BADF,
+        wasip1::ERRNO_ACCES
     );
 
     wasip1::fd_close(f_readonly).expect("close readonly");
@@ -89,7 +90,8 @@ unsafe fn test_path_open_read_write(dir_fd: wasip1::Fd, blocking_mode: BlockingM
             .err()
             .expect("read of writeonly fails"),
         wasip1::ERRNO_PERM,
-        wasip1::ERRNO_BADF
+        wasip1::ERRNO_BADF,
+        wasip1::ERRNO_ACCES
     );
     let bytes_written = blocking_mode
         .write(f_writeonly, &[ciovec])

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

@@ -12,41 +12,43 @@ impl test_programs::p3::exports::wasi::cli::run::Guest for Component {
         let (dir, _) = &preopens[0];
 
         let filename = "test.txt";
-        let file = dir
-            .open_at(
-                PathFlags::empty(),
-                filename.to_string(),
-                OpenFlags::CREATE,
-                DescriptorFlags::READ | DescriptorFlags::WRITE,
-            )
-            .await
-            .unwrap();
-        let (mut data_tx, data_rx) = wit_stream::new();
-        join!(
-            async {
-                file.write_via_stream(data_rx, 5).await.unwrap();
-            },
-            async {
-                let remaining = data_tx.write_all(b"Hello, ".to_vec()).await;
-                assert!(remaining.is_empty());
-                let remaining = data_tx.write_all(b"World!".to_vec()).await;
-                assert!(remaining.is_empty());
-                drop(data_tx);
-            },
-        );
-        let (data_rx, data_fut) = file.read_via_stream(0);
-        let contents = data_rx.collect().await;
-        data_fut.await.unwrap();
-        assert_eq!(
-            String::from_utf8_lossy(&contents),
-            "\0\0\0\0\0Hello, World!"
-        );
+        {
+            let file = dir
+                .open_at(
+                    PathFlags::empty(),
+                    filename.to_string(),
+                    OpenFlags::CREATE,
+                    DescriptorFlags::READ | DescriptorFlags::WRITE,
+                )
+                .await
+                .unwrap();
+            let (mut data_tx, data_rx) = wit_stream::new();
+            join!(
+                async {
+                    file.write_via_stream(data_rx, 5).await.unwrap();
+                },
+                async {
+                    let remaining = data_tx.write_all(b"Hello, ".to_vec()).await;
+                    assert!(remaining.is_empty());
+                    let remaining = data_tx.write_all(b"World!".to_vec()).await;
+                    assert!(remaining.is_empty());
+                    drop(data_tx);
+                },
+            );
+            let (data_rx, data_fut) = file.read_via_stream(0);
+            let contents = data_rx.collect().await;
+            data_fut.await.unwrap();
+            assert_eq!(
+                String::from_utf8_lossy(&contents),
+                "\0\0\0\0\0Hello, World!"
+            );
 
-        // Test that file read streams behave like other read streams.
-        let (data_rx, data_fut) = file.read_via_stream(5);
-        let contents = data_rx.collect().await;
-        data_fut.await.unwrap();
-        assert_eq!(String::from_utf8_lossy(&contents), "Hello, World!");
+            // Test that file read streams behave like other read streams.
+            let (data_rx, data_fut) = file.read_via_stream(5);
+            let contents = data_rx.collect().await;
+            data_fut.await.unwrap();
+            assert_eq!(String::from_utf8_lossy(&contents), "Hello, World!");
+        }
 
         dir.unlink_file_at(filename.to_string()).await.unwrap();
         Ok(())

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

@@ -34,10 +34,10 @@ io-lifetimes = { workspace = true }
 fs-set-times = { workspace = true }
 bitflags = { workspace = true }
 async-trait = { workspace = true }
-system-interface = { workspace = true}
 futures = { workspace = true }
 url = { workspace = true }
 rand = { workspace = true, features = ['std_rng', 'thread_rng'] }
+cfg-if = { workspace = true }
 
 [dev-dependencies]
 tokio = { workspace = true, features = ["time", "sync", "io-std", "io-util", "rt", "rt-multi-thread", "net", "macros", "fs"] }
@@ -54,9 +54,17 @@ rustix = { workspace = true, features = ["event", "fs", "net"] }
 
 [target.'cfg(windows)'.dependencies]
 io-extras = { workspace = true }
-windows-sys = { workspace = true }
 rustix = { workspace = true, features = ["event", "net"] }
 
+[target.'cfg(windows)'.dependencies.windows-sys]
+workspace = true
+features = [
+  "Wdk_Storage_FileSystem",
+  "Win32_Foundation",
+  "Win32_Storage_FileSystem",
+  "Win32_System_IO",
+]
+
 [features]
 default = ["p1", "p2"]
 p0 = ["p1"]

crates/wasi/Cargo.toml

@@ -8,6 +8,15 @@ use tracing::debug;
 use wasmtime::component::{HasData, Resource, ResourceTable};
 use wasmtime::error::Context as _;
 
+#[cfg(unix)]
+pub(crate) mod unix;
+#[cfg(unix)]
+pub(crate) use unix as sys;
+#[cfg(windows)]
+pub(crate) mod windows;
+#[cfg(windows)]
+pub(crate) use windows as sys;
+
 /// A helper struct which implements [`HasData`] for the `wasi:filesystem` APIs.
 ///
 /// This can be useful when directly calling `add_to_linker` functions directly,
@@ -350,6 +359,16 @@ impl From<&cap_std::fs::Metadata> for MetadataHashValue {
     }
 }
 
+#[derive(Copy, Clone, Debug)]
+pub(crate) enum Advice {
+    Normal,
+    Sequential,
+    Random,
+    WillNeed,
+    DontNeed,
+    NoReuse,
+}
+
 #[cfg(unix)]
 fn from_raw_os_error(err: Option<i32>) -> Option<ErrorCode> {
     use rustix::io::Errno as RustixErrno;
@@ -509,25 +528,9 @@ impl Descriptor {
     }
 
     pub(crate) async fn get_flags(&self) -> Result<DescriptorFlags, ErrorCode> {
-        use system_interface::fs::{FdFlags, GetSetFdFlags};
-
-        fn get_from_fdflags(flags: FdFlags) -> DescriptorFlags {
-            let mut out = DescriptorFlags::empty();
-            if flags.contains(FdFlags::DSYNC) {
-                out |= DescriptorFlags::REQUESTED_WRITE_SYNC;
-            }
-            if flags.contains(FdFlags::RSYNC) {
-                out |= DescriptorFlags::DATA_INTEGRITY_SYNC;
-            }
-            if flags.contains(FdFlags::SYNC) {
-                out |= DescriptorFlags::FILE_INTEGRITY_SYNC;
-            }
-            out
-        }
         match self {
             Self::File(f) => {
-                let flags = f.run_blocking(|f| f.get_fd_flags()).await?;
-                let mut flags = get_from_fdflags(flags);
+                let mut flags = f.run_blocking(|f| sys::get_flags(f)).await?;
                 if f.open_mode.contains(OpenMode::READ) {
                     flags |= DescriptorFlags::READ;
                 }
@@ -537,8 +540,7 @@ impl Descriptor {
                 Ok(flags)
             }
             Self::Dir(d) => {
-                let flags = d.run_blocking(|d| d.get_fd_flags()).await?;
-                let mut flags = get_from_fdflags(flags);
+                let mut flags = d.run_blocking(|d| sys::get_flags(d)).await?;
                 if d.open_mode.contains(OpenMode::READ) {
                     flags |= DescriptorFlags::READ;
                 }
@@ -747,10 +749,9 @@ impl File {
         &self,
         offset: u64,
         len: u64,
-        advice: system_interface::fs::Advice,
+        advice: Advice,
     ) -> Result<(), ErrorCode> {
-        use system_interface::fs::FileIoExt as _;
-        self.run_blocking(move |f| f.advise(offset, len, advice))
+        self.run_blocking(move |f| sys::advise(f, offset, len, advice))
             .await?;
         Ok(())
     }
@@ -930,7 +931,6 @@ impl Dir {
         allow_blocking_current_thread: bool,
     ) -> Result<Descriptor, ErrorCode> {
         use cap_fs_ext::{FollowSymlinks, OpenOptionsFollowExt, OpenOptionsMaybeDirExt};
-        use system_interface::fs::{FdFlags, GetSetFdFlags};
 
         if !self.perms.contains(DirPerms::READ) {
             return Err(ErrorCode::NotPermitted);
@@ -1023,18 +1023,14 @@ impl Dir {
 
         let opened = self
             .run_blocking::<_, std::io::Result<OpenResult>>(move |d| {
-                let mut opened = d.open_with(&path, &opts)?;
+                let opened = d.open_with(&path, &opts)?;
                 if opened.metadata()?.is_dir() {
                     Ok(OpenResult::Dir(cap_std::fs::Dir::from_std_file(
                         opened.into_std(),
                     )))
                 } else if oflags.contains(OpenFlags::DIRECTORY) {
                     Ok(OpenResult::NotDir)
                 } else {
-                    // FIXME cap-std needs a nonblocking open option so that files reads and writes
-                    // are nonblocking. Instead we set it after opening here:
-                    let set_fd_flags = opened.new_set_fd_flags(FdFlags::NONBLOCK)?;
-                    opened.set_fd_flags(set_fd_flags)?;
                     Ok(OpenResult::File(opened))
                 }
             })

crates/wasi/src/filesystem.rs

@@ -0,0 +1,100 @@
+use crate::filesystem::{Advice, DescriptorFlags};
+use io_lifetimes::AsFilelike;
+use rustix::fs::{OFlags, fcntl_getfl, fcntl_setfl};
+use rustix::io::write;
+use std::fs::File;
+use std::io;
+use std::os::fd::AsFd;
+use std::os::unix::fs::FileExt;
+
+pub(crate) fn get_flags(file: impl AsFd) -> io::Result<DescriptorFlags> {
+    let flags = fcntl_getfl(file)?;
+    let mut ret = DescriptorFlags::empty();
+    ret.set(
+        DescriptorFlags::REQUESTED_WRITE_SYNC,
+        flags.contains(OFlags::DSYNC),
+    );
+    ret.set(
+        DescriptorFlags::FILE_INTEGRITY_SYNC,
+        flags.contains(OFlags::SYNC),
+    );
+    #[cfg(not(any(target_vendor = "apple", target_os = "freebsd")))]
+    ret.set(
+        DescriptorFlags::DATA_INTEGRITY_SYNC,
+        flags.contains(OFlags::RSYNC),
+    );
+    Ok(ret)
+}
+
+pub(crate) fn advise(file: impl AsFd, offset: u64, len: u64, advice: Advice) -> io::Result<()> {
+    cfg_if::cfg_if! {
+        if #[cfg(target_vendor = "apple")] {
+            match advice {
+                Advice::WillNeed => {
+                    rustix::fs::fcntl_rdadvise(file, offset, len)?;
+                }
+                Advice::Normal |
+                Advice::Sequential |
+                Advice::Random |
+                Advice::DontNeed |
+                Advice::NoReuse => {}
+            }
+        } else if #[cfg(any(target_os = "linux", target_os = "android"))] {
+            use std::num::NonZeroU64;
+            let advice = match advice {
+                Advice::Normal => rustix::fs::Advice::Normal,
+                Advice::Sequential => rustix::fs::Advice::Sequential,
+                Advice::Random => rustix::fs::Advice::Random,
+                Advice::WillNeed => rustix::fs::Advice::WillNeed,
+                Advice::DontNeed => rustix::fs::Advice::DontNeed,
+                Advice::NoReuse => rustix::fs::Advice::NoReuse,
+            };
+            rustix::fs::fadvise(file, offset, NonZeroU64::new(len), advice)?;
+        } else {
+            // noop on other platforms
+            let _ = (file, offset, len, advice);
+        }
+    }
+    Ok(())
+}
+
+pub(crate) fn append_cursor_unspecified(file: impl AsFd, data: &[u8]) -> io::Result<usize> {
+    // On Linux, use `pwritev2`.
+    #[cfg(target_os = "linux")]
+    {
+        use rustix::io::{Errno, ReadWriteFlags, pwritev2};
+        use std::io::IoSlice;
+
+        let iovs = [IoSlice::new(data)];
+        match pwritev2(&file, &iovs, 0, ReadWriteFlags::APPEND) {
+            Err(Errno::NOSYS) | Err(Errno::NOTSUP) => {}
+            otherwise => return Ok(otherwise?),
+        }
+    }
+
+    // Otherwise use `F_SETFL` to switch the file description to append
+    // mode, do the write, and switch back. This is not atomic with
+    // respect to other users of the file description, but WASI isn't fully
+    // threaded right now anyway.
+    let old_flags = fcntl_getfl(&file)?;
+    fcntl_setfl(&file, old_flags | OFlags::APPEND)?;
+    let result = write(&file, data);
+    fcntl_setfl(&file, old_flags).unwrap();
+    Ok(result?)
+}
+
+pub(crate) fn write_at_cursor_unspecified(
+    file: impl AsFd,
+    data: &[u8],
+    pos: u64,
+) -> io::Result<usize> {
+    file.as_filelike_view::<File>().write_at(data, pos)
+}
+
+pub(crate) fn read_at_cursor_unspecified(
+    file: impl AsFd,
+    buf: &mut [u8],
+    pos: u64,
+) -> io::Result<usize> {
+    file.as_filelike_view::<File>().read_at(buf, pos)
+}