Allow WASI to open directories without `O_DIRECTORY`. #4967

sunfishcode · 2022-09-27T01:34:50Z

O_DIRECTORY says that a directory is required, but O_DIRECTORY is not required for opening directories. To implement this on top of the current APIs, use open_dir to try to open a directory first, and then fall back to trying to open it as a file if that fails.

In the future, we may be able to simplify this code even more, using maybe_dir, which is needed to allow Windows to be able to open a directory, though it needs bytecodealliance/cap-std#277 for Windows.

The testcase here is the testcase from #4947.

github-actions · 2022-09-27T02:37:54Z

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

rvolosatovs

Implementation LGTM.

Since you took the test from #4947, is there a reason you didn't just cherry-pick fe33e29 in this PR?

rvolosatovs · 2022-09-27T21:07:36Z

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

+fn is_error_notdir(e: &Error) -> bool {
+    if e.is::<ErrorKind>() {
+        let e = e.downcast_ref::<ErrorKind>().unwrap();
+        if let ErrorKind::Notdir = *e {
+            true
+        } else {
+            false
+        }
+    } else if e.is::<std::io::Error>() {
+        let e = e.downcast_ref::<std::io::Error>().unwrap();
+        #[cfg(unix)]
+        {
+            e.raw_os_error() == Some(rustix::io::Errno::NOTDIR.raw_os_error())
+        }
+        #[cfg(windows)]
+        {
+            e.raw_os_error() == Some(windows_sys::Win32::Foundation::ERROR_DIRECTORY as _)
+        }
+    } else {
+        false
+    }
+}
+
+fn is_error_noent(e: &Error) -> bool {
+    if e.is::<ErrorKind>() {
+        let e = e.downcast_ref::<ErrorKind>().unwrap();
+        if let ErrorKind::Noent = *e {
+            true
+        } else {
+            false
+        }
+    } else if e.is::<std::io::Error>() {
+        let e = e.downcast_ref::<std::io::Error>().unwrap();
+        e.kind() == std::io::ErrorKind::NotFound
+    } else {
+        false
+    }
+}


It looks like the error handling in this crate can benefit from quite some refactoring as this does not look like a maintainable way to match on errors.

Note, that this gets much worse for external users of this crate providing e.g. multiple WasiFile or WasiDir implementations, since users must return wasi-common::Error constructed internally from an ErrorKind in custom implementations or a few other special cases, e.g. from

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

Lines 48 to 64 in 29c7de7

use types::Errno;

if e.is::<ErrorKind>() {

let e = e.downcast::<ErrorKind>().unwrap();

Ok(e.into())

} else if e.is::<std::io::Error>() {

let e = e.downcast::<std::io::Error>().unwrap();

e.try_into()

} else if e.is::<wiggle::GuestError>() {

let e = e.downcast::<wiggle::GuestError>().unwrap();

Ok(e.into())

} else if e.is::<std::num::TryFromIntError>() {

Ok(Errno::Overflow)

} else if e.is::<std::str::Utf8Error>() {

Ok(Errno::Ilseq)

} else {

Err(e)

}

, anything else results in a trap and terminates execution.
Every assertion on the error kind now requires a downcast and this runtime reflection step can fail if e.g. the wasi-common crate changes the set of specific error types returned or downcasted internally on update.

So perhaps these utilities should actually be public to expose them to users who may need to match on the error kind from outside this crate?

I agree that the current code is not great. It sounds like another way to approach this would be to get rid of the anyhow and downcasting altogether and just use an enum. I may be up for doing the work for this refactoring, but in this PR I'd like to focus on just fixing the underlying bug first, and we can figure out refactoring separately.

Yes, I agree that does not belong in this PR :)
Another option could be an associated type Error: AsRef<ErrorKind> or type Error: AsRef<Errno>, whichever is the most applicable.

sunfishcode · 2022-09-27T22:24:52Z

Implementation LGTM.

Since you took the test from #4947, is there a reason you didn't just cherry-pick fe33e29 in this PR?

Just that I didn't notice it was a separate commit. I've now rebased this PR to cherry-pick that patch.

rvolosatovs

A few more questions from a deeper look than I did yesterday night

rvolosatovs · 2022-09-28T10:54:23Z

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

+
+        // `O_DIRECTORY` is incompatible with `O_CREAT`, `O_EXCL`, and `O_TRUNC`.
+        if oflags.contains(OFlags::DIRECTORY)
+            && (oflags.contains(OFlags::CREATE)


(nit, I suppose?) I think this if statement is way too complex and hard to read, which is exactly why I introduced https://github.com/rvolosatovs/wasmtime/blob/5ffb7f8990d1562bf9a1b6b29e3faadc934d024f/crates/wasi-common/src/snapshots/preview_1.rs#L895-L898 in #4947

rvolosatovs · 2022-09-28T10:55:08Z

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

+        if oflags.contains(OFlags::DIRECTORY)
+            || fs_rights_base.contains(types::Rights::FD_READDIR)
+            || !(fs_rights_base.contains(types::Rights::FD_WRITE)
+                || oflags.contains(OFlags::CREATE)
+                || oflags.contains(OFlags::EXCLUSIVE)
+                || oflags.contains(OFlags::TRUNCATE))


this is very difficult to read and comprehend especially due to the nesting

rvolosatovs · 2022-09-28T10:58:22Z

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

+            || 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)


You mentioned a race condition in https://github.com/bytecodealliance/wasmtime/pull/4947/files#r980506965 as reasoning for this PR, where the underlying filetype stored under a path could change between the calls, but doesn't this suffer from exact same race condition, where:

initial open_dir fails on path A with ENOTDIR

in the meantime, A is replaced by a directory

open_file fails, because A is now a directory
?

That's a good point. I need to look at this more closely.

`ErrorKind` is an internal enum used in wasi-libc to represent WASI errors that aren't precisely represened by `std::io::ErrorKind` errors. Add a descriptive comment, and remove some codes that are no longer needed: - Remove `NotCapable`, which is no longer used. - Remove `WouldBlk`, `Exist`, `Noent`, and `Inval`, which have one-to-one correspondences with codes in `std::io::ErrorKind`. This will simplify the error handling in bytecodealliance#4947 and bytecodealliance#4967, as it means the code will no longer have to check for two different forms of these errors.

* Tidy up the WASI `ErrorKind` enum. `ErrorKind` is an internal enum used in wasi-libc to represent WASI errors that aren't precisely represened by `std::io::ErrorKind` errors. Add a descriptive comment, and remove some codes that are no longer needed: - Remove `NotCapable`, which is no longer used. - Remove `WouldBlk`, `Exist`, `Noent`, and `Inval`, which have one-to-one correspondences with codes in `std::io::ErrorKind`. This will simplify the error handling in #4947 and #4967, as it means the code will no longer have to check for two different forms of these errors. * Map `std::io::ErrorKind::InvalidInput` to `Ok(types::Errno::Inval)`.

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

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

`O_DIRECTORY` says that a directory is required, but `O_DIRECTORY` is not required for opening directories. To implement this on top of the current APIs, use `open_dir` to try to open a directory first, and then fall back to trying to open it as a file if that fails. In the future, we may be able to simplify this code even more, using [`maybe_dir`], which is needed to allow Windows to be able to open a directory, though it needs bytecodealliance/cap-std#277 for Windows. And, factor out flags that are incompatible with directories. This comes from: https://github.com/rvolosatovs/wasmtime/blob/5ffb7f8990d1562bf9a1b6b29e3faadc934d024f/crates/wasi-common/src/snapshots/preview_1.rs#L895-L898 The testcase here is the testcase from bytecodealliance#4947. [`maybe_dir`]: https://docs.rs/cap-fs-ext/latest/cap_fs_ext/struct.OpenOptions.html#method.maybe_dir` Co-authored-by: Roman Volosatovs <rvolosatovs@riseup.net> Co-authored-by: Harald Hoyer <harald@hoyer.xyz>

jameysharp · 2023-03-30T22:37:31Z

Since bytecodealliance/cap-std#277 merged, can this issue be fixed without race conditions now?

What I would expect, coming from a POSIX perspective, is to call Dir::open on the given path (apparently with the maybe_dir open flag for Windows), then check f.metadata().is_dir(). If it was a directory, it looks to me like Dir::from_std_file(f.into_std()) would fix up the Rust types to reflect reality.

Is that a reasonable approach?

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

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

…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.

Wasmtime used to error when opening a directory without passing the O_DIRECTORY flag, which required doing a stat to determine whether to inject the flag prior to opening any file. The workaround was subject to races since the stat and open calls were not atomic. Wasmtime fixed the issue in v8.0.1. For details see: - bytecodealliance/wasmtime#4967 - bytecodealliance/wasmtime#6163 - https://github.com/bytecodealliance/wasmtime/releases/tag/v8.0.1 Change-Id: I0f9fe6a696024b70fffe63b83e8f61e48acd0c4a Reviewed-on: https://go-review.googlesource.com/c/go/+/489955 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

…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.

sunfishcode mentioned this pull request Sep 27, 2022

fix: check filetype in path_open #4947

Closed

github-actions bot added the wasi Issues pertaining to WASI label Sep 27, 2022

rvolosatovs reviewed Sep 27, 2022

View reviewed changes

sunfishcode force-pushed the sunfishcode/open-dir-or-not branch from 3f043a3 to 704e53e Compare September 27, 2022 22:24

rvolosatovs reviewed Sep 28, 2022

View reviewed changes

sunfishcode mentioned this pull request Oct 4, 2022

Tidy up the WASI ErrorKind enum. #5015

Merged

haraldh reviewed Oct 6, 2022

View reviewed changes

crates/wasi-common/src/snapshots/preview_1.rs Outdated Show resolved Hide resolved

crates/wasi-common/src/snapshots/preview_1.rs Outdated Show resolved Hide resolved

rvolosatovs and others added 2 commits January 23, 2023 11:47

test(fd_readdir): add a nested directory

d35c4df

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

sunfishcode force-pushed the sunfishcode/open-dir-or-not branch from 3a20157 to 40676a6 Compare January 23, 2023 20:26

jameysharp mentioned this pull request Apr 6, 2023

Allow WASI to open directories without O_DIRECTORY #6163

Merged

jameysharp added a commit to jameysharp/wasmtime that referenced this pull request Apr 18, 2023

Add tests from bytecodealliance#4967/bytecodealliance#4947

f8b5820

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

pchickey closed this in efdfc36 Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow WASI to open directories without `O_DIRECTORY`. #4967

Allow WASI to open directories without `O_DIRECTORY`. #4967

sunfishcode commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

rvolosatovs left a comment •

edited

Loading

rvolosatovs Sep 27, 2022

sunfishcode Sep 27, 2022

rvolosatovs Sep 27, 2022

sunfishcode commented Sep 27, 2022

rvolosatovs left a comment

rvolosatovs Sep 28, 2022

rvolosatovs Sep 28, 2022

rvolosatovs Sep 28, 2022

sunfishcode Sep 30, 2022

jameysharp commented Mar 30, 2023

	use types::Errno;
	if e.is::<ErrorKind>() {
	let e = e.downcast::<ErrorKind>().unwrap();
	Ok(e.into())
	} else if e.is::<std::io::Error>() {
	let e = e.downcast::<std::io::Error>().unwrap();
	e.try_into()
	} else if e.is::<wiggle::GuestError>() {
	let e = e.downcast::<wiggle::GuestError>().unwrap();
	Ok(e.into())
	} else if e.is::<std::num::TryFromIntError>() {
	Ok(Errno::Overflow)
	} else if e.is::<std::str::Utf8Error>() {
	Ok(Errno::Ilseq)
	} else {
	Err(e)
	}

Allow WASI to open directories without O_DIRECTORY. #4967

Allow WASI to open directories without O_DIRECTORY. #4967

Conversation

sunfishcode commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

Subscribe to Label Action

rvolosatovs left a comment • edited Loading

Choose a reason for hiding this comment

rvolosatovs Sep 27, 2022

Choose a reason for hiding this comment

sunfishcode Sep 27, 2022

Choose a reason for hiding this comment

rvolosatovs Sep 27, 2022

Choose a reason for hiding this comment

sunfishcode commented Sep 27, 2022

rvolosatovs left a comment

Choose a reason for hiding this comment

rvolosatovs Sep 28, 2022

Choose a reason for hiding this comment

rvolosatovs Sep 28, 2022

Choose a reason for hiding this comment

rvolosatovs Sep 28, 2022

Choose a reason for hiding this comment

sunfishcode Sep 30, 2022

Choose a reason for hiding this comment

jameysharp commented Mar 30, 2023

Allow WASI to open directories without `O_DIRECTORY`. #4967

Allow WASI to open directories without `O_DIRECTORY`. #4967

rvolosatovs left a comment •

edited

Loading