Skip to content

Commit

Permalink
Allow WASI to open directories without O_DIRECTORY (bytecodealliance#…
Browse files Browse the repository at this point in the history
…6163)

* Allow WASI to open directories without O_DIRECTORY

The `O_DIRECTORY` flag is a request that open should fail if the named
path is not a directory. Opening a path which turns out to be a
directory is not supposed to fail if this flag is not specified.
However, wasi-common required callers to use it when opening
directories.

With this PR, we always open the path the same way whether or not the
`O_DIRECTORY` flag is specified. However, after opening it, we `stat` it
to check whether it turned out to be a directory, and determine which
operations the file descriptor should support accordingly. In addition,
we explicitly check whether the precondition defined by `O_DIRECTORY` is
satisfied.

Closes bytecodealliance#4947 and closes bytecodealliance#4967, which were earlier attempts at fixing the
same issue, but which had race conditions.

prtest:full

* Add tests from bytecodealliance#4967/bytecodealliance#4947

This test was authored by Roman Volosatovs <rvolosatovs@riseup.net> as
part of bytecodealliance#4947.

* Tests: Close FDs before trying to unlink files

On Windows, when opening a path which might be a directory using
`CreateFile`, cap-primitives also removes the `FILE_SHARE_DELETE` mode.

That means that if we implement WASI's `path_open` such that it always
uses `CreateFile` on Windows, for both files and directories, then
holding an open file handle prevents deletion of that file.

So I'm changing these test programs to make sure they've closed the
handle before trying to delete the file.
  • Loading branch information
jameysharp authored and afonso360 committed Apr 24, 2023
1 parent 1ce2d3c commit 3effe3f
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 93 deletions.
49 changes: 42 additions & 7 deletions crates/test-programs/wasi-tests/src/bin/fd_readdir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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(
Expand All @@ -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();

Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion crates/test-programs/wasi-tests/src/bin/path_link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
69 changes: 46 additions & 23 deletions crates/wasi-common/cap-std-sync/src/dir.rs
Original file line number Diff line number Diff line change
@@ -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)
}

Expand All @@ -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);
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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()
Expand All @@ -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.
Expand Down
15 changes: 6 additions & 9 deletions crates/wasi-common/src/dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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())
}

Expand Down
66 changes: 32 additions & 34 deletions crates/wasi-common/src/snapshots/preview_1.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -747,41 +750,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>(
Expand Down
Loading

0 comments on commit 3effe3f

Please sign in to comment.