Backport "Allow WASI to open directories without O_DIRECTORY" (#6163)

RELEASES.md

@@ -1,5 +1,23 @@
 --------------------------------------------------------------------------------
 
+## 8.0.1
+
+Released 2023-04-27.
+
+### Changed
+
+* Breaking: Files opened using Wasmtime's implementation of WASI on Windows now
+ cannot be deleted until the file handle is closed. This was already true for
+ open directories. The change was necessary for the bug fix in
+ [#6163](https://github.com/bytecodealliance/wasmtime/pull/6163).
+
+### Fixed
+
+* Fixed wasi-common's implementation of the `O_DIRECTORY` flag to match POSIX.
+ [#6163](https://github.com/bytecodealliance/wasmtime/pull/6163)
+
+--------------------------------------------------------------------------------
+
 ## 8.0.0
 
 Released 2023-04-20.

crates/test-programs/wasi-tests/src/bin/fd_readdir.rs

@@ -65,10 +65,9 @@ unsafe fn exec_fd_readdir(fd: wasi::Fd, cookie: wasi::Dircookie) -> (Vec<DirEntr
  (dirs, eof)
 }
 
-unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
+unsafe fn assert_empty_dir(dir_fd: wasi::Fd) {
  let stat = wasi::fd_filestat_get(dir_fd).expect("failed filestat");
 
- // Check the behavior in an empty directory
  let (mut dirs, eof) = exec_fd_readdir(dir_fd, 0);
  assert!(eof, "expected to read the entire directory");
  dirs.sort_by_key(|d| d.name.clone());
@@ -91,6 +90,11 @@ unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
  dirs.next().is_none(),
  "the directory should be seen as empty"
  );
+}
+
+unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
+ // Check the behavior in an empty directory
+ assert_empty_dir(dir_fd);
 
  // Add a file and check the behavior
  let file_fd = wasi::path_open(
@@ -111,16 +115,33 @@ unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
  "file descriptor range check",
  );
 
- let stat = wasi::fd_filestat_get(file_fd).expect("failed filestat");
+ let file_stat = wasi::fd_filestat_get(file_fd).expect("failed filestat");
  wasi::fd_close(file_fd).expect("closing a file");
 
+ wasi::path_create_directory(dir_fd, "nested").expect("create a directory");
+ let nested_fd = wasi::path_open(
+ dir_fd,
+ 0,
+ "nested",
+ 0,
+ wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_READDIR | wasi::RIGHTS_FD_FILESTAT_GET,
+ 0,
+ 0,
+ )
+ .expect("failed to open nested directory");
+ assert!(
+ nested_fd > file_fd,
+ "nested directory file descriptor range check",
+ );
+ let nested_stat = wasi::fd_filestat_get(nested_fd).expect("failed filestat");
+
  // Execute another readdir
  let (mut dirs, eof) = exec_fd_readdir(dir_fd, 0);
  assert!(eof, "expected to read the entire directory");
- assert_eq!(dirs.len(), 3, "expected three entries");
+ assert_eq!(dirs.len(), 4, "expected four entries");
  // Save the data about the last entry. We need to do it before sorting.
- let lastfile_cookie = dirs[1].dirent.d_next;
- let lastfile_name = dirs[2].name.clone();
+ let lastfile_cookie = dirs[2].dirent.d_next;
+ let lastfile_name = dirs[3].name.clone();
  dirs.sort_by_key(|d| d.name.clone());
  let mut dirs = dirs.into_iter();
 
@@ -136,15 +157,29 @@ unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
  wasi::FILETYPE_REGULAR_FILE,
  "type for the real file"
  );
- assert_eq!(dir.dirent.d_ino, stat.ino);
+ assert_eq!(dir.dirent.d_ino, file_stat.ino);
+ let dir = dirs.next().expect("fourth entry is None");
+ // check the directory info
+ assert_eq!(dir.name, "nested", "nested directory name doesn't match");
+ assert_eq!(
+ dir.dirent.d_type,
+ wasi::FILETYPE_DIRECTORY,
+ "type for the nested directory"
+ );
+ assert_eq!(dir.dirent.d_ino, nested_stat.ino);
 
  // check if cookie works as expected
  let (dirs, eof) = exec_fd_readdir(dir_fd, lastfile_cookie);
  assert!(eof, "expected to read the entire directory");
  assert_eq!(dirs.len(), 1, "expected one entry");
  assert_eq!(dirs[0].name, lastfile_name, "name of the only entry");
 
+ // check if nested directory shows up as empty
+ assert_empty_dir(nested_fd);
+ wasi::fd_close(nested_fd).expect("closing a nested directory");
+
  wasi::path_unlink_file(dir_fd, "file").expect("removing a file");
+ wasi::path_remove_directory(dir_fd, "nested").expect("removing a nested directory");
 }
 
 unsafe fn test_fd_readdir_lots(dir_fd: wasi::Fd) {

crates/test-programs/wasi-tests/src/bin/path_link.rs

@@ -91,9 +91,9 @@ unsafe fn test_path_link(dir_fd: wasi::Fd) {
  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);
+ 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
- wasi::fd_close(link_fd).expect("Closing link_fd"); // needed for Windows
  wasi::path_remove_directory(dir_fd, "subdir").expect("removing a subdirectory");
 
  // Create a link to a path that already exists

crates/test-programs/wasi-tests/src/bin/poll_oneoff_files.rs

@@ -231,6 +231,7 @@ unsafe fn test_fd_readwrite_valid_fd(dir_fd: wasi::Fd) {
  test_fd_readwrite(readable_fd, writable_fd, wasi::ERRNO_SUCCESS);
 
  wasi::fd_close(readable_fd).expect("closing readable_file");
+ wasi::fd_close(writable_fd).expect("closing writable_file");
  wasi::path_unlink_file(dir_fd, "readable_file").expect("removing readable_file");
  wasi::path_unlink_file(dir_fd, "writable_file").expect("removing writable_file");
 }

crates/wasi-common/cap-std-sync/src/dir.rs

@@ -1,18 +1,24 @@
 use crate::file::{filetype_from, File};
-use cap_fs_ext::{DirEntryExt, DirExt, MetadataExt, SystemTimeSpec};
+use cap_fs_ext::{DirEntryExt, DirExt, MetadataExt, OpenOptionsMaybeDirExt, SystemTimeSpec};
+use cap_std::fs;
 use std::any::Any;
 use std::path::{Path, PathBuf};
 use system_interface::fs::GetSetFdFlags;
 use wasi_common::{
  dir::{ReaddirCursor, ReaddirEntity, WasiDir},
- file::{FdFlags, FileType, Filestat, OFlags, WasiFile},
+ file::{FdFlags, FileType, Filestat, OFlags},
  Error, ErrorExt,
 };
 
-pub struct Dir(cap_std::fs::Dir);
+pub struct Dir(fs::Dir);
+
+pub enum OpenResult {
+ File(File),
+ Dir(Dir),
+}
 
 impl Dir {
- pub fn from_cap_std(dir: cap_std::fs::Dir) -> Self {
+ pub fn from_cap_std(dir: fs::Dir) -> Self {
  Dir(dir)
  }
 
@@ -24,10 +30,11 @@ impl Dir {
  read: bool,
  write: bool,
  fdflags: FdFlags,
- ) -> Result<File, Error> {
+ ) -> Result<OpenResult, Error> {
  use cap_fs_ext::{FollowSymlinks, OpenOptionsFollowExt};
 
- let mut opts = cap_std::fs::OpenOptions::new();
+ let mut opts = fs::OpenOptions::new();
+ opts.maybe_dir(true);
 
  if oflags.contains(OFlags::CREATE | OFlags::EXCLUSIVE) {
  opts.create_new(true);
@@ -71,22 +78,31 @@ impl Dir {
  return Err(Error::not_supported().context("SYNC family of FdFlags"));
  }
 
+ if oflags.contains(OFlags::DIRECTORY) {
+ if oflags.contains(OFlags::CREATE)
+ || oflags.contains(OFlags::EXCLUSIVE)
+ || oflags.contains(OFlags::TRUNCATE)
+ {
+ return Err(Error::invalid_argument().context("directory oflags"));
+ }
+ }
+
  let mut f = self.0.open_with(Path::new(path), &opts)?;
  // NONBLOCK does not have an OpenOption either, but we can patch that on with set_fd_flags:
  if fdflags.contains(wasi_common::file::FdFlags::NONBLOCK) {
  let set_fd_flags = f.new_set_fd_flags(system_interface::fs::FdFlags::NONBLOCK)?;
  f.set_fd_flags(set_fd_flags)?;
  }
- Ok(File::from_cap_std(f))
- }
 
- pub fn open_dir_(&self, symlink_follow: bool, path: &str) -> Result<Self, Error> {
- let d = if symlink_follow {
- self.0.open_dir(Path::new(path))?
+ if f.metadata()?.is_dir() {
+ Ok(OpenResult::Dir(Dir::from_cap_std(fs::Dir::from_std_file(
+ f.into_std(),
+ ))))
+ } else if oflags.contains(OFlags::DIRECTORY) {
+ Err(Error::not_dir().context("expected directory but got file"))
  } else {
- self.0.open_dir_nofollow(Path::new(path))?
- };
- Ok(Dir::from_cap_std(d))
+ Ok(OpenResult::File(File::from_cap_std(f)))
+ }
  }
 
  pub fn rename_(&self, src_path: &str, dest_dir: &Self, dest_path: &str) -> Result<(), Error> {
@@ -120,14 +136,12 @@ impl WasiDir for Dir {
  read: bool,
  write: bool,
  fdflags: FdFlags,
- ) -> Result<Box<dyn WasiFile>, Error> {
+ ) -> Result<wasi_common::dir::OpenResult, Error> {
  let f = self.open_file_(symlink_follow, path, oflags, read, write, fdflags)?;
- Ok(Box::new(f))
- }
-
- async fn open_dir(&self, symlink_follow: bool, path: &str) -> Result<Box<dyn WasiDir>, Error> {
- let d = self.open_dir_(symlink_follow, path)?;
- Ok(Box::new(d))
+ match f {
+ OpenResult::File(f) => Ok(wasi_common::dir::OpenResult::File(Box::new(f))),
+ OpenResult::Dir(d) => Ok(wasi_common::dir::OpenResult::Dir(Box::new(d))),
+ }
  }
 
  async fn create_dir(&self, path: &str) -> Result<(), Error> {
@@ -327,6 +341,7 @@ fn convert_systimespec(t: Option<wasi_common::SystemTimeSpec>) -> Option<SystemT
 mod test {
  use super::Dir;
  use cap_std::ambient_authority;
+ use wasi_common::file::{FdFlags, OFlags};
  #[test]
  fn scratch_dir() {
  let tempdir = tempfile::Builder::new()
@@ -336,8 +351,16 @@ mod test {
  let preopen_dir = cap_std::fs::Dir::open_ambient_dir(tempdir.path(), ambient_authority())
  .expect("open ambient temporary dir");
  let preopen_dir = Dir::from_cap_std(preopen_dir);
- run(wasi_common::WasiDir::open_dir(&preopen_dir, false, "."))
- .expect("open the same directory via WasiDir abstraction");
+ run(wasi_common::WasiDir::open_file(
+ &preopen_dir,
+ false,
+ ".",
+ OFlags::empty(),
+ false,
+ false,
+ FdFlags::empty(),
+ ))
+ .expect("open the same directory via WasiDir abstraction");
  }
 
  // Readdir does not work on windows, so we won't test it there.

crates/wasi-common/src/dir.rs

@@ -5,6 +5,11 @@ use std::any::Any;
 use std::path::PathBuf;
 use std::sync::{Arc, RwLock};
 
+pub enum OpenResult {
+ File(Box<dyn WasiFile>),
+ Dir(Box<dyn WasiDir>),
+}
+
 #[wiggle::async_trait]
 pub trait WasiDir: Send + Sync {
  fn as_any(&self) -> &dyn Any;
@@ -17,15 +22,7 @@ pub trait WasiDir: Send + Sync {
  _read: bool,
  _write: bool,
  _fdflags: FdFlags,
- ) -> Result<Box<dyn WasiFile>, Error> {
- Err(Error::not_supported())
- }
-
- async fn open_dir(
- &self,
- _symlink_follow: bool,
- _path: &str,
- ) -> Result<Box<dyn WasiDir>, Error> {
+ ) -> Result<OpenResult, Error> {
  Err(Error::not_supported())
  }
 

crates/wasi-common/src/snapshots/preview_1.rs

@@ -1,5 +1,8 @@
 use crate::{
- dir::{DirCaps, DirEntry, DirEntryExt, DirFdStat, ReaddirCursor, ReaddirEntity, TableDirExt},
+ dir::{
+ DirCaps, DirEntry, DirEntryExt, DirFdStat, OpenResult, ReaddirCursor, ReaddirEntity,
+ TableDirExt,
+ },
  file::{
  Advice, FdFlags, FdStat, FileCaps, FileEntry, FileEntryExt, FileType, Filestat, OFlags,
  RiFlags, RoFlags, SdFlags, SiFlags, TableFileExt, WasiFile,
@@ -741,41 +744,36 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
  let oflags = OFlags::from(&oflags);
  let fdflags = FdFlags::from(fdflags);
  let path = path.as_cow()?;
- if oflags.contains(OFlags::DIRECTORY) {
- if oflags.contains(OFlags::CREATE)
- || oflags.contains(OFlags::EXCLUSIVE)
- || oflags.contains(OFlags::TRUNCATE)
- {
- return Err(Error::invalid_argument().context("directory oflags"));
- }
- let dir_caps = dir_entry.child_dir_caps(DirCaps::from(&fs_rights_base));
- let file_caps = dir_entry.child_file_caps(FileCaps::from(&fs_rights_inheriting));
- let dir = dir_entry.get_cap(DirCaps::OPEN)?;
- let child_dir = dir.open_dir(symlink_follow, path.deref()).await?;
- drop(dir);
- let fd = table.push(Arc::new(DirEntry::new(
- dir_caps, file_caps, None, child_dir,
- )))?;
- Ok(types::Fd::from(fd))
- } else {
- let mut required_caps = DirCaps::OPEN;
- if oflags.contains(OFlags::CREATE) {
- required_caps = required_caps | DirCaps::CREATE_FILE;
- }
 
- let file_caps = dir_entry.child_file_caps(FileCaps::from(&fs_rights_base));
- let dir = dir_entry.get_cap(required_caps)?;
- let read = file_caps.contains(FileCaps::READ);
- let write = file_caps.contains(FileCaps::WRITE)
- || file_caps.contains(FileCaps::ALLOCATE)
- || file_caps.contains(FileCaps::FILESTAT_SET_SIZE);
- let file = dir
- .open_file(symlink_follow, path.deref(), oflags, read, write, fdflags)
- .await?;
- drop(dir);
- let fd = table.push(Arc::new(FileEntry::new(file_caps, file)))?;
- Ok(types::Fd::from(fd))
+ let mut required_caps = DirCaps::OPEN;
+ if oflags.contains(OFlags::CREATE) {
+ required_caps = required_caps | DirCaps::CREATE_FILE;
  }
+
+ let dir_dir_caps = dir_entry.child_dir_caps(DirCaps::from(&fs_rights_base));
+ let dir_file_caps = dir_entry.child_file_caps(FileCaps::from(&fs_rights_inheriting));
+ let file_caps = dir_entry.child_file_caps(FileCaps::from(&fs_rights_base));
+
+ let dir = dir_entry.get_cap(required_caps)?;
+ let read = file_caps.contains(FileCaps::READ);
+ let write = file_caps.contains(FileCaps::WRITE)
+ || file_caps.contains(FileCaps::ALLOCATE)
+ || file_caps.contains(FileCaps::FILESTAT_SET_SIZE);
+ let file = dir
+ .open_file(symlink_follow, path.deref(), oflags, read, write, fdflags)
+ .await?;
+ drop(dir);
+
+ let fd = match file {
+ OpenResult::File(file) => table.push(Arc::new(FileEntry::new(file_caps, file)))?,
+ OpenResult::Dir(child_dir) => table.push(Arc::new(DirEntry::new(
+ dir_dir_caps,
+ dir_file_caps,
+ None,
+ child_dir,
+ )))?,
+ };
+ Ok(types::Fd::from(fd))
  }
 
  async fn path_readlink<'a>(