From 3effe3f68746f9252122bd139e0a220250cf43d1 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Fri, 21 Apr 2023 09:55:35 -0700 Subject: [PATCH] Allow WASI to open directories without O_DIRECTORY (#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 #4947 and closes #4967, which were earlier attempts at fixing the same issue, but which had race conditions. prtest:full * Add tests from #4967/#4947 This test was authored by Roman Volosatovs as part of #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. --- .../wasi-tests/src/bin/fd_readdir.rs | 49 +++++++++++-- .../wasi-tests/src/bin/path_link.rs | 2 +- .../wasi-tests/src/bin/poll_oneoff_files.rs | 1 + crates/wasi-common/cap-std-sync/src/dir.rs | 69 ++++++++++++------- crates/wasi-common/src/dir.rs | 15 ++-- crates/wasi-common/src/snapshots/preview_1.rs | 66 +++++++++--------- crates/wasi-common/tokio/src/dir.rs | 34 +++++---- crates/wasi-common/tokio/tests/poll_oneoff.rs | 27 ++++++-- 8 files changed, 170 insertions(+), 93 deletions(-) diff --git a/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs b/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs index 87aa112c2b04..bc4bdd2d8524 100644 --- a/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs +++ b/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 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,7 +157,16 @@ 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); @@ -144,7 +174,12 @@ unsafe fn test_fd_readdir(dir_fd: wasi::Fd) { 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) { diff --git a/crates/test-programs/wasi-tests/src/bin/path_link.rs b/crates/test-programs/wasi-tests/src/bin/path_link.rs index 1216c334f95e..d229ba8aa5bb 100644 --- a/crates/test-programs/wasi-tests/src/bin/path_link.rs +++ b/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 diff --git a/crates/test-programs/wasi-tests/src/bin/poll_oneoff_files.rs b/crates/test-programs/wasi-tests/src/bin/poll_oneoff_files.rs index cbf9803130cf..8709fd8603d4 100644 --- a/crates/test-programs/wasi-tests/src/bin/poll_oneoff_files.rs +++ b/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"); } diff --git a/crates/wasi-common/cap-std-sync/src/dir.rs b/crates/wasi-common/cap-std-sync/src/dir.rs index 750bd3786d7b..18a1f8a5348b 100644 --- a/crates/wasi-common/cap-std-sync/src/dir.rs +++ b/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 { + ) -> Result { 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 { - 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, Error> { + ) -> Result { 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, 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) -> Option), + Dir(Box), +} + #[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, Error> { - Err(Error::not_supported()) - } - - async fn open_dir( - &self, - _symlink_follow: bool, - _path: &str, - ) -> Result, Error> { + ) -> Result { Err(Error::not_supported()) } diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs index 4f5efa6d0973..dcda64d06ab2 100644 --- a/crates/wasi-common/src/snapshots/preview_1.rs +++ b/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, @@ -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>( diff --git a/crates/wasi-common/tokio/src/dir.rs b/crates/wasi-common/tokio/src/dir.rs index f95e61219eaa..5482a08c7619 100644 --- a/crates/wasi-common/tokio/src/dir.rs +++ b/crates/wasi-common/tokio/src/dir.rs @@ -3,7 +3,7 @@ use std::any::Any; use std::path::PathBuf; use wasi_common::{ dir::{ReaddirCursor, ReaddirEntity, WasiDir}, - file::{FdFlags, Filestat, OFlags, WasiFile}, + file::{FdFlags, Filestat, OFlags}, Error, ErrorExt, }; @@ -28,18 +28,19 @@ impl WasiDir for Dir { read: bool, write: bool, fdflags: FdFlags, - ) -> Result, Error> { + ) -> Result { let f = block_on_dummy_executor(move || async move { self.0 .open_file_(symlink_follow, path, oflags, read, write, fdflags) })?; - Ok(Box::new(File::from_inner(f))) - } - - async fn open_dir(&self, symlink_follow: bool, path: &str) -> Result, Error> { - let d = - block_on_dummy_executor(move || async move { self.0.open_dir_(symlink_follow, path) })?; - Ok(Box::new(Dir(d))) + match f { + wasi_cap_std_sync::dir::OpenResult::File(f) => Ok(wasi_common::dir::OpenResult::File( + Box::new(File::from_inner(f)), + )), + wasi_cap_std_sync::dir::OpenResult::Dir(d) => { + Ok(wasi_common::dir::OpenResult::Dir(Box::new(Dir(d)))) + } + } } async fn create_dir(&self, path: &str) -> Result<(), Error> { @@ -127,6 +128,7 @@ impl WasiDir for Dir { mod test { use super::Dir; use cap_std::ambient_authority; + use wasi_common::file::{FdFlags, OFlags}; #[tokio::test(flavor = "multi_thread")] async fn scratch_dir() { @@ -137,9 +139,17 @@ 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); - wasi_common::WasiDir::open_dir(&preopen_dir, false, ".") - .await - .expect("open the same directory via WasiDir abstraction"); + wasi_common::WasiDir::open_file( + &preopen_dir, + false, + ".", + OFlags::empty(), + false, + false, + FdFlags::empty(), + ) + .await + .expect("open the same directory via WasiDir abstraction"); } // Readdir does not work on windows, so we won't test it there. diff --git a/crates/wasi-common/tokio/tests/poll_oneoff.rs b/crates/wasi-common/tokio/tests/poll_oneoff.rs index f305b7c44d3d..852abce2aad2 100644 --- a/crates/wasi-common/tokio/tests/poll_oneoff.rs +++ b/crates/wasi-common/tokio/tests/poll_oneoff.rs @@ -2,6 +2,7 @@ use anyhow::{Context, Error}; use cap_std::time::Duration; use std::collections::HashMap; use wasi_common::{ + dir::OpenResult, file::{FdFlags, OFlags}, sched::{Poll, RwEventFlags, SubscriptionResult, Userdata}, WasiDir, WasiFile, @@ -25,18 +26,26 @@ async fn empty_file_readable() -> Result<(), Error> { .await .context("create writable file f")?; let to_write: Vec = vec![0]; - f.write_vectored(&vec![std::io::IoSlice::new(&to_write)]) - .await - .context("write to f")?; + if let OpenResult::File(ref f) = f { + f.write_vectored(&vec![std::io::IoSlice::new(&to_write)]) + .await + .context("write to f")?; + } else { + unreachable!(); + } drop(f); - let mut f = d + let f = d .open_file(false, "f", OFlags::empty(), true, false, FdFlags::empty()) .await .context("open f as readable")?; let mut poll = Poll::new(); - poll.subscribe_read(&mut *f, Userdata::from(123)); + if let OpenResult::File(ref f) = f { + poll.subscribe_read(f.as_ref(), Userdata::from(123)); + } else { + unreachable!(); + } // Timeout bounds time in poll_oneoff let monotonic = &*clocks.monotonic()?.abs_clock; poll.subscribe_monotonic_clock( @@ -73,13 +82,17 @@ async fn empty_file_writable() -> Result<(), Error> { let d = workspace.open_dir("d").context("open dir")?; let d = Dir::from_cap_std(d); - let mut writable_f = d + let writable_f = d .open_file(false, "f", OFlags::CREATE, true, true, FdFlags::empty()) .await .context("create writable file")?; let mut poll = Poll::new(); - poll.subscribe_write(&mut *writable_f, Userdata::from(123)); + if let OpenResult::File(ref writable_f) = writable_f { + poll.subscribe_write(writable_f.as_ref(), Userdata::from(123)); + } else { + unreachable!(); + } // Timeout bounds time in poll_oneoff let monotonic = &*clocks.monotonic()?.abs_clock; poll.subscribe_monotonic_clock(