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

fs.Dir: Give the option to stat a symbolic link #20843

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

samy-00007
Copy link

This pull request adds Dir.statLink (with Z and W variants) to stat symlink.
It also improves the windows version of Dir.statFile.

@alexrp
Copy link
Contributor

alexrp commented Jul 28, 2024

Note that this will need Linux-specific statx()-based code once #20389 is merged.

lib/std/fs/Dir.zig Outdated Show resolved Hide resolved
lib/std/fs/Dir.zig Outdated Show resolved Hide resolved
lib/std/fs/test.zig Outdated Show resolved Hide resolved
@alexrp
Copy link
Contributor

alexrp commented Jul 30, 2024

Note that this will need Linux-specific statx()-based code once #20389 is merged.

Just dropping in again to say that it has been merged.

@samy-00007
Copy link
Author

I will make the necessary changes.

@samy-00007
Copy link
Author

It should be good.

@samy-00007
Copy link
Author

samy-00007 commented Jul 31, 2024

From what i am seeing, the test fails with wasm because in stage1/wasi.c, DirEntry_lookup which is used to stat the file ignores the flags, and so follows the symlink. Should i skip the test for wasi ? (Unless this is not the where the wasm functions are defined. I don't know enough about that)

@squeek502
Copy link
Collaborator

squeek502 commented Jul 31, 2024

Unless this is not the where the wasm functions are defined. I don't know enough about that

That's not the relevant code for the test failure (that's only used for bootstrapping a Zig compiler).

My guess is that it's wasi-libc which is ignoring the AT.SYMLINK_NOFOLLOW flag of fstat called from here, but will need to confirm (see #20747 for a similar wasi-libc bug). My second guess is that this test failure will only be present when linking wasi-libc, but also need to confirm that.


EDIT: Confirmed both guesses:

strace of the Dir.readLink function when linking wasi-libc (lack of O_NOFOLLOW in the openat2 flags):

openat2(13, "symlink", {flags=O_RDONLY|O_CLOEXEC|O_PATH, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 11
statx(11, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0664, stx_size=17, ...}) = 0
close(11)                               = 0

and without (inclusion of O_NOFOLLOW in the openat2 flags):

openat2(13, "symlink", {flags=O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 11
statx(11, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFLNK|0777, stx_size=9, ...}) = 0
close(11)                               = 0

This is very likely to be either a wasmtime or a wasi-libc bug. Will try to put together a reproduction with wasi-sdk and report it downstream.


EDIT#2: Unable to find a reproduction, filed a follow-up issue: #20890

lib/std/fs/Dir.zig Outdated Show resolved Hide resolved
lib/std/fs/Dir.zig Outdated Show resolved Hide resolved
samy-00007 and others added 2 commits August 1, 2024 10:36
Co-authored-by: Ryan Liptak <squeek502@hotmail.com>
@samy-00007
Copy link
Author

Is it possible to redo the CI tests ? They did not fail because of my code

@squeek502
Copy link
Collaborator

squeek502 commented Aug 2, 2024

Rebase the branch onto the latest master branch and it should get fixed.

@samy-00007
Copy link
Author

Is it good ?

lib/std/fs/Dir.zig Outdated Show resolved Hide resolved
@squeek502
Copy link
Collaborator

squeek502 commented Aug 6, 2024

Looks good to me.

One possible variation on this would be to make statFile take an options struct parameter with a follow_symlinks field (instead of having separate pub statLink functions). Unsure which would be better, will leave that up to the core team member(s) that review this.

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.

3 participants