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

Fix opam unable to find executables on systems where users belong to more than 32 groups when opam is built using musl libc #5381

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Fix opam unable to find executables on systems where users belong to more than 32 groups when opam is built using musl libc #5381

merged 4 commits into from
Sep 17, 2024

Conversation

kit-ty-kate
Copy link
Member

Details of the issue in musl described in https://www.openwall.com/lists/musl/2021/07/03/1
Fixes #5373

@dra27
Copy link
Member

dra27 commented Dec 6, 2022

I think there's a couple of bits in configure.ac from 5377b60#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810 which also need reverting.

We should double-check that Unix.access doesn't regress either d6faa8d or 6f756ef (this is probably just a case of checking the Open Group POSIX docs... I'll be amazed if it regresses the first and quite surprised if it regresses the second!). It would be nice to check that the original bug being fixed in #4265, but that may be too tricky to set-up again.

@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Dec 7, 2022
@rjbou rjbou requested a review from dra27 December 7, 2022 17:56
@dra27
Copy link
Member

dra27 commented Dec 9, 2022

Annoying, Unix.access checks ruid and rgid rather than euid and egid (which is what exec will do). Ideally we therefore want to use faccessat with AT_EACCESS instead, which should be exactly equivalent to the present check, but without needing the libacl hackery for Cygwin (and, in fairness, any other Unix system which chooses to grant permissions with ACLs instead). However, that would cause opam to require C stubs for all builds, and I'm not so sure about that 🤷

@dra27
Copy link
Member

dra27 commented Dec 14, 2022

Discussed today: we decided we're not worried about requiring stubs in opam-core for all platforms, so wrapping faccessat should be fine, which we can then use instead of Unix.access. Happy for either of us to get to this - either way, we get rid of the libacl bindings!

@kit-ty-kate kit-ty-kate mentioned this pull request Dec 14, 2022
Closed
@dra27 dra27 removed this from the 2.2.0~alpha milestone Jan 23, 2023
@kit-ty-kate kit-ty-kate force-pushed the unix-access branch 2 times, most recently from 2d6126b to 1f6ebf2 Compare July 8, 2024 21:13
@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 8, 2024
@kit-ty-kate kit-ty-kate force-pushed the unix-access branch 4 times, most recently from 25f8b6c to d26ffce Compare September 8, 2024 14:56
@rjbou rjbou self-requested a review September 16, 2024 12:16
rjbou
rjbou reviewed Sep 16, 2024
View reviewed changes
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Some format remarks : commits to squash, etc.
Also, commit messages are not very explicit, an update with more information could help.

master_changes.md Outdated Show resolved Hide resolved
src/stubs/common/opamCommonStubs.mli Outdated Show resolved Hide resolved
src/core/opamStubsTypes.ml Show resolved Hide resolved
src/core/opamCommonStubs.c Show resolved Hide resolved
@kit-ty-kate kit-ty-kate requested a review from rjbou September 17, 2024 10:51
@kit-ty-kate
Copy link
Member Author

squashed

rjbou
rjbou reviewed Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Does doc/index.html need to be updated by the last commit (or others) ?

master_changes.md Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

Does doc/index.html need to be updated by the last commit

done. Feel free to squash if you're fine with the two last fixup commits

kit-ty-kate and others added 4 commits September 17, 2024 16:09
@kit-ty-kate
Remove the --with-acl option from the configure script
03a3dbb
@dra27 @kit-ty-kate
Use copy# where appropriate in dune files
3d283f9
@kit-ty-kate
resolve_command: Use faccessat instead of custom permission check
0e74798 
Unix.access uses RUID and RGID, which is not correct when doing a
PATH-search. The faccessat function is able to check permissions using
EUID and EGID instead. opam's hand-rolled check_permissions function is
therefore replaced with a binding for faccessat.

This simultaneously fixes two other things:
- Platforms (such as Cygwin) which use ACLs no longer need special
  support, because their implementations of faccessat already take ACLs
  into account
- We no longer use Unix.getgroups which means that we work around a
  problem with binaries built using musl libc and then used on systems
  where a user belongs to more than 32 groups
  (cf. https://www.openwall.com/lists/musl/2021/07/03/1)
@dra27 @kit-ty-kate
Move Windows stubs to opam-core
1e79693 
As opam now routinely has stubs, eliminate the complicated dances for
only linking stub libraries on Windows.
rjbou
rjbou approved these changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks!

@kit-ty-kate kit-ty-kate merged commit 21bb251 into ocaml:master Sep 17, 2024
29 checks passed
This was referenced Sep 17, 2024
Closed
Merged
kit-ty-kate pushed a commit to dra27/opam that referenced this pull request Sep 18, 2024
@dra27 @kit-ty-kate
release: Remove the getgroups workaround allowing users belonging to …
ac8a72d 
…more than 32 groups

Following the merge of ocaml#5381 this workaround is no longer needed
@kit-ty-kate kit-ty-kate mentioned this pull request Sep 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjbou rjbou rjbou approved these changes

@dra27 dra27 Awaiting requested review from dra27

Assignees
No one assigned
Labels
KIND: BUG PR: WAITING FOR REVIEW
Projects
None yet
Milestone
2.3.0~alpha1
Development

Successfully merging this pull request may close these issues.

opam init cannot find tools
3 participants
@kit-ty-kate @dra27 @rjbou