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

Unix.execvpe issues since 4.05.0 #7640

Closed
vicuna opened this issue Sep 26, 2017 · 14 comments
Closed

Unix.execvpe issues since 4.05.0 #7640

vicuna opened this issue Sep 26, 2017 · 14 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Sep 26, 2017

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 in env.

Ref: 36272215ef

Steps to reproduce

From the shell:

$ mkdir -p /tmp/foo
$ ln -s /bin/true /tmp/foo/frobz
$ ocaml <(echo '#use "topfind";; #require "unix";; Unix.execvpe "frobz" [||] [|"PATH=/tmp/foo"|];;')

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

@vicuna
Copy link
Author

vicuna commented Sep 26, 2017

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.

@vicuna
Copy link
Author

vicuna commented Sep 26, 2017

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
" execvpe() operates identically to execve(), with one exception: If
'pathname' is not found as described in execve(), then execvpe() will
use the DOS PATH to continue the search for pathname. With that
single exception, the two functions are identical; see execve() for a
complete description."

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.

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

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:

  1. a new race condition where the executable can be removed between path resolution and [execve]
  2. non-executable files in PATH will now be considered for execution (which will fail)
  3. shebang-less scripts will no longer work on linux

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).

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

Comment author: aalekseyev

To avoid misleading anyone: behavior changes don't stop at the list I gave. There is also at least:

  • failure to "stat" a file is now treated differently (this change seems bad)
  • default value of PATH works differently
    ...

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

Comment author: aalekseyev

Ouch, and maybe the worst one: this change implicitly adds current working directory as the last component in PATH.

cp which true ./frobz
PATH=$(dirname "$(which ocaml)") ocaml unix.cma <(printf "%s" 'Unix.execvpe "frobz" [||] [|"PATH=/usr/bin"|];;')

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

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.

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

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:

   If  the  header  of  a  file isn't recognized (the attempted execve(2) failed with the error ENOEXEC), these
   functions will execute the shell (/bin/sh) with the path of the  file  as  its  first  argument.   (If  this
   attempt fails, no further searching is done.)

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.

@vicuna
Copy link
Author

vicuna commented Sep 27, 2017

Comment author: aalekseyev

By "normally" I mean glibc and musl because I'm not familiar with other implementations.

@vicuna
Copy link
Author

vicuna commented Sep 28, 2017

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...

@vicuna
Copy link
Author

vicuna commented Sep 28, 2017

Comment author: aalekseyev

I can't see a contradiction: "the actions of the shell" is vague enough. Maybe shell just calls execvp. :)

@vicuna
Copy link
Author

vicuna commented Sep 28, 2017

Comment author: aalekseyev

@avsm, I think you might be mixing together the two distinct error cases:

  • the binary is not marked as executable (EACCESS)
  • the binary is marked as executable, but does not have the correct file format (#! at the top of the file or ELF header etc) (ENOEXEC)

Not that it leads you to any incorrect conclusions.

@vicuna
Copy link
Author

vicuna commented Sep 28, 2017

Comment author: aalekseyev

Sorry if pedantic, but this is not quite right:

it through to exec "sh" (not "/bin/sh")
This can't be true because "sh" will usually not exist in the current directory. Indeed if you look closer it calls the binary at _PATH_BSHELL, which is ultimately defined like so:

#define _PATH_BSHELL "/bin/sh"

"sh" is just the value of argv[0] it passes to /bin/sh.

@vicuna
Copy link
Author

vicuna commented Oct 7, 2017

Comment author: @xavierleroy

Proposed reimplementation of Unix.execvpe here: #1414
Please continue the discussion there.

@vicuna
Copy link
Author

vicuna commented Oct 11, 2017

Comment author: @xavierleroy

Reimplemented in 4.06.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants