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
Unix.execvpe issues since 4.05.0 #7640
Comments
Comment author: @xavierleroy Thanks for this interesting observation. On first read it looked like a stupid programming mistake, and I can say that with confidence as the author of the 4.05 implementation of Unix.execvpe. On second thoughts, the behavior observed in 4.04 and before (search in the PATH variable of the "new" environment) is an artefact of the way Unix.execvpe was implemented up to 4.04 (change global environment then do execvp()). Looking at manual pages for the (non-standard) execvpe() C library function, most don't say in which environment PATH is looked up, but at least one says explicitly that it is in the "old" environment of the process executing execvpe(), not the "new" environment passed to the executed program. My feeling is that the 4.05 behavior makes more sense than the 4.04 behavior, and that it would be better to fix OPAM rather than revert to the 4.04 behavior. Discussion welcome. |
Comment author: @avsm The 4.05 behaviour makes more sense to me given the following interpretation: the only difference between execve and execvpe is that if the filename is not found as an absolute path, then execvpe will duplicate the efforts of the shell to locate a suitable binary to execute. Only after that happens does the new environment and arguments have any use (for spawning the calling process). This is agreed to by the venerable Turbo C documentation (!) of execvpe, which seems to be independent of the GNU implementation. http://www.ousob.com/ng/turboc/ng1b67e.php The code example given there explicitly overrides PATH and only uses it for the child process, preferring the DOS PATH for the executable search path. |
Comment author: aalekseyev In [core] we have yet another implementation of execvpe semantics in the form of [Core.Unix.create_process_env], and I'm looking at this issue as a maintainer of that. In our API we additionally let the caller specify ~working_dir, which makes the issue more delicate: you can't (easily/portably) interpret the binary path relative to the "old" working_dir, and I think it's natural for the user to expect working_dir and environment to be changed "at the same time". That said, both musl and glibc seem to use the "old" environment, and it just seems like a better behavior for [execvpe], so we'll probably have to sacrifice that property. Looking at the other changes caused by commit [3627221], I notice:
I don't know which of these are deliberate. I think (2) and (3) are good if you designed the function from scratch, but not worth breaking programs for, and (1) is just undesirable (while simplifying implementation). |
Comment author: aalekseyev To avoid misleading anyone: behavior changes don't stop at the list I gave. There is also at least:
|
Comment author: aalekseyev Ouch, and maybe the worst one: this change implicitly adds current working directory as the last component in PATH. cp |
Comment author: @xavierleroy The race condition (1) is in all the exec*p functions, I believe. (They are all implemented by searching in the path, then calling into execve.) (2) is debatable, but if we really want to we can skip nonexecutable files. I don't understand (3), could you elaborate? (4) failure to "stat": could you elaborate? (5) default value of PATH: fair point, could be improved. (6) CWD as last component of PATH: to be improved. |
Comment author: aalekseyev re (1): execvp normally works by going through the list of candidate paths and execve'ing each of them in turn, stopping as soon as the first execve succeeds (or something bad happens). This means the check of whether the file exists is only done once and not twice, which squashes this race condition. Granted there are other, more suble ones (e.g. renaming the file). By (3) I refer to this bit from [execvp] man page:
I think nothing like this is happening with 4.05, so executable scripts that lack #! header line will stop working. By (4) I mean that if stat failed for any reason other than ENOENT/EACCESS, 4.04 would stop there and complain, but 4.05 will continue to the next candidate. |
Comment author: aalekseyev By "normally" I mean glibc and musl because I'm not familiar with other implementations. |
Comment author: @avsm Looking at the OpenBSD libc implementation of execvpe clarified some of the semantics for me (https://github.com/openbsd/src/blob/master/lib/libc/gen/exec.c#L131) execvpe iterates through the PATH attempting execs until one succeeds. However, it doesnt check if the file is executable or not, and instead passes it through to exec "sh" (not "/bin/sh") if the file is not executable due to an ENOEXEC return value. If that subshell fails, then the command terminates without continuing through the rest of the PATH. This somewhat contradicts the Linux man page, which says: "The execlp(), execvp(), and execvpe() functions duplicate the actions of the shell in searching for an executable file if the specified filename does not contain a slash (/) character." So there is no 'stat' of each file matching a PATH entry that checks if it's marked as an executable binary before attempting to call it. I guess any such check would add to the races inherent in this library call... |
Comment author: aalekseyev I can't see a contradiction: "the actions of the shell" is vague enough. Maybe shell just calls execvp. :) |
Comment author: aalekseyev @avsm, I think you might be mixing together the two distinct error cases:
Not that it leads you to any incorrect conclusions. |
Comment author: aalekseyev Sorry if pedantic, but this is not quite right:
#define _PATH_BSHELL "/bin/sh" "sh" is just the value of argv[0] it passes to /bin/sh. |
Comment author: @xavierleroy Proposed reimplementation of Unix.execvpe here: #1414 |
Comment author: @xavierleroy Reimplemented in 4.06. |
Original bug ID: 7640
Reporter: AltGr
Assigned to: @xavierleroy
Status: resolved (set by @xavierleroy on 2017-10-11T08:55:29Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.05.0
Target version: 4.06.0 +dev/beta1/beta2/rc1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: otherlibs
Related to: #6903
Monitored by: @gasche @dbuenzli
Bug description
[ Original summary was: Unix.execvpe doesn't follow PATH updates since 4.05.0 ]
execvpe
searches the specified program in the PATH, and is given an environment. The usual behaviour -- or at least the behaviour on Linux until 4.04.2 -- is to lookup in PATH as defined in the new environment.Patch 3627221 fixes a cygwin issue by avoiding
execvp
, and does the lookup in PATH manually, but based on the former value of PATH, not the new one specified inenv
.Ref: 36272215ef
Steps to reproduce
From the shell:
Works on 4.04.2 and before, breaks on 4.05.0
Additional information
This typically trips opam when it creates a new switch, then attempts to run binaries within there. See for example https://travis-ci.org/ocaml/opam/jobs/279454219#L6216 or OCamlPro/ocp-indent#256
The text was updated successfully, but these errors were encountered: