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

Fix tests on WAMR #85

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Fix tests on WAMR #85

merged 1 commit into from
Sep 18, 2023

Conversation

zoraaver
Copy link
Contributor

@zoraaver zoraaver commented Sep 11, 2023

Most of the changes are adding rights to fix permissions issues and changing expected error codes.

I've also changed the stdio.rs test so that the destination (to) file descriptors are valid. The test was previously failing on WAMR since the destination fd's were arbitrary numbers which seems to be explicitly not permitted by the docs. A test case has been added in renumber.rs to enforce that renumbering to invalid destination file descriptors is an error.

If we want to align on return error codes between the runtimes for certain filesystem functions, I don't think it would be too much work to change the WAMR implementation to make it fall in line with wasmtime but for the moment have raised this PR to get some clarification and feedback.

@zoraaver zoraaver force-pushed the fix-tests-on-wamr branch 2 times, most recently from 3d08a35 to f443d26 Compare September 12, 2023 14:46
@@ -39,6 +39,12 @@ pub unsafe fn create_tmp_dir(dir_fd: wasi::Fd, name: &str) -> wasi::Fd {
| wasi::RIGHTS_FD_READDIR
| wasi::RIGHTS_FD_FILESTAT_GET
| wasi::RIGHTS_FD_SEEK
| wasi::RIGHTS_PATH_LINK_SOURCE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that those tests were passing on wasmtime, I wonder if wasmtime correctly implemented file right system. Looks like files created in parent directory inherit base rights, instead of inheriting inheriting rights. @abrown is that something you could confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to which version of Wasmtime is being used to run the tests? I do see recent-ish PRs that may have changed this behavior:

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is on 12.0.2

Copy link
Contributor

Choose a reason for hiding this comment

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

I would try the latest version of Wasmtime then and see if that resolves things?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tried the latest main branch, and tests in its current form (e.g. path_filestat) are still passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like files created in parent directory inherit base rights, instead of inheriting inheriting rights.

Just want to confirm that's the behaviour I observed.

@abrown if it's helpful, here's a list of tests which should fail (without the changes from this PR) and the required missing rights:

  • path_link.rs
wasi::RIGHTS_PATH_LINK_SOURCE
wasi::RIGHTS_PATH_LINK_TARGET
wasi::RIGHTS_PATH_OPEN
wasi::RIGHTS_PATH_UNLINK_FILE
  • path_filestat and symlink_filestat.rs
wasi::RIGHTS_PATH_FILESTAT_GET

Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@loganek loganek merged commit 79479e9 into WebAssembly:main Sep 18, 2023
11 checks passed
@zoraaver zoraaver deleted the fix-tests-on-wamr branch September 18, 2023 14:04
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.

3 participants