Skip to content

stdlib std.os: Update CWD emulation to match wasi-libc behavior #11053

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 1 commit into from
Apr 16, 2022

Conversation

topolarity
Copy link
Contributor

@topolarity topolarity commented Mar 4, 2022

Dependent on #11021 .

Some more work is needed to align our CWD treatment on WASI with wasi-libc. This is particularly important for runtimes like wasmer, which inserts a special "/" Preopen by default and also strips leading slashes from other Preopens

Here's the idea behind these changes:

  1. We store the CWD as a simple []const u8. We lookup Preopens for every absolute or CWD-referenced file operation, based on the Preopen with the longest match (i.e. most specific path)
  2. Preorders are normalized to POSIX absolute paths at init time, based on the cwd_root parameter of initPreopensWasi. cwd_root is used only for any Preopens that start with "." so that:
   "./foo/bar" ~ canonicalizes to ~> "{cwd_root}/foo/bar"
   "foo/bar"   ~ canonicalizes to ~> "/foo/bar"
   "/foo/bar"  ~ canonicalizes to ~> "/foo/bar"

The intention is that:

  • Using "/" as cwd_root gives behavior similar to wasi-libc
  • Applications can provide a unique prefix, such as "/preopens/cwd" to prevent name collisions, esp. with the "." Preopen

These changes come at a performance cost, since we no longer cache any matching Preopen for the CWD, but the implementation is also much simpler and the compatibility gain from matching wasi-libc's behavior seems worth it.

Another consequence is that we no longer support fchdir on WASI

@topolarity
Copy link
Contributor Author

topolarity commented Mar 4, 2022

cc @zigazeljko

Can you take a peek at 4830e16 here and see what you think of the new approach? I'm open to any alternatives or concerns that might jump out to you.

@zigazeljko
Copy link
Contributor

Looks good to me.

@topolarity topolarity force-pushed the wasi-libc-parity branch 3 times, most recently from 4830e16 to 0fff5c4 Compare March 4, 2022 19:29
@kubkon
Copy link
Member

kubkon commented Mar 5, 2022

#11021 is now merged - would you mind rebasing onto master please?

@topolarity topolarity force-pushed the wasi-libc-parity branch 3 times, most recently from a6b53cd to 5d9f508 Compare March 5, 2022 20:55
@matu3ba
Copy link
Contributor

matu3ba commented Mar 7, 2022

  1. Question for edge case: Does "../foo/bar" ~ canonicalize to ~> "{cwd_root}/foo/bar" or is it invalid/creates an error?
    A test for this behavior would be useful.

  2. Does this also solve the shortcoming of realpath requiring a prefixed / or will this be a separate commit? see Add assumptions for resolveWindows, resolvePosixand realpath #10988

  3. Can you add to the doc comments of function realpath "assume: relative path is given from from cwd() of process" ?
    And "assume: / followed by file description can be cded to" ?

@topolarity
Copy link
Contributor Author

  1. Question for edge case: Does "../foo/bar" ~ canonicalize to ~> "{cwd_root}/foo/bar" or is it invalid/creates an error?

A very good question. This would be canonicalized as {cwd_root}/../foo/bar after the substitution of "cwd_root". So if cwd_root is "/", this resolves to "/foo/bar". If cwd_root is "/preopens/cwd", this resolves to "/preopens/foo/bar".

Whether this behavior is "right" probably depends on how WebAssembly/wasi-filesystem#24 is resolved. I don't have a strong argument either way, right now

  1. Does this also solve the shortcoming of realpath requiring a prefixed / or will this be a separate commit? see Add assumptions for resolveWindows, resolvePosixand realpath #10988

The WASI implementation for realpath added here does not require a prefixed /, and the resolve always includes an absolute path (i.e. the absolute CWD)

  1. Can you add to the doc comments of function realpath "assume: relative path is given from from cwd() of process" ?
    And "assume: / followed by file description can be cded to" ?

That seems like it should be tackled in your existing PR #10988

@kubkon kubkon force-pushed the wasi-libc-parity branch from 5d9f508 to abd9e4b Compare April 16, 2022 10:01
Two major changes here:
  1. We store the CWD as a simple `[]const u8` and lookup Preopens for
     every absolute or CWD-referenced file operation, based on the
     Preopen with the longest match (i.e. most specific path)
  2. Preorders are normalized to POSIX absolute paths at init time.
     Behavior depends on the "cwd_root" parameter of `initPreopensWasi`:

	`cwd_root` is used for any Preopens that start with "."

	  For example:
            "./foo/bar" - inits to -> "{cwd_root}/foo/bar"
            "foo/bar"   - inits to -> "/foo/bar"
	    "/foo/bar"  - inits to -> "/foo/bar"

        `cwd_root` must be an absolute path.

	Using "/" as `cwd_root` gives behavior similar to wasi-libc.
@kubkon kubkon force-pushed the wasi-libc-parity branch from abd9e4b to 8f6fd5e Compare April 16, 2022 10:36
@kubkon kubkon merged commit 7b090df into ziglang:master Apr 16, 2022
@topolarity topolarity deleted the wasi-libc-parity branch April 16, 2022 16:22
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.

4 participants