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 is_absolute on WASI #77362

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Fix is_absolute on WASI #77362

merged 1 commit into from
Oct 2, 2020

Conversation

RReverser
Copy link
Contributor

@RReverser RReverser commented Sep 30, 2020

WASI does not match cfg(unix), but its paths are Unix-like (/some/path) and don't have Windows-like prefixes.

Without this change, is_absolute for any paths, including /some/path, was returning falseon a WASI target, which is obviously not true and undesirable.

WASI does not match `cfg(unix)`, but its paths are Unix-like (`/some/path`) and don't have Windows-like prefixes.

Without this change, `is_absolute` for paths like `/some/path` was returning `false`on a WASI target, which is obviously not true and undesirable.
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Oct 1, 2020

This sounds reasonable but I don't know anything about WASI, so I'll poke the last few people who touched WASI filesystem stuff for another set of eyes: @ibmibmibm @sunfishcode. Please shout if you notice anything wrong with adopting a unix-equivalent definition of what constitutes an absolute path for WASI.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit 494d6e5 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020
@CryZe
Copy link
Contributor

CryZe commented Oct 1, 2020

I believe to some degree that's intentional. There's various documentation stating there's no absolute paths on WASI.

Such as here: https://docs.rs/wasi-common/0.18.0/wasi_common/fs/struct.Dir.html

Unlike std::fs, this API has no canonicalize, because absolute paths don't interoperate well with the capability-oriented security model.

and here: https://docs.rs/wasi-common/0.18.0/wasi_common/fs/index.html

This combined with WASI's lack of absolute paths provides a natural capability-oriented interface.

and here: WebAssembly/wasi-libc#81 (comment)

Yeah, WASI doesn't have absolute paths, so the standard realpath doesn't fit.

@dtolnay
Copy link
Member

dtolnay commented Oct 1, 2020

Thank you!

What do you make of that, @RReverser?

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 1, 2020
@RReverser
Copy link
Contributor Author

That's an interesting view. I've rather seen it as "WASI doesn't have relative paths" (because there is no concept of current working directory) not "WASI doesn't have absolute paths" (because those are the only kind of paths that WASI path APIs do accept).

In fact, being able to distinguish between those two in a cross-platform manner is exactly the reason I wanted this function to work.

As a practical example, Rust port of coreutils has its own userland implementation of canonicalize / realpath that uses this function https://github.com/uutils/uucore/blob/6b7e1cd29812aa30e28b720fce0e52dcf25f9a74/src/lib/features/fs.rs#L103-L109 in order to determine whether path should be normalised as-is or first prepended with the current working directory.

Since is_absolute unconditionally returns false, such function would always try to prepend the current working directory by getting std::env::current_dir, and reasonably crash because, as mentioned above, this concept doesn't exist on WASI.

However, if is_absolute would return true for absolute paths, it would be able to normalise such paths and continue execution.

Personally, I think that even though WASI doesn't have absolute paths in the traditional sense (not every path level exists, you can't go between them etc.), it would be still useful to distinguish "/some/path" mount points as absolute, while any other as relative, so that code that can deal with the former would continue work on WASI just like it does on any other system.

@sunfishcode
Copy link
Member

@RReverser I agree. The docs are admittedly ambiguous on this point, WASI functions interpret a path starting with / as an absolute path; they just always produce io::ErrorKind::PermissionDenied errors when given such paths.

@RReverser
Copy link
Contributor Author

they just always produce io::ErrorKind::PermissionDenied errors when given such paths

I assume you mean, unless such path can be matched via preopened dirs, correct?

@sunfishcode
Copy link
Member

they just always produce io::ErrorKind::PermissionDenied errors when given such paths

I assume you mean, unless such path can be matched via preopened dirs, correct?

That's a good point, and another reason this PR makes sense. The WASI "system calls" themselves do require relative paths, however in "userspace", library code maps absolute paths into handle + relative path. Above that library abstraction layer, WASI does support absolute paths in a sense, so it makes sense to have Path::is_absolute be able to reflect that.

@RReverser
Copy link
Contributor Author

@sunfishcode Sounds like we're on the same page, thanks for the confirmation.

@dtolnay
Copy link
Member

dtolnay commented Oct 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit 494d6e5 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#76851 (Fix 'FIXME' about using NonZeroU32 instead of u32.)
 - rust-lang#76979 (Improve std::sys::windows::compat)
 - rust-lang#77111 (Stabilize slice_ptr_range.)
 - rust-lang#77147 (Split sys_common::Mutex in StaticMutex and MovableMutex.)
 - rust-lang#77312 (Remove outdated line from `publish_toolstate` hook)
 - rust-lang#77362 (Fix is_absolute on WASI)
 - rust-lang#77375 (rustc_metadata: Do not forget to encode inherent impls for foreign types)
 - rust-lang#77385 (Improve the example for ptr::copy)
 - rust-lang#77389 (Fix some clippy lints)
 - rust-lang#77399 (BTreeMap: use Unique::from to avoid a cast where type information exists)
 - rust-lang#77429 (Link `new` method in `DefautHasher`s doc)

Failed merges:

r? `@ghost`
@bors bors merged commit 55d0959 into rust-lang:master Oct 2, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 2, 2020
@RReverser RReverser deleted the patch-1 branch October 2, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants