Stream: git-wasmtime

Topic: wasmtime / issue #6163 Allow WASI to open directories wit...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 06 2023 at 02:03):

github-actions[bot] commented on issue #6163:

Subscribe to Label Action

cc @kubkon

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2023 at 01:10):

sunfishcode commented on issue #6163:

Looks like this is headed in the right direction!

I don't understand how the capabilities are supposed to work yet. Opening a directory got file_caps from fs_rights_inheriting, while opening a file got them from fs_rights_base. I haven't implemented that distinction in this PR yet.

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 00:44):

jameysharp commented on issue #6163:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 19:45):

jameysharp commented on issue #6163:

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

Do we need to support deleting files which still have open handles on Windows?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 21:51):

pchickey commented on issue #6163:

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 TESTCONFIG.support_dangling_filesystem() boolean is false when running on windows, so please modify the test to check for whatever behavior windows does when that is false.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 18 2023 at 22:04):

jameysharp commented on issue #6163:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 18:11):

pchickey commented on issue #6163:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 18:48):

jameysharp commented on issue #6163:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 19:16):

pchickey commented on issue #6163:

I was confused as well.

So I'm changing these test programs to make sure they've closed the handle before trying to delete the file.
This is a totally sufficient solution. I don't think we need to test any of these windows or not-windows behaviors further.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 19:16):

pchickey edited a comment on issue #6163:

I was confused as well.

So I'm changing these test programs to make sure they've closed the handle before trying to delete the file.

This is a totally sufficient solution. I don't think we need to test any of these windows or not-windows behaviors further.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 19:29):

jameysharp commented on issue #6163:

Hooray! Any other concerns about merging this, then? As far as I can tell, I think it's ready to go.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 25 2023 at 04:45):

achille-roussel commented on issue #6163:

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 https://github.com/golang/go/issues/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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 25 2023 at 14:46):

alexcrichton commented on issue #6163:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 25 2023 at 15:45):

jameysharp commented on issue #6163:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 25 2023 at 15:54):

alexcrichton commented on issue #6163:

Ok that sounds good to me, yeah. If you're up for it a PR to release-8.0.0 would work well and then this'll get folded into Thursday's release.

Last updated: Oct 23 2024 at 20:03 UTC