Skip to content

fix: check filetype in path_open #4947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rvolosatovs wants to merge 2 commits into from
Closed

fix: check filetype in path_open #4947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe33e29
test(fd_readdir): add a nested directory
rvolosatovs Sep 26, 2022
5ffb7f8
fix: check filetype in `path_open`
rvolosatovs Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 44 additions & 9 deletions crates/test-programs/wasi-tests/src/bin/fd_readdir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,10 @@ unsafe fn exec_fd_readdir(fd: wasi::Fd, cookie: wasi::Dircookie) -> (Vec<DirEntr
(dirs, eof)
}

unsafe fn test_fd_readdir(dir_fd: wasi::Fd) {
let stat = wasi::fd_filestat_get(dir_fd).expect("failed filestat");
unsafe fn assert_empty_dir(fd: wasi::Fd) {
let stat = wasi::fd_filestat_get(fd).expect("failed filestat");

// Check the behavior in an empty directory
let (mut dirs, eof) = exec_fd_readdir(dir_fd, 0);
let (mut dirs, eof) = exec_fd_readdir(fd, 0);
assert!(eof, "expected to read the entire directory");
dirs.sort_by_key(|d| d.name.clone());
assert_eq!(dirs.len(), 2, "expected two entries in an empty directory");
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
88 changes: 55 additions & 33 deletions crates/wasi-common/src/snapshots/preview_1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,45 +890,67 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
}
let dir_entry = table.get_dir(dirfd)?;

let oflags = OFlags::from(&oflags);

// OFlags that are not supported for directories
let non_dir_oflags = oflags.contains(OFlags::CREATE)
|| oflags.contains(OFlags::EXCLUSIVE)
|| oflags.contains(OFlags::TRUNCATE);

if oflags.contains(OFlags::DIRECTORY) && non_dir_oflags {
return Err(Error::invalid_argument().context("incompatible oflag combination"));
}

let mut required_caps = DirCaps::OPEN;
if oflags.contains(OFlags::CREATE) {
required_caps = required_caps | DirCaps::CREATE_FILE;
}
let dir = dir_entry.get_cap(required_caps)?;

let symlink_follow = dirflags.contains(types::Lookupflags::SYMLINK_FOLLOW);

let oflags = OFlags::from(&oflags);
let fdflags = FdFlags::from(fdflags);
let path = path.as_str()?;
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 path = path.deref();
match dir.get_path_filestat(path, symlink_follow).await {
Copy link
Copy Markdown
Member

@sunfishcode sunfishcode Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about this get_path_filestat, because it does a second path lookup. There's a window for a race condition here, where a directory could be replaced by something which is not a directory. I don't yet have a clear view of the consequences of the open succeeding on an unexpected type, so this is something I want to investigate more.

I wonder if it would be possible to instead do something like:

  • Attempt to open with open_dir; if that works, create a DirEntry entry.
  • If that fails with Notdir, open it with regular open.
    Would that work?

I know that this is redundant on POSIX hosts, and possibly we could optimize things even more there, but this code is currently written to support Windows also.

Copy link
Copy Markdown
Member

@sunfishcode sunfishcode Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now have an implementation of this here: #4967

It uses the same testcase as this PR, so it fixes the same bug, but does so in a way that doesn't require doing a stat followed by a separate open. There's more room for improvement beyond that, but hopefully that's a good step.

Ok(Filestat {
filetype: FileType::Directory,
..
}) if non_dir_oflags => {
Err(Error::invalid_argument().context("oflags not applicable to a directory"))
}
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(Box::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;

Ok(Filestat {
filetype: FileType::Directory,
..
}) => {
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 child_dir = dir.open_dir(symlink_follow, path).await?;
drop(dir);
let fd = table.push(Box::new(DirEntry::new(
dir_caps, file_caps, None, child_dir,
)))?;
Ok(types::Fd::from(fd))
}

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(Box::new(FileEntry::new(file_caps, file)))?;
Ok(types::Fd::from(fd))
Ok(_) if oflags.contains(OFlags::DIRECTORY) => {
Err(Error::not_dir().context("directory requested"))
}

Copy link
Copy Markdown
Member Author

@rvolosatovs rvolosatovs Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, I would want to fail here if error is not not found, but that would require either cloning the error (to be able to propagate it if it does not match) and then converting it into a types::Errno or downcasting the error, which would require matching on all possible error types, which seems like the wrong thing to do as well.
Is there a reasonable way to do that? I'm looking for Error::not_found(&self) -> bool
Discard the error instead and preserve the original behavior.

Copy link
Copy Markdown
Contributor

@pchickey pchickey Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable to downcast_ref and match for an ErrorKind::Noent, and if that fails downcast to a std::io::Error with the appropriate check there as well. This is a big janky, and I think if I had to redesign wasi-common I probably wouldn't use a fully dynamic error type and instead make enum Error { Std(std::io::Error), Wasi(ErrorKind) } or something like that, but its been a while since I had this system in my head so I'm not sure why I didn't do that in the first place.

_ => {
let file_caps = dir_entry.child_file_caps(FileCaps::from(&fs_rights_base));
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 fdflags = FdFlags::from(fdflags);
let file = dir
.open_file(symlink_follow, path, oflags, read, write, fdflags)
.await?;
drop(dir);
let fd = table.push(Box::new(FileEntry::new(file_caps, file)))?;
Ok(types::Fd::from(fd))
}
}
}

Expand Down