Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasi: path_open should accept a dir with RIGHT_FD_WRITE #2244

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Jun 10, 2024

This commit fixes a path_open behavior that allows a directory to be path_opened with the rights::fd_write flag. Since Wazero considers it to be an error to open a directory with the write flag normally, it is safe to treat this as a bug. Although rights/capability system is being scrapped, we still use the fd_read and fd_write flag to determine if write permission needs to be set.

fixes #2243

@yagehu yagehu requested a review from mathetake as a code owner June 10, 2024 17:21
This commit fixes a `path_open` behavior that allows a directory to be
`path_open`ed with the `rights::fd_write` flag.  Since Wazero considers
it to be an error to open a directory with the write flag normally, it
is safe to treat this as a bug.  Although rights/capability system is
being scrapped, we still use the `fd_read` and `fd_write` flag to
determine if write permission needs to be set.

fixes tetratelabs#2243

Signed-off-by: Yage Hu <me@huyage.dev>
@evacchi
Copy link
Contributor

evacchi commented Jun 10, 2024

Interesting apparently this fails a test case in the zig stdlib!

@yagehu
Copy link
Contributor Author

yagehu commented Jun 10, 2024

Interesting apparently this fails a test case in the zig stdlib!

I checked out main and the Zig stdlib test is still failing. Seems unrelated?

@evacchi
Copy link
Contributor

evacchi commented Jun 10, 2024

First, thanks anyway for the contribution!

Now, unfortunately I checked out both this branch and main and main succeeds while this branch reports 1 failed test. You can run Zig's test suite directly by installing ./cmd/wazero and then:

wazero run -mount . ./internal/integration_test/stdlibs/testdata/zig/test-opt.wasm

make sure you first build the zig test cases, see

make build.zig zigroot=${{ env.ZIG_SOURCE }}

Regardless, the PR should include a test for this case too.

@yagehu
Copy link
Contributor Author

yagehu commented Jun 10, 2024

Ahh OK it uses the built Wazero. I just had to rebuild wazero. Now I can repro. Thanks!

@mathetake mathetake removed their request for review June 10, 2024 19:11
@yagehu yagehu changed the title Fix O_DIRECTORY allows opening dir with write [Draft] Fix O_DIRECTORY allows opening dir with write Jun 10, 2024
@mathetake mathetake marked this pull request as draft June 10, 2024 19:49
@mathetake mathetake changed the title [Draft] Fix O_DIRECTORY allows opening dir with write Fix O_DIRECTORY allows opening dir with write Jun 10, 2024
yagehu added 2 commits June 10, 2024 14:21
Signed-off-by: Yage Hu <me@huyage.dev>
Signed-off-by: Yage Hu <me@huyage.dev>
@yagehu yagehu marked this pull request as ready for review June 10, 2024 21:37
@mathetake mathetake requested a review from evacchi June 11, 2024 00:04
Copy link
Contributor

@evacchi evacchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor change, overall looks fine!

@evacchi evacchi changed the title Fix O_DIRECTORY allows opening dir with write wasi: path_open should accept a dir with RIGHT_FD_WRITE Jun 11, 2024
@evacchi evacchi merged commit 8b3af37 into tetratelabs:main Jun 11, 2024
64 checks passed
@yagehu yagehu deleted the open-dir-write branch June 17, 2024 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should not be able to open a directory with write
2 participants