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

cd: open directory with O_PATH or O_SEARCH #10442

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

Conversation

TheJonny
Copy link

Description

fish implements the cd builtin using the system calls open and fchdir. On platforms where it is supported, this PR changes the flags argument for open to O_SEARCH or O_PATH, which does not require read permission on the directory we change into. This is more consistent with the behavior of chdir or cd in other shells.

also it cleans up the unused "mode" argument to fds::[w]open_dir.

Fixes issue #10432
Alternative to #10436 or #10433

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages. (no usage changes should appear)
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

[w]open_dir does not pass O_CREAT, so the mode argument to open is never used.
also, O_CREAT | O_DIRECTORY could not be used (portably) to create a directory.
(on POSIX does not specify what should happen, on Linux it is EINVAL.)
@TheJonny
Copy link
Author

I opened rust-lang/libc#3659 to add O_SEARCH to the libc crate for macos.

  • do we want to hardcode the constant for macos or wait?
  • do we want to add it to nix first?

@zanchey
Copy link
Member

zanchey commented Apr 15, 2024

From the linked source, it looks like they only exist on macOS 13+. This probably needs a build.rs check.

@TheJonny
Copy link
Author

From the linked source, it looks like they only exist on macOS 13+. This probably needs a build.rs check.

that would also be cleaner, I think.

i would add cfg flags HAVE_O_SEARCH and HAVE_O_PATH in build.rs, and not hardcode O_SEARCH = 0x40100000, but wait for it to land in libc.

any opinions on this?

@TheJonny
Copy link
Author

From the linked source, it looks like they only exist on macOS 13+. This probably needs a build.rs check.

@zanchey you are right, O_SEARCH does not work on macos 12. but the #define exists in the libc - at least in the github action. (I don't have access to another macos 12)
It works fine with macos-13.

I tried it here: https://github.com/TheJonny/mactest/

@TheJonny
Copy link
Author

O_SEARCH = (0x40000000 | 0x00100000)
does the right thing on macos 13, and can open directories with read permission on macos 12, so it should silently fall back to the current behavior without a compile time version check.

@ridiculousfish
Copy link
Member

For macOS, rather than a build.rs check, I'd rather do a runtime check - that way we can provide a single package for 10.9+. I will be happy to implement that.

@TheJonny
Copy link
Author

For macOS, rather than a build.rs check, I'd rather do a runtime check

After a quick experiment using the macos-12 github runner, just adding the O_SEARCH flag from macos-13 has no effect, opening readable directories still works. in this case, the runtime check is not needed in fish, because it is also implemented (by ignoring it) in the kernel. but we should verify it on more cases / versions / platforms

@ridiculousfish ridiculousfish self-requested a review April 24, 2024 03:37
@ridiculousfish
Copy link
Member

I do intend to validate the behavior on macOS <=12 but I just discovered the Python tests haven't been running on my Mac for a while now and they've bitrotted...anyways I haven't forgotten this

@TheJonny
Copy link
Author

TheJonny commented May 1, 2024

Also the next nix release should contain the O_SEARCH flag for all platforms :)

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.

None yet

3 participants