Implement path_link for Windows. #1199

marmistrz · 2020-03-01T15:58:02Z

This is probably the last missing syscall for Windows!

This PR implements path_link for Windows and adds a non-strict version of the path_link integration test.

I'm unsure about the error handling in path_link. MSDN doesn't say much about possible error codes for either CreateHardLinkA or CreateSymbolicLinkA. I mostly copied over the error conversion from path_symlink, but I'm not sure if it's correct. In particular, it's unclear to me what the purpose of strip_trailing_slashes_and_concatenate is.

path_symlink will now also detect an attempt to create a dangling symlink and return ENOTSUP. (is this the correct return code)?

Currently the non-strictness of the test consists of:

we use a separate subdirectories subdir, subdir2, subdir3 for each test stage. This is due to the fact Windows will not remove the directory and won't allow to create a directory with the same name until the previous one has been deleted. I don't see any way of circumventing it, because the application may still try to access the directory through the unclosed file descriptor.
path_link will return EACCES instead of EPERM when trying to create a link to a subdirectory. This violates the POSIX spec. We could manually check if the source path is a directory in case of ERROR_ACCESS_DENIED but this would cost us an extra syscall.
Tests for dangling symlinks or symlink loops have been disabled. Alternatively, we could check if the attempt to create a dangling symlink returns ENOTSUP, but this doesn't make much sense while 1&2 are an issue.

Let me know what you think.

Btw. @kubkon, according to this stackoverflow post Mac OS X 10.5+ permits hard links to directories, which our tests expect to fail.

Notes about links and symlinks under Windows:

creating a symlink requires administrative privileges (SeCreateSymbolicLinkPrivilege). On Windows 10 this requirement may be removed, but this requires enabling developer mode
Windows distinguishes between file and directory symlinks
It's possible to create a dangling symlink, but the type (file/directory) has to be specified upon creation. The behavior in case of type mismatch is inconsistent. Precisely, suppose that a dangling file symlink is created foo -> bar and later, a directory bar is created. Then:
- under msys64 bash, cd foo succeeds and the directory view is the same when access either directly or through the symlink
- under cmd (both windowed and as a child process from msys64 bash). cd foo fails with The directory name is invalid

marmistrz · 2020-03-02T07:56:44Z

The test failure seems unrelated to this change. (some cranelift/codegen issues)

peterhuene · 2020-03-02T19:08:59Z

It's hard to see because we output so much junk with --nocapture, but the new test does appear to be failing in CI:

thread 'main' panicked at 'removing a subdirectory: Error { code: 55, message: "Directory not empty." }', src/libcore/result.rs:1188:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
test wasi_tests::path_link_nonstrict ... FAILED

marmistrz · 2020-03-04T17:47:38Z

You're right. Unfortunately, I can't reproduce the error locally, the tests always succeed. I'm running the tests on Windows 10, they succeed both when run through msys64 ssh and windowed cmd (as administrator). I'm using cargo test --features test_programs --package test-programs but the full CI command succeeds too.

Could you try and see if you can reproduce it?

peterhuene · 2020-03-04T19:24:27Z

I'll see if I can reproduce it.

crates/test-programs/wasi-tests/src/bin/path_link_nonstrict.rs

peterhuene · 2020-03-04T19:57:05Z

I'm unable to reproduce the failure locally as well. I tried rerunning the GitHub actions jobs, but that apparently is broken (seems to checkout the wasmtime repo and then try to checkout your HEAD, but it obviously isn't in the history 🤷‍♂).

The expect message of removing a subdirectory only appears in path_link which should still be ignored, so I'm not sure how we saw that in the log.

To restart CI, let's rebase this branch off of master and push it up.

marmistrz · 2020-03-04T20:33:02Z

I rebased upon latest master. The message is coming from path_link_nonstrict.

kubkon · 2020-03-04T21:09:04Z

Oh, strip_trailing_slashes_and_concatenate is there to correctly account for the case where old_path is a file, and new_path contains a trailing slash. On POSIX (and in WASI), as far as I remember, if new_path exists on the host but without the trailing slash, we expect in this case an EEXIST error code. On Windows, this is not respected in the same way, hence why the additional check.

kubkon · 2020-03-04T21:10:58Z

As far as mapping of error codes is concerned, yeah, MSDN is not very instructive. When I was figuring out path_symlink and path_rename, it was mostly the case of trial and error unfortunately.

kubkon · 2020-03-04T21:12:24Z

Also, thanks for the link! I'll need to investigate MacOS a bit further I think!

crates/test-programs/wasi-tests/src/bin/path_link_nonstrict.rs

kubkon · 2020-03-04T21:16:48Z

crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs

+                        } else if !old_path.exists() {
+                            // attempt to create a dangling symlink
+                            warn!("Dangling symlink or symlink loops unsupported on Windows");
+                            Err(Error::ENOTSUP)


This looks like a good choice of errno value here, but I'd like @sunfishcode to double check that though.

marmistrz · 2020-03-06T10:45:41Z

I tried to integrate those two tests into one and it appears that creating dangling symlinks doesn't fail when it should. I'll need to see why.

The previous version of the branch is available here: https://github.com/marmistrz/wasmtime/tree/path_link_old

github-actions · 2020-03-25T13:26:09Z

Subscribe to Label Action

This issue or pull request has been labeled: "wasi", "wasi:impl", "wasi:tests"

Users Subscribed to "wasi"

@kubkon

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

marmistrz · 2020-03-25T15:29:41Z

Good news! I have managed to isolate the cause of the mysterious CI failures.

It appears that after calling fd_fdstat_get on a file the file will not properly be removed (internally, path_unlink_file uses fs::remove_file). This would manifest itself in two ways: (1) path_remove_directory would fail due to the directory (2) if we skipped that part of the test, an attempt to create another file with the same name would fail due, due to permission having been denied

Support for fd_fdstat_get on Windows was introduced in #557. @peterhuene, since the call was implemented by you, could you take this over from me at this point? I also suggest we disable the checks using fd_fdstat_get until it's figured out.

The change is mostly ready. I'm still not sure if the ERROR_ACCESS_DENIED conversion is still needed, but this is basically discussed in #1359. Should it be the only blocker, I'm fine with removing it for now and possibly adding it back if we decide it's needed in #1359.

peterhuene · 2020-03-25T17:35:53Z

I don't particularly see how the problem lies with fd_fdstat_get. I double checked and it's not inadvertently leaking any handles.

I do see how the test was missing calls to fd_close before the attempts to unlink the files and directories, which is required on Windows.

If we uncomment the fd_fdstat_get calls with the rest of the changes, the test will still fail? I can't explain that, but I'll look into it.

marmistrz · 2020-03-25T18:40:43Z

If we uncomment the following lines: https://github.com/bytecodealliance/wasmtime/pull/1199/files#diff-6242b29603dfbc39ebbd9f3e58dde615R98-R102
the tests will fail in the CI (but not locally). I have absolutely no idea why this could happen, I've checked the source too.

kubkon · 2020-04-01T07:00:03Z

@peterhuene @marmistrz did you manage to work out what the problem is? If not, what do you think the next steps should be both wrt the problem and this PR?

peterhuene · 2020-04-01T22:54:41Z

I haven't yet determined the problem and I have no educated guesses since the commented out lines should have absolutely no impact on handle reference counts that might keep things from being deleted properly.

@marmistrz when you get a chance, can you uncomment out the problematic lines and push it up for CI to fail again? I'd like to investigate the log just a little more.

marmistrz · 2020-04-01T23:56:36Z

I thought about merging this PR as-is and letting @peterhuene figure out what's wrong with fdstat in a subsequent PR. But for now, I'll add a comment uncommenting the problematic lines.

peterhuene · 2020-04-02T00:27:19Z

So it looks like it is passing Windows CI with the uncommented lines. Perhaps the other fix to properly close the handles before unlinking was what actually resolved the issue?

I think we can leave it uncommented and remove the preceding comments about the lines being a problem. I'll go ahead with a review.

peterhuene

Great work! Just some minor feedback before we merge.

crates/test-programs/wasi-tests/src/bin/path_link.rs

peterhuene · 2020-04-02T00:30:51Z

crates/wasi-common/src/sys/windows/path.rs

+            Some(code) => {
+                debug!("path_link at fs::hard_link error code={:?}", code);
+                match code as u32 {
+                    // TODO is this needed at all????


Is this TODO comment actionable or can it be removed?

I'm removing this mapping, because there is no clear reason why it's needed, just as in #1359.

marmistrz · 2020-04-02T17:11:28Z

So it looks like it is passing Windows CI with the uncommented lines. Perhaps the other fix to properly close the handles before unlinking was what actually resolved the issue?

Absolutely no idea. Perhaps some cleanup I did after removing the fdstats interfered. Anyway, I'm happy that this mysterious failure is gone. I will push out the review changes in several steps.

I'm still unsure about point 2 in non-strictness:

2\. `path_link` will return `EACCES` instead of `EPERM` when trying to create a link to a subdirectory. This violates the POSIX spec. We could manually check if the source path is a directory in case of `ERROR_ACCESS_DENIED` but this would cost us an extra syscall.

Should I correct this in the error mappings or do we prefer to avoid the extra syscall?

kubkon · 2020-04-02T17:18:19Z

So it looks like it is passing Windows CI with the uncommented lines. Perhaps the other fix to properly close the handles before unlinking was what actually resolved the issue?

Absolutely no idea. Perhaps some cleanup I did after removing the fdstats interfered. Anyway, I'm happy that this mysterious failure is gone. I will push out the review changes in several steps.

I'm still unsure about point 2 in non-strictness:
2\. `path_link` will return `EACCES` instead of `EPERM` when trying to create a link to a subdirectory. This violates the POSIX spec. We could manually check if the source path is a directory in case of `ERROR_ACCESS_DENIED` but this would cost us an extra syscall.
Should I correct this in the error mappings or do we prefer to avoid the extra syscall?

I think we should be fine either way. We should be fine with an additional syscall since we already do that in path::symlink anyway.

marmistrz · 2020-04-02T17:22:46Z

So it looks like it is passing Windows CI with the uncommented lines. Perhaps the other fix to properly close the handles before unlinking was what actually resolved the issue?

Absolutely no idea. Perhaps some cleanup I did after removing the fdstats interfered. Anyway, I'm happy that this mysterious failure is gone. I will push out the review changes in several steps.
I'm still unsure about point 2 in non-strictness:
2\. `path_link` will return `EACCES` instead of `EPERM` when trying to create a link to a subdirectory. This violates the POSIX spec. We could manually check if the source path is a directory in case of `ERROR_ACCESS_DENIED` but this would cost us an extra syscall.
Should I correct this in the error mappings or do we prefer to avoid the extra syscall?
I think we should be fine either way. We should be fine with an additional syscall since we already do that in path::symlink anyway.

@peterhuene and what do you think?

peterhuene · 2020-04-02T18:04:32Z

I'm 👍 on what you now have.

marmistrz · 2020-04-02T20:12:26Z

Awesome! Then, unless anything comes up, I'm merging the PR tomorrow evening CET.

marmistrz added wasi:impl Issues pertaining to WASI implementation in Wasmtime wasi:tests Issues pertaining to WASI tests in Wasmtime labels Mar 1, 2020

marmistrz requested review from peterhuene, kubkon and sunfishcode March 1, 2020 15:58

marmistrz force-pushed the path_link branch from 9220fd5 to 425d5d7 Compare March 2, 2020 07:21

marmistrz force-pushed the path_link branch 3 times, most recently from 5a7b9c6 to 94a29ca Compare March 4, 2020 17:12

peterhuene reviewed Mar 4, 2020

View reviewed changes

crates/test-programs/wasi-tests/src/bin/path_link_nonstrict.rs Outdated Show resolved Hide resolved

marmistrz force-pushed the path_link branch from 94a29ca to 14edc0e Compare March 4, 2020 20:32

marmistrz force-pushed the path_link branch from 14edc0e to 51c6a80 Compare March 4, 2020 20:34

kubkon requested changes Mar 4, 2020

View reviewed changes

marmistrz force-pushed the path_link branch 5 times, most recently from 22a3a24 to 43195ad Compare March 6, 2020 10:28

marmistrz force-pushed the path_link branch from 43195ad to a022784 Compare March 9, 2020 15:48

github-actions bot added the wasi Issues pertaining to WASI label Mar 11, 2020

marmistrz force-pushed the path_link branch 3 times, most recently from d05a9f0 to adf86e4 Compare March 25, 2020 15:23

Implement path_link for Windows.

4277821

marmistrz force-pushed the path_link branch from adf86e4 to 4277821 Compare March 25, 2020 15:28

marmistrz requested review from peterhuene and kubkon March 25, 2020 15:29

Uncomment the problematic fdstats

61d163d

peterhuene reviewed Apr 2, 2020

View reviewed changes

marmistrz added 2 commits April 2, 2020 19:01

Remove RAII wrappers

e5d9b89

Remove #[allow(warnings)]

05a376e

peterhuene approved these changes Apr 2, 2020

View reviewed changes

marmistrz force-pushed the path_link branch from 6f90807 to 653dba4 Compare April 2, 2020 18:01

Map EACCES -> EPERM for hard links to directories

788dc8c

marmistrz force-pushed the path_link branch from 653dba4 to 788dc8c Compare April 2, 2020 18:02

kubkon approved these changes Apr 2, 2020

View reviewed changes

marmistrz merged commit f6e3ab0 into bytecodealliance:master Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement path_link for Windows. #1199

Implement path_link for Windows. #1199

marmistrz commented Mar 1, 2020 •

edited

marmistrz commented Mar 2, 2020 •

edited

peterhuene commented Mar 2, 2020

marmistrz commented Mar 4, 2020

peterhuene commented Mar 4, 2020

peterhuene commented Mar 4, 2020

marmistrz commented Mar 4, 2020 •

edited

kubkon commented Mar 4, 2020

kubkon commented Mar 4, 2020

kubkon commented Mar 4, 2020

kubkon Mar 4, 2020

marmistrz commented Mar 6, 2020

github-actions bot commented Mar 25, 2020

marmistrz commented Mar 25, 2020

peterhuene commented Mar 25, 2020

marmistrz commented Mar 25, 2020 •

edited

kubkon commented Apr 1, 2020

peterhuene commented Apr 1, 2020

marmistrz commented Apr 1, 2020

peterhuene commented Apr 2, 2020

peterhuene left a comment

peterhuene Apr 2, 2020

marmistrz Apr 2, 2020

marmistrz commented Apr 2, 2020

kubkon commented Apr 2, 2020

marmistrz commented Apr 2, 2020

peterhuene commented Apr 2, 2020

marmistrz commented Apr 2, 2020

Implement path_link for Windows. #1199

Implement path_link for Windows. #1199

Conversation

marmistrz commented Mar 1, 2020 • edited

marmistrz commented Mar 2, 2020 • edited

peterhuene commented Mar 2, 2020

marmistrz commented Mar 4, 2020

peterhuene commented Mar 4, 2020

peterhuene commented Mar 4, 2020

marmistrz commented Mar 4, 2020 • edited

kubkon commented Mar 4, 2020

kubkon commented Mar 4, 2020

kubkon commented Mar 4, 2020

kubkon Mar 4, 2020

Choose a reason for hiding this comment

marmistrz commented Mar 6, 2020

github-actions bot commented Mar 25, 2020

Subscribe to Label Action

marmistrz commented Mar 25, 2020

peterhuene commented Mar 25, 2020

marmistrz commented Mar 25, 2020 • edited

kubkon commented Apr 1, 2020

peterhuene commented Apr 1, 2020

marmistrz commented Apr 1, 2020

peterhuene commented Apr 2, 2020

peterhuene left a comment

Choose a reason for hiding this comment

peterhuene Apr 2, 2020

Choose a reason for hiding this comment

marmistrz Apr 2, 2020

Choose a reason for hiding this comment

marmistrz commented Apr 2, 2020

kubkon commented Apr 2, 2020

marmistrz commented Apr 2, 2020

peterhuene commented Apr 2, 2020

marmistrz commented Apr 2, 2020

marmistrz commented Mar 1, 2020 •

edited

marmistrz commented Mar 2, 2020 •

edited

marmistrz commented Mar 4, 2020 •

edited

marmistrz commented Mar 25, 2020 •

edited