Skip to content

stdlib: Add emulated CWD to std.os for WASI targets #11021

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

Merged
merged 3 commits into from
Mar 5, 2022

Conversation

topolarity
Copy link
Contributor

@topolarity topolarity commented Mar 1, 2022

This PR provides emulated relative and absolute path support for std.os on WASI targets (resolves #10749).

"os.initPreopensWasi()" must be called before std.os functions can resolve relative or absolute paths, which is asserted at runtime.

Support has been added for: open, rename, mkdir, rmdir, chdir, fchdir, link, symlink, unlink, readlink, fstatat, access, and faccessat.

This also includes limited support for getcwd() and realpath(). These return an error if the CWD does not correspond to a Preopen with an absolute path. They also do not currently expand symlinks. Tests have been updated to expect the CWD at "/cwd" rather than ".", so that "realpath" will work for CWD-relative paths.

@topolarity topolarity force-pushed the wasi-cwd branch 3 times, most recently from 3565b6c to 71ea69d Compare March 1, 2022 18:57
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

A couple of comments to get the ball rolling. I'm still reading over the changes, but I'll try to leave feedback incrementally to make it more manageable.

@topolarity
Copy link
Contributor Author

Thanks for the review!

Just pushed a small fix for a Windows-specific test failure and corrected the stylistic problems you pointed out.

Copy link
Member

@kubkon kubkon 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 for this excellent contribution @topolarity !

@zigazeljko
Copy link
Contributor

@topolarity How is initPreopensWasi supposed to be used -- in other words, what should init_cwd be set to to make path resolution work in a "normal" way?

@zigazeljko
Copy link
Contributor

To clarify my previous comment, let me give an example. Assume the program is given these preopens:

  • fd 3 as /foo,
  • fd 4 as ./foo,
  • fd 5 as ./bar,
  • fd 6 as ./bar/quux.

Intuitively, the user would expect the path resolution to work like this:

  • /foo/aaa resolves to (fd=3, rel_path="aaa"),
  • foo/bbb resolves to (fd=4, rel_path="bbb"),
  • ./foo/ccc resolves to (fd=4, rel_path="ccc"),
  • ./bar/ddd resolves to (fd=5, rel_path="ddd"),
  • bar/eee/../quux/fff resolves to (fd=6, rel_path="eee/fff").

This is exactly how path resolution used to work in wasi-libc, before they introduced CWD emulation. If I understand the code correctly, such behavior is not possible with the implementation in this PR.

When init_cwd is null, resolvePathAndGetWasiPreopen behaves as if CWD was set to "/", which means:

  • foo/bbb is treated as /foo/bbb and resolves to (fd=3, rel_path="bbb"),
  • ./foo/ccc is treated as /foo/ccc and resolves to (fd=3, rel_path="ccc"),
  • ./bar/ddd and bar/eee/../quux/fff are not found.

When init_cwd is ".", initPreopensWasi fails, since there is no preopen for ".", only for its subdirectories.

When init_cwd is (let's say) "./foo", initPreopensWasi works, but:

  • foo/bbb resolves to (fd=4, rel_path="foo/bbb"),
  • similarly for ./foo/ccc, ./bar/ddd, and bar/eee/../quux/fff,
  • other CWD-relative preopens are not reachable with normal paths.

@topolarity
Copy link
Contributor Author

topolarity commented Mar 3, 2022

Thanks for the detailed input @zigazeljko.

I'm planning on opening an additional PR this evening to bring us up to parity with the "wasi-libc" behavior. initPreopensWasi(..., null) will give you the wasi-libc behavior by default. Under those rules, "./foo", "/foo", and "foo" preopens would be equivalent - the highest fd would take priority and is available at /foo . This behavior is necessary to support wasmer-style Preopens, which are prefix-stripped by the runtime (no leading . or /).

Nested preopens will need to be fixed up in that PR (or we'll need to detect and reject them).

The only bit I'm unsure about is whether we can achieve the "intuitive" behavior of distinguishing ./foo/bar vs. /foo/bar/, especially since some runtimes (wasmer) do not support this.

I'm going to cc you there so that we can discuss in more detail :-)

This adds a special CWD file descriptor, AT.FDCWD (-2), to refer to the
current working directory. The `*at(...)` functions look for this and
resolve relative paths against the stored CWD. Absolute paths are
dynamically matched against the stored Preopens.

"os.initPreopensWasi()" must be called before std.os functions will
resolve relative or absolute paths correctly. This is asserted at
runtime.

Support has been added for: `open`, `rename`, `mkdir`, `rmdir`, `chdir`,
`fchdir`, `link`, `symlink`, `unlink`, `readlink`, `fstatat`, `access`,
and `faccessat`.

This also includes limited support for `getcwd()` and `realpath()`.
These return an error if the CWD does not correspond to a Preopen with
an absolute path. They also do not currently expand symlinks.
@zigazeljko
Copy link
Contributor

which are prefix-stripped by the runtime (no leading . or /)

Is this documented anywhere and/or have you tested it?

I just tested this in wasmer (versions 1.0.0 and 2.2.0) and while leading / is stripped, leading . is not stripped. For my example above, I get:

$ wasmer --mapdir=/foo:foo --mapdir=./foo:foo --mapdir=./bar:bar --mapdir=./bar/quux:quux preopens.wasm
3 -> /
4 -> foo
5 -> ./foo
6 -> ./bar
7 -> ./bar/quux

As far as I tested, wasmer only trims all leading /, but does no other path normalization (i.e. it keeps leading ., trailing /, repeated /, and . and .. components).

Additionally, the first preopen (3 -> /) is always present, and is also the only preopen that can begin with a / (using --mapdir=/:dir gives you a preopen with an empty name).

@topolarity
Copy link
Contributor Author

You're right, I had only tested absolute paths. Thanks for the correction - That's an important detail.

Let's continue this discussion in the new PR, once its open?

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.

add the concept of cwd to standard library file system abstraction for WASI
3 participants