Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007640OCamlotherlibspublic2017-09-26 11:072017-10-11 10:55
ReporterAltGr 
Assigned Toxleroy 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.05.0 
Target Version4.06.0 +dev/beta1/beta2/rc1Fixed in Version4.06.0 +dev/beta1/beta2/rc1 
Summary0007640: Unix.execvpe issues since 4.05.0
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 36272215ef 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: https://github.com/ocaml/ocaml/commit/36272215ef [^]
Steps To ReproduceFrom 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 InformationThis 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 https://github.com/OCamlPro/ocp-indent/issues/256 [^]
TagsNo tags attached.
Attached Files

- Relationships
related to 0006903resolved Unix.execvpe doesn't change environment on Cygwin and maybe Windows 

-  Notes
(0018341)
xleroy (administrator)
2017-09-26 14:17

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.
(0018348)
avsm (developer)
2017-09-26 16:24

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.
(0018354)
aalekseyev (reporter)
2017-09-27 12:47

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).
(0018357)
aalekseyev (reporter)
2017-09-27 14:09

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
...
(0018362)
aalekseyev (reporter)
2017-09-27 16:17

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"|];;')
(0018365)
xleroy (administrator)
2017-09-27 16:27

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.
(0018368)
aalekseyev (reporter)
2017-09-27 16:45

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.
(0018370)
aalekseyev (reporter)
2017-09-27 16:49

By "normally" I mean glibc and musl because I'm not familiar with other implementations.
(0018386)
avsm (developer)
2017-09-28 15:02

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...
(0018387)
aalekseyev (reporter)
2017-09-28 15:11

I can't see a contradiction: "the actions of the shell" is vague enough. Maybe shell just calls execvp. :)
(0018388)
aalekseyev (reporter)
2017-09-28 15:18

@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.
(0018389)
aalekseyev (reporter)
2017-09-28 15:24

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.
(0018500)
xleroy (administrator)
2017-10-07 15:15

Proposed reimplementation of Unix.execvpe here: https://github.com/ocaml/ocaml/pull/1414 [^]
Please continue the discussion there.
(0018540)
xleroy (administrator)
2017-10-11 10:55

Reimplemented in 4.06.

- Issue History
Date Modified Username Field Change
2017-09-26 11:07 AltGr New Issue
2017-09-26 11:11 frisch Description Updated View Revisions
2017-09-26 11:11 frisch Relationship added related to 0006903
2017-09-26 14:17 xleroy Note Added: 0018341
2017-09-26 14:17 xleroy Status new => acknowledged
2017-09-26 16:24 avsm Note Added: 0018348
2017-09-27 12:47 aalekseyev Note Added: 0018354
2017-09-27 14:09 aalekseyev Note Added: 0018357
2017-09-27 16:17 aalekseyev Note Added: 0018362
2017-09-27 16:27 xleroy Note Added: 0018365
2017-09-27 16:45 aalekseyev Note Added: 0018368
2017-09-27 16:49 aalekseyev Note Added: 0018370
2017-09-28 15:02 avsm Note Added: 0018386
2017-09-28 15:11 aalekseyev Note Added: 0018387
2017-09-28 15:18 aalekseyev Note Added: 0018388
2017-09-28 15:24 aalekseyev Note Added: 0018389
2017-10-02 14:05 doligez Target Version => 4.06.0 +dev/beta1/beta2/rc1
2017-10-07 15:15 xleroy Note Added: 0018500
2017-10-07 15:16 xleroy Summary Unix.execvpe doesn't follow PATH updates since 4.05.0 => Unix.execvpe issues since 4.05.0
2017-10-07 15:16 xleroy Description Updated View Revisions
2017-10-07 15:17 xleroy Assigned To => xleroy
2017-10-07 15:17 xleroy Status acknowledged => confirmed
2017-10-11 10:55 xleroy Note Added: 0018540
2017-10-11 10:55 xleroy Status confirmed => resolved
2017-10-11 10:55 xleroy Resolution open => fixed
2017-10-11 10:55 xleroy Fixed in Version => 4.06.0 +dev/beta1/beta2/rc1


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker