fix: check filetype in path_open#4947
fix: check filetype in path_open#4947rvolosatovs wants to merge 2 commits intobytecodealliance:mainfrom
path_open#4947Conversation
Subscribe to Label Actioncc @kubkon DetailsThis issue or pull request has been labeled: "wasi"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
rvolosatovs
force-pushed
the
fix/path_open
branch
3 times, most recently
from
0bc9f4a to
ad11d83
Compare
|
@pchickey @sunfishcode We are blocked on this and would appreciate a prompt review. Thanks! |
rvolosatovs
force-pushed
the
fix/path_open
branch
from
ad11d83 to
4a77b25
Compare
|
I've rebased the PR on latest This is the failure on latest |
rvolosatovs
force-pushed
the
fix/path_open
branch
from
4a77b25 to
4d7c470
Compare
| Ok(_) if oflags.contains(OFlags::DIRECTORY) => { | ||
| Err(Error::not_dir().context("directory requested")) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Using the `directory` [open flag] to determine whether the entity being opened is a directory or a file is undefined behavior, since that means that the only way to acquire a usable directory is to specify the flag. Instead of relying on the [open flag] value to determine the filetype, issue `get_path_filestat` first to determine it and then perform appropriate operation depending on the value. [open flag]: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-oflags-record Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
rvolosatovs
force-pushed
the
fix/path_open
branch
from
4d7c470 to
5ffb7f8
Compare
pchickey
left a comment
pchickey
left a comment
There was a problem hiding this comment.
This is a pretty subtle one that clearly I never thought of. Looks good, aside from the question about the not found error. Lets get in some solution to that and then we can land this.
| Ok(_) if oflags.contains(OFlags::DIRECTORY) => { | ||
| Err(Error::not_dir().context("directory requested")) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
|
@pchickey I disagree strongly that downcasting should ever be used in |
| { | ||
| return Err(Error::invalid_argument().context("directory oflags")); | ||
| let path = path.deref(); | ||
| match dir.get_path_filestat(path, symlink_follow).await { |
There was a problem hiding this comment.
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
DirEntryentry. - If that fails with
Notdir, open it with regularopen.
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.
There was a problem hiding this comment.
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.
|
@npmccallum OK, send a PR to fix it then |
|
@pchickey I discussed this and other issues with @sunfishcode and he recommended that I not try to patch the existing implementation and wait until preview2. Should I file an issue requesting the removal of downcasting under preview2? |
`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 bytecodealliance#4947. [`maybe_dir`]: https://docs.rs/cap-fs-ext/latest/cap_fs_ext/struct.OpenOptions.html#method.maybe_dir`
`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 bytecodealliance#4947. [`maybe_dir`]: https://docs.rs/cap-fs-ext/latest/cap_fs_ext/struct.OpenOptions.html#method.maybe_dir`
`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)`.
`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>
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.
…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.
…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.
4 participants
Using the
directoryopen flag to determine whether the entity being opened is a directory or a file is undefined behavior, since that means that the only way to acquire a usable directory is to specify the flag.Instead of relying on the open flag value to determine the filetype, issue
get_path_filestatfirst to determine it and then perform appropriate operation depending on the value.Note that this also slightly changes the behavior, wherecreateopen flag is not dropped anymore, but rather passed through to theopen_dircall.@pchickey seems to be the last person making any significant changes in this method, so assigning him for review