-
Notifications
You must be signed in to change notification settings - Fork 368
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
Restore `Denied behaviour of PATH resolution #4265
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Excellent, our old friend the |
dra27
force-pushed
the
break-cygwin-again
branch
6 times, most recently
from
July 13, 2020 13:28
614344d
to
0921640
Compare
Wow! Impressive detective work, and I agree with your recommendations about testing |
dra27
force-pushed
the
break-cygwin-again
branch
from
July 17, 2020 08:22
0921640
to
ac3474d
Compare
When resolving a command in PATH, `Not_found should be returned only if the command was not in all the directories listed in PATH. If the command was found in at least one, but none of the commands found were executable, then `Denied should be returned instead.
A principal does not get the union of user, group and other permissions - you only ever get one part of the mask.
Permissions checking is based on effective UID. It is undefined whether getgroups includes the effective GID, so include it in the list explicitly.
libacl is only mandatory for Cygwin and must be explicitly selected for other platforms. For now, expressly binding to libacl in order to have acl_get_perm, rather than adding general ACL support (see libarchive for the full horrendous details of how to do this portably otherwise).
Need 3.1.7 when it's released.
rjbou
force-pushed
the
break-cygwin-again
branch
from
July 21, 2020 10:37
ac3474d
to
61a0d62
Compare
Rebased & fix upgrade job (no hash needed for opam 1.2.2 init) |
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-on from #4072, which alas is evidence that absence of evidence is not evidence of absence 🙂
This PR alters
OpamSystem.t_resolve_command
so that`Not_found
is only returned ifname
was not present in of the directories ofPATH
. Ifname
is present in at least one directory, but all the matches fail the permissions check, then`Denied
is now returned instead, which more accurately mirrors the previous behaviour (and is also consistent with the shell's behaviour).This is a draft PR, as sadly this re-opens my need to fix the Cygwin's permission check...While working on this, I also noticed that
check_perms
was not using the correct set of uids/gids - it now correctly uses the EUID and also ensures that EGID is included in the list of groups considered (sincegetgroups
is not required to return the EGID).The fix to the behaviour restores the breakage which we'd seen on Cygwin CI since late September last year. I chased down what was broken in January, but erroneously put fixing it properly on the backburner as CI started working when #4072 was merged.
The history is mildly interesting, and yields another cautionary tale for our testing:
"cmd": permission denied.
on the Cygwin32 build during initial switch creation. This was coming from the command inOpamSysPoll.os_version_lazy
failing the permissions check inOpamSystem.t_resolve_command
. As we didn't think we'd changed anything, I initially assumed that Cygwin had changed the way the ACL handling was working, but that was incorrect (it hasn't changed since at least Cygwin 1.7, which is a very long time ago). It turns out that 2 packages from issuu/ocaml-protoc-plugin at 0.9 opam-repository#14908 innocently created the first opam package in opam-repository to useos-version
in anavailable
field which means that determining the set of available packages will now always force that variable. This was merged on 26 Sep 2019, and you can then see the error appearing in our AppVeyor logs for all PRs and branches after this time. One can test with the previous SHAopam init git+https://github.com/ocaml/opam-repository.git#3303f6
and opam built at that time. However, depexts integration #3975 then cemented the issue further - there have been several packages (correctly) usingos-version
indepext
so in fact you can only get the pre 26 Sep 2019 behaviour by runningopam init git+https://github.com/ocaml/opam-repository.git#3303f6 --no-depexts
!OPAM_REPO_SHA
defined for both Travis and AppVeyor and allopam init
instructions now use the opam-repository Git repo. The only downside is that it means Travis and AppVeyor no longer test the HTML backend - if this is a concern, then I guess we could mirror opam-repository in order to have anindex.tar.gz
at our own SHA?Therefore:
Is entirely expected. The execute permission cannot be determined from
stat
, therefore, but only by checking the ACL.--with-libacl
which requires libacl-like support rather than--enable-acl
, which would be the more complete "do whatever is necessary to get ACL support on this platform" (the horrific detail of how you get a cross-platform version of the so-niche-that-they-didn't-include-it-in-the-draft functionacl_get_perm
can be seen in libarchive's sources). On Cygwin,libacl
support is required (i.e.--with-libacl
is the default), on all other platforms it must be explicitly selected. You can explicitly disable libacl support on Cygwin with--without-libacl
but your opam won't work properly. The support appears to work OK on Ubuntu (where it correctly detects the need for-lacl
) and Cygwin (where it correctly detects that no linker options are required).The final entertainment is that while Cygwin's libacl works correctly on 64-bit, it turns out that there's a bug affecting two of the functions on 32-bit Cygwin! My fix to that bug has been merged upstream but, until Cygwin 3.1.7 is released, AppVeyor is switched to use a developer snapshot of the Cygwin DLL (as it happens, this fix is the only new thing in that snapshot), I'll keep an eye on Cygwin releases and update AppVeyor after the next Cygwin release. If used on Cygwin 3.1.6 and earlier, the ACL simply fails to see the execute permission and you get the "Permission denied" error, as before.