Stream: git-wasmtime

Topic: wasmtime / PR #7876 Preview 2 filesystem: track open_mode...

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 01:11):

pchickey opened PR #7876 from bytecodealliance:pch/preview2_fs_dont_mask_perms to bytecodealliance:main:

First update the fd_filestat_set test to assert that the filestat_set operations on a file descriptor behaves the same whether opened readonly or readwrite. This test now reproduces the behavior reported in https://github.com/bytecodealliance/wasmtime/issues/7829. wasi-common passes this changed test, but wasmtime-wasi::preview2 does not, showing that this is a regression between the two implementations.

Then, in preview2 implementation, track open_mode for a File and Dir, and use that instead of reporting permissions in DescriptorFlags.

FilePerms/DirPerms is a way of having wasmtime-wasi, instead of the operating system, mediate permissions on a preview2 Descriptor. This was conflated with the open mode, which should determine whether DescriptorFlags contains READ or WRITE or MUTATE_DIRECTORY.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 01:11):

pchickey requested alexcrichton for a review on PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 01:11):

pchickey requested wasmtime-core-reviewers for a review on PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 01:11):

pchickey requested sunfishcode for a review on PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 23:58):

sunfishcode submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 06 2024 at 23:58):

sunfishcode created PR review comment:

This doesn't seem right. If we open the file with just RIGHTS_FD_READ, we shuldn't be able to do fd_filestat_set_size on it. That would be analogous in POSIX to opening a file with FD_READ and calling ftruncate, which is defined to fail.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:42):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:42):

pchickey created PR review comment:

That was what I thought too but I didnt chase it down in the ftruncate case. From the python test suite I found that futime is expected to succeed even when a file is read-only. So, I'll have to change this test, as well as set-len to check the open mode and reject size changes when not open for writing (fail with EINVAL, which I prefer here to EBADF) but permit set-times.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 00:50):

sunfishcode edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2024 at 17:58):

pchickey edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:20):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:23):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:23):

pchickey created PR review comment:

This problem ended up being down to how the test was written. wasi::OFLAGS_CREAT implies that the WASI impl open the file with the O_RRONLY or O_RDWR in posix, so we were never actually opening a file just for reading in this test. I changed the test to create the file, then open it under the requested mode, and it behaves as expected.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:24):

pchickey commented on PR #7876:

I believe the test now shows that the implementations all behave as expected.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:25):

pchickey edited a comment on PR #7876:

I believe the test now shows that the implementations all behave as desired.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 00:25):

pchickey requested sunfishcode for a review on PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 11:49):

alexcrichton submitted PR review:

Seems reasonable to me but I'm finding it difficult to follow what each of the sets of permissions are so I'm not 100% sure. Could you expand a bit on how this fixes https://github.com/bytecodealliance/wasmtime/issues/7829? I'm having a tough time connecting permissions and how that'd fail something fd-based vs path-based.

Also https://github.com/bytecodealliance/wasmtime/issues/7829 mentions errno 63 and ENOSR but I suspect that was something Python-specific or something like that?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 11:49):

alexcrichton submitted PR review:

Seems reasonable to me but I'm finding it difficult to follow what each of the sets of permissions are so I'm not 100% sure. Could you expand a bit on how this fixes https://github.com/bytecodealliance/wasmtime/issues/7829? I'm having a tough time connecting permissions and how that'd fail something fd-based vs path-based.

Also https://github.com/bytecodealliance/wasmtime/issues/7829 mentions errno 63 and ENOSR but I suspect that was something Python-specific or something like that?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 11:49):

alexcrichton created PR review comment:

While you're here mind adding documentation to this method? I forget how perms relates to file_perms myself

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2024 at 11:49):

alexcrichton created PR review comment:

Mind adding some brief comments here about what these fields are? Three sets of permissions seems like a good threshold for needing disambiguation

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2024 at 00:32):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2024 at 00:42):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2024 at 00:42):

pchickey created PR review comment:

Yep improved and documented this :)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2024 at 01:19):

brettcannon commented on PR #7876:

I just finished running CPython's test suite and this fixed the issue I found and didn't cause any new ones!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 11 2024 at 18:41):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 12 2024 at 19:04):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 12 2024 at 19:35):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 12 2024 at 20:01):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 12 2024 at 20:44):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 12 2024 at 21:54):

pchickey updated PR #7876.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 13 2024 at 00:52):

pchickey merged PR #7876.

Last updated: Nov 22 2024 at 17:03 UTC