Allow WASI to open directories without O_DIRECTORY #6163
Conversation
Subscribe to Label Actioncc @kubkon
This 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 |
|
Looks like this is headed in the right direction!
"Inheriting" rights are rights that should apply to derived file descriptors, such as files opened from directories. For directories opened for directories, they can just assume the same base and inheriting rights as the directory they were owned from, because the "inheriting" rights will apply to files opened under them. |
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.
|
Okay cool, so this now passes all the tests on Linux, but fails some tests on Windows. I'm probably going to need help figuring out how to fix that. |
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.
|
I've identified the root cause of the test failures on Windows. I pushed a commit that changes the tests, which let them pass in CI. My question now: Is it okay to treat this as a bug in the tests, or do we need an implementation that supports the existing behavior of the tests? Details: On Windows, when opening a path which might be a directory using That means that if we implement WASI's So I'm changing these test programs to make sure they've closed the handle before trying to delete the file. Do we need to support deleting files which still have open handles on Windows? |
|
It is OK to not support deleting files which still have open handles on windows - we have a number of other special cases where the tests expect a different behavior for things like this on windows (among them: symlink behavior, and renaming a dir to an empty dir). While it isnt a properly descriptive name, the |
|
Just to make sure I understand: you want the tests to specifically verify that deleting files is prohibited if there are still open handles? Right now this PR just avoids trying to delete files when there are still open handles, so it passes on all platforms. But you want the tests to fail if our implementation on Windows ever starts allowing deleting open files? |
|
In keeping with the rest of the test suite, I don't think we need to test for positive behavior on windows (i.e. deleting files with open handles is prohibited), just conditionally skip the check for the deletion behavior if on windows. |
|
I'm still confused, I guess. All I had to do to make the tests work on all platforms was to ensure that the file handle was closed before attempting to unlink the underlying file. So I don't strictly need to conditionally disable anything, unless you want it to specifically check the behavior of trying to unlink an open file. I guess you're saying we don't need to check that behavior on Windows, so are you asking for an explicit test that non-Windows systems do allow unlinking open files? I can do that, I just didn't see any sign that this was something these tests were intended to cover. It looked to me like it was more of an accident that they failed in this case. |
|
I was confused as well.
This is a totally sufficient solution. I don't think we need to test any of these windows or not-windows behaviors further. |
|
Hooray! Any other concerns about merging this, then? As far as I can tell, I think it's ready to go. |
…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.
…r rights bytecodealliance/wasmtime#6265 note, im leaving out upstream changes to fd_readdir which are from bytecodealliance/wasmtime#6163, which requires porting the rest of that PR into this repo
* Revert "wasi-tests: add configuration to ignore rights readback (#81)" This reverts commit 087179a. * wasi-tests: pull in all changes from upstream which remove support for rights bytecodealliance/wasmtime#6265 note, im leaving out upstream changes to fd_readdir which are from bytecodealliance/wasmtime#6163, which requires porting the rest of that PR into this repo * remove rights readback variable from test runner * rustfmt * delete path_open_read_without_rights test which was deleted upstream
|
Thank you for wrapping up this fix! We have code in the upcoming Go release (1.21) which acted as a temporary workaround https://github.com/golang/go/blob/22d94dfdc8ab866ff6097b6b92a2860233873c95/src/syscall/fs_wasip1.go#L435-L456 An automated test runner using wasmtime is also being added to Go golang/go#59583 It seems wasmtime has been releasing major versions mostly (around once a month). Having this fix available in an earlier release would allow us to integrate it immediately in the test suite, and remove the workaround. Would it be possible to issue a patch release (v8.0.1) which includes this fix? |
|
We do have an upcoming security release this Thursday for 8.0.1, so this could ride out with that. @jameysharp do you feel confident enough in this being back-portable to 8.0.1? At least IIRC we don't have a firm policy about what and what not to backport, so in lieu of that I'm assuming that explicitly requested changes are fine for considering as candidates. |
|
I'm confident in this PR on POSIX-conforming systems. I'm not 100% confident that I didn't break something on Windows, but the fact that CI did catch Windows-specific bugs in my first attempts is reassuring. So I'd say I'm pretty confident this is fine to backport to 8.0.1. Also, this commit cherry-picks cleanly onto release-8.0.0, which is nice. Would you like me to open a PR for that, Alex? |
|
Ok that sounds good to me, yeah. If you're up for it a PR to |
…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.
…dealliance#6163) 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. 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.
…#6283) * Backport "Allow WASI to open directories without O_DIRECTORY" (#6163) 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. 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. * Update RELEASES.md
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.
* Revert "wasi-tests: add configuration to ignore rights readback (bytecodealliance#81)" This reverts commit d006b1b. * wasi-tests: pull in all changes from upstream which remove support for rights bytecodealliance#6265 note, im leaving out upstream changes to fd_readdir which are from bytecodealliance#6163, which requires porting the rest of that PR into this repo * remove rights readback variable from test runner * rustfmt * delete path_open_read_without_rights test which was deleted upstream
This is an initial proof-of-concept alternative to #4967, which was an alternative to #4947. Unlike the other two attempts, this one should be race-free. I'm not particularly happy with it yet though, just opening this PR for discussion.
Open issues:
file_capsfromfs_rights_inheriting, while opening a file got them fromfs_rights_base. I haven't implemented that distinction in this PR yet.To-do:
WasiDir::open_dirand related bits, which is now used only in tests.