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.

Sign up for GitHub

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

Jump to bottom

WASI symlink implementation is incorrect #51

Closed
RReverser opened this issue Jan 30, 2021 · 7 comments · Fixed by #52
Closed

WASI symlink implementation is incorrect #51

RReverser opened this issue Jan 30, 2021 · 7 comments · Fixed by #52

Comments

@RReverser
Copy link
Contributor

Actually, wait, this PR is wrong. The file descriptor in the API is supposed to be a preopen directory descriptor, not the descriptor for the target file.

Originally posted by @RReverser in #50 (comment)
@RReverser
Copy link
Contributor Author

Until rust-lang/rust#68574 is fixed, it's best to either use symlink via libc, or fallback to not supporting them altogether.

@vitiral
Copy link
Owner

vitiral commented Jan 30, 2021 via email

@RReverser
Copy link
Contributor Author

That's totally why I haven't merged yet...

Can't tell if you're just joking or I'm misunderstanding what you're saying 🤔 The PR above is merged...

@RReverser
Copy link
Contributor Author

FWIW I meanwhile made a PR to Rust to expose the correct API: rust-lang/rust#81542

Let's see how that goes...

@vitiral
Copy link
Owner

vitiral commented Feb 2, 2021

Sorry, I meant it's the reason I haven't pushed and was joking. The reason I haven't pushed is 🅻🅸🅵🅴

I'll hold off on updating the library until someone tells me the implementation is correct. I'd be willing to also accept a PR to add a crate feature for wasi and use the wasi crate to implement the "correct" behavior, as suggested in rust-lang/rust#81542

@RReverser
Copy link
Contributor Author

Sorry, I meant it's the reason I haven't pushed and was joking. The reason I haven't pushed is 🅻🅸🅵🅴

Ah, that's how I understood it first too, but wanted to double-check :)

I have a pending PR to implement correct behaviour for symlinks, but first had to make some PRs to add WASI support to tempfile crate which is used for tests in this one.

Now that Stebalien/tempfile#138 is merged, I'll be able to make a PR here soon-ish too.

RReverser added a commit to RReverser/path_abs that referenced this issue Feb 19, 2021
@RReverser
Fix WASI symlink implementation
3dfaba2 
As described in vitiral#51, current implementation is incorrect and won't work and, as described in rust-lang/rust#68574, there was no correct way to use that API.

I went ahead and added a new API for symlinks in rust-lang/rust#81542, and now that it's landed, it can be used to correctly create symlinks.

Fixes vitiral#51.
@RReverser RReverser mentioned this issue Feb 19, 2021
Merged
@RReverser
Copy link
Contributor Author

Fix in #52 (ignoring the tests for now, as the situation around temporary files in WASI is pretty tricky and the previous implementation didn't work with them either, so no point in blocking on them).

vitiral pushed a commit that referenced this issue Feb 22, 2021
@RReverser
Fix WASI symlink implementation (#52)
771c0f5 
As described in #51, current implementation is incorrect and won't work and, as described in rust-lang/rust#68574, there was no correct way to use that API.

I went ahead and added a new API for symlinks in rust-lang/rust#81542, and now that it's landed, it can be used to correctly create symlinks.

Fixes #51.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix WASI symlink implementation RReverser/path_abs
2 participants
@RReverser @vitiral