path_get refactor and implementation of missing path_ hostcalls on Windows by kubkon · Pull Request #41 · CraneStation/wasi-common

kubkon · 2019-07-23T10:47:46Z

This PR moves path_get helper out of the sys submodule and puts it in platform-independent hostcalls_impl::fs_helpers module. ~~It requires readlinkat to be implemented which currently is missing on Windows, hence, this PR breaks WASI tutorial on Windows.~~

It also strives at adding missing path_ hostcalls so that we can enable more tests on Windows.

TODO:

EDIT: some basics of the above syscalls are now drafted out. I'm still uneasy about symlink handling on Windows for a couple of reasons:

creating symlinks requires that the user has either the necessary privileges or the OS is in developer mode -- not much we can do about this I guess!
Windows differentiates between file symlinks and dir symlinks whereas AFAIK *nix does not -- for that reason, unlinking a symlink done via path_unlink_file is rather messy as we need to check whether we are dealing with a file or dir symlink and invoke appropriately either std::fs::remove_file or std::fs::remove_dir
AFAICT dangling symlinks are impossible to create on Windows, not to mention creating a symlink look -- for that reason, to temporarily overcome the former until a better solution uncovers (on that below), I've added a hacky routine create_dangling_symlink which essentially creates a target directory, creates a symlink, and then deletes the target directory thus making the symlink dangling
opening files as directories with path_open needs more love ;-)

My current idea for symlinks on Windows is to create a thin, virtual wrapper Symlink (or similar) which will imitate *nix's functionality thus allowing for symlink loops and dangling symlinks until the symlink is actually made solid (by pointing to an actual resource). Then, the Symlink should know whether it points to a file or a dir thus making the cleanup simpler.

kubkon · 2019-08-01T21:32:12Z

cc @sunfishcode @alexcrichton

OK, so I guess the PR is ready for review and I'd really welcome your feedback here guys. I'm actually kinda stumped with some of this stuff, and especially handling of symlinks on Windows for the reasons mentioned in the PR's description. I've actually gone ahead and created a thin wrapper type, Symlink, around Windows symlinks that essentially tries to imitate *nix symlinks so that self loops and dangling symlinks are possible (otherwise the tests are broken). I'm not sure this is the way we want to go though. In fact, I'd welcome any and all discussion here on the best way to proceed.

Finally, if we decide to scrap this PR as-is, I'm OK with that as well. We should still be able to salvage the implementation of at least some of the path_ syscalls I guess.

kubkon · 2019-08-02T10:51:32Z

@sunfishcode here's a thought on the WASI spec: do you think it would make sense to disallow creation of symlink loops and dangling symlinks in WASI? Additionally, would it also make sense to split path_symlink syscall into two: path_symlink_file and path_symlink_dir? Those two changes to the spec would clean up the implementation on Windows a lot (to the best of my knowledge that is! @alexcrichton could you weigh in here as well please?), since then we'd only ever be allowed to open a symlink to an existing resource (which immediately precludes a symlink loop), and having separate syscalls for symlinking files and dirs would match Windows syscalls symlink_file and symlink_dir, whereas on *nix, they'd be one and the same.

This still doesn't address the problem of privileges on Windows: creating symlinks requires that the user has either the necessary privileges or the OS is in developer mode, but I guess it'd clean up the implementation a lot. Anyhow, I'd love to hear your thoughts on all of this!

alexcrichton · 2019-08-02T16:41:25Z

Without looking too much into the implementation, this overall looks pretty reasonable. I agree that symlink_file and symlink_dir should probably be added (or at least considered) for the spec itself, but for now having this sort of emulation for Windows seems reasonable. I suspect that removing looping/dangling symlinks may be harder to do, but supporting it the best we can for now seems totally reasonable as well.

kubkon · 2019-08-05T10:57:27Z

@sunfishcode did you perhaps have a look at the PR? I'd like to merge it into master before #46, #47 and #54 so as to avoid rebase conflicts and to give @marmistrz ability to enable+add more tests. :-)

sunfishcode

Looks good to me! path_get is some of the most complex code, and it's nice to have more of it factored out into common code.

sunfishcode · 2019-08-06T20:41:51Z

src/hostcalls_impl/fs_helpers.rs

+) -> Result<(File, String)> {
+    const MAX_SYMLINK_EXPANSIONS: usize = 128;
+
+    if path.contains("\0") {


Can this be a character literal instead of a string literal?

It should be indeed, thanks!

sunfishcode · 2019-08-06T20:58:48Z

src/hostcalls_impl/fs.rs

+    let (old_dirfd, old_path) = path_get(old_dirfd, 0, old_path, false)?;
+    let (new_dirfd, new_path) = path_get(new_dirfd, 0, new_path, false)?;

    hostcalls_impl::path_link(old_dirfd, new_dirfd, old_path, new_path)


This looks like it might be a little error prone, where one could forget to call path_get and in some cases it would still compile and seem to work. Would it make sense to create a struct to replace path_get's tuple return value? Then you could make the hostcalls_impl functions use that struct for their arguments, which would help ensure that path_get has been called. Does that make sense?

Hmm, that sounds like a good idea. Let me have a play with it and I'll report back to this thread :-)

OK, I've encapsulated the return value of path_get inside a PathGet struct. Have a look and let me know if this is what you've had in mind :-) Also, I can see several places we could refactor a lot of impl by extending this struct but I don't want to pile onto this PR too much. Instead, we can for sure refactor in smaller steps in subsequent PRs.

kubkon · 2019-08-07T13:36:56Z

@sunfishcode if you're happy with the rest of PR as is, I guess I'll go ahead and squash and merge so that we can move forward with other PRs?

sunfishcode

I'm not yet comfortable with the path_symlink implementation. I made a few comments below, but looking at the big picture, if symlink creation requires special privileges or a special OS mode, it suggests to me we should either:

remove path_symlink from WASI, or
document that support for path_symlink may be subject to limitations of the underyling filesystem, and maybe even add a way to query a directory to determine what form of symlinks may be created inside it

My suggestion would be to remove the SYMLINKS singleton here, since it's kind of misleading -- it helps emulate the desired functionality for the program that creates the symlinks, but they're not real and other processes wouldn't see them. I propose it's better to just have path_symlink fail in the cases that Windows doesn't support, and we'll take up the remaining questions at the WASI standards level. How does that sound?

sunfishcode · 2019-08-07T16:48:54Z

src/sys/windows/hostcalls_impl/symlink.rs

+        let target_path: &Path = target.as_ref();
+
+        if source_path.exists() {
+            Err(host::__WASI_EEXIST)


Is it necessary to check if the source_path exists up front, or can we defer this and only check it in the non-file non-dir case?

sunfishcode · 2019-08-07T16:50:48Z

src/sys/windows/hostcalls_impl/symlink.rs

+                SymlinkKind::File
+            } else if target_path.is_dir() {
+                symlink_dir(target_path, source_path).map_err(errno_from_ioerror)?;
+                SymlinkKind::Dir


How crazy would it be to do this: instead of checking is_file and is_dir, just start by calling symlink_file and if that fails with the right error code, then try symlink_dir, and if that fails, check for the loop/dangling cases?

kubkon · 2019-08-07T17:44:19Z

I'm not yet comfortable with the path_symlink implementation. I made a few comments below, but looking at the big picture, if symlink creation requires special privileges or a special OS mode, it suggests to me we should either:
* remove `path_symlink` from WASI, or

* document that support for `path_symlink` may be subject to limitations of the underyling filesystem, and maybe even add a way to query a directory to determine what form of symlinks may be created inside it
My suggestion would be to remove the SYMLINKS singleton here, since it's kind of misleading -- it helps emulate the desired functionality for the program that creates the symlinks, but they're not real and other processes wouldn't see them. I propose it's better to just have path_symlink fail in the cases that Windows doesn't support, and we'll take up the remaining questions at the WASI standards level. How does that sound?

Yep, sounds good to me! As I said, it's been a roller-coaster ride for me trying to get it to work on Windows. The emulation also made me somewhat uneasy since emulating a dangling symlink would not get committed to the OS until it became solid, which, if the app died prematurely, etc., would potentially never happen, leaving other processes none-the-wiser. Anyhow, is the following plan of action aligned with what you've had in mind:

creating dangling and self-loops will result in an error on Windows (no need for SYMLINKS singleton then)
if the OS is not set to allow for symlink creation, we return a __WASI_ENOTCAPABLE error from path_symlink
creating proper symlinks (pointing at existing resources) would be done in two stages: try symlink_file, if fails, try symlink_dir, otherwise, trip with an error
adapt test cases (whichever seem reasonable) to not use dangling symlinks

Those are just some loose thoughts but, in principle, I reckon I should be able to refactor the code to make it work.

Also, do you want me to address the comments relating SYMLINKS singleton if it's going to go anyhow? I'd imagine we just leave them be if that's the case.

kubkon · 2019-08-08T06:39:13Z

@sunfishcode OK, so here are the changes as per your suggestion:

removed SYMLINKS singleton altogether
creating dangling and self-loops now results in an error on Windows
if the user lacks permissions to create symlinks, we return a __WASI_ENOTCAPABLE error from path_symlink
creating proper symlinks (pointing at existing resources) is done in two stages: try symlink_file, if fails, try symlink_dir, otherwise, trip with an error
the test cases are adapted as follows:

readlink_no_buffer is ignored on Windows as it relates to interpreting what a dangling symlink is pointing at
symlink_loop related to symlink loops, hence it's also turned off on Windows
nofollow_errors are now split into two testcases: dangling_symlink and nofollow_errors; in the former, we check if opening a dangling symlink yields a __WASI_ELOOP error but only on *nix, and in the latter, we have most of the logic of the original nofollow_errors testcase but without any dangling symlinks.

Have a look and let me know what you reckon!

There is an additional caveat here that I've already mentioned in this PR: Windows does make a distinction between file and directory symlinks. I've adapted the nofollow_errors testcase so that we never try and unlink a symlink pointing at a dir and then link it to a file instead as on Windows that would not make much sense. Question and perhaps a suggestion here for WASI: should we perhaps have two methods path_symlink_file and path_symlink_dir instead of simply path_symlink in the spec? Alternatively, if we won't go down this road, I guess we'll need to ensure that in the docs, we emphasise that on Windows, the apps should not try and swap the target of a symlink as it might yield erroneous results on Windows host.

marmistrz · 2019-08-08T13:41:47Z

If path_symlink still requires some work, could we possibly merge this change without path_symlink and add the support for this syscall later on? I'd like to fix the conflicts of #46 with this PR and go ahead with path_filestat_get.

sunfishcode · 2019-08-08T13:50:06Z

Cargo.toml

 rand = "0.6"
 cfg-if = "0.1.9"
 log = "0.4"
+lazy_static = "1.3.0"


This dependency is no longer needed.

Ah, good catch, thanks!

sunfishcode

Other than the comment above, this LGTM, so feel free to merge when ready.

sunfishcode · 2019-08-08T13:58:20Z

I don't yet have an opinion about splitting path_symlink into file vs directory forms. My impression is that the technique of doing symlink_file and falling back to symlink_dir as needed is workable, though it is a little suboptimal. However, the problem of users trying to swap out the target of a symlink exists even if we split path_symlink into two forms.

kubkon · 2019-08-08T14:15:09Z

I don't yet have an opinion about splitting path_symlink into file vs directory forms. My impression is that the technique of doing symlink_file and falling back to symlink_dir as needed is workable, though it is a little suboptimal. However, the problem of users trying to swap out the target of a symlink exists even if we split path_symlink into two forms.

Unfortunately, that's very true on Windows. Perhaps this will require some more thought. :-)

This way, we can re-enable nofollow_errors testcase on Windows also.

kubkon changed the title ~~path_get refactor and implementation of missing path_ hostcalls readlinkat on Windows~~ path_get refactor and implementation of missing path_ hostcalls on Windows Jul 23, 2019

kubkon mentioned this pull request Jul 24, 2019

Implement fd_filestat_get for all platforms #42

Merged

kubkon marked this pull request as ready for review August 1, 2019 21:21

kubkon mentioned this pull request Aug 4, 2019

Allow colons in Windows host paths bytecodealliance/wasmtime#235

Merged

kubkon mentioned this pull request Aug 5, 2019

Reuse errno_from_ioerror to simplify error handling #46

Merged

sunfishcode reviewed Aug 6, 2019

View reviewed changes

Jakub Konka and others added 20 commits August 7, 2019 14:05

Move path_get outside of sys module

275eb7a

Add implementation of readlinkat

669e0c6

Clean up path_open; use OpenOptions as much as possible

3767602

Enable close_preopen test

934c9fb

Implement path_create_directory; fix path_open

d3e2770

Refactor path concatenation onto a descriptor

a4094d9

Implement path_remove_directory

05fb0dc

Implement path_unlink_file

58893e6

Rewrite path_open using specific access mask

77549cd

Fix error mapping when unlinking file

a3daad0

Fix readlinkat to pass nofollow_errors testcase

107cc88

Clean up winerror to WASI conversion

223bf0b

Spoof creating dangling symlinks on windows (hacky!)

22fcaf7

Add positive testcase for readlink

7860fa7

Implement path_readlink (for nonzero buffers for now)

1fbd7a5

Clean up

378dc61

Add Symlink struct immitating *nix symlink

d9b0876

Fix path_readlink

92c8215

Augment interesting_paths testcase with trailing slashes example

7e78c43

Encapsulate path_get return value as PathGet struct

4a03234

sunfishcode reviewed Aug 7, 2019

View reviewed changes

sunfishcode reviewed Aug 8, 2019

View reviewed changes

sunfishcode approved these changes Aug 8, 2019

View reviewed changes

kubkon added 3 commits August 8, 2019 16:17

Remove dangling symlink emulation

bb48200

Extract dangling symlinks into its own testcase

d7673fe

This way, we can re-enable nofollow_errors testcase on Windows also.

Return __WASI_ENOTCAPABLE if user lacks perms to symlink

0ea1b5b

kubkon merged commit e18175c into CraneStation:master Aug 8, 2019

kubkon deleted the path_get_refactor branch August 8, 2019 15:06

Conversation

kubkon commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubkon commented Aug 1, 2019

Uh oh!

kubkon commented Aug 2, 2019

Uh oh!

alexcrichton commented Aug 2, 2019

Uh oh!

kubkon commented Aug 5, 2019

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented Aug 7, 2019

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubkon commented Aug 7, 2019

Uh oh!

kubkon commented Aug 8, 2019

Uh oh!

marmistrz commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented Aug 8, 2019

Uh oh!

kubkon commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kubkon commented Jul 23, 2019 •

edited

Loading

marmistrz commented Aug 8, 2019 •

edited

Loading