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 doesn't change environment on Cygwin and maybe Windows #6903

Closed
vicuna opened this Issue Jun 14, 2015 · 9 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Jun 14, 2015

Original bug ID: 6903
Reporter: adrien
Status: resolved (set by @xavierleroy on 2017-02-14T12:55:25Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.1
Target version: 4.03.1+dev
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: platform support (windows, cross-compilation, etc)
Related to: #4499 #7640
Monitored by: @dbuenzli

Bug description

The following reproduces the issue:

open Unix
let env = Array.concat [ [| "LAPIN=lapin" |] ; Unix.environment () ]
let () =
match fork () with
| 0 -> execvpe "env" [| "env" |] env
| x -> ()

Run inside cygwin and pipe the output to 'grep lapin'. You should see nothing (unless you already had that environment variable defined).

The code in otherlibs/unix/execvp.c looks fine but does nothing. However, Unix.execve works. Both bindings are very old: execvpe in 1996 and execve in 1995 (this one, untouched since then).

The code for Unix.execvpe plays with the environ variable rather than calling execvpe(); I don't know why and this dates back to 1996: "Renommage unix.h -> unixsupport.h Petites adaptations pour Win32." ( ac9f8e8 / rev 951 ). One possibility is that is execvpe wasn't available back then on Windows.

As for why this doesn't work, if I remember correctly, Windows has two environments: one for ansi, one for unicode and this would require getters and setters rather than direct access to the variable.

I'm attaching a patch. I've only tested it on Cygwin and it fixes the testcase above.

I've also been a bit surprised by the following in otherslibs/threads/unix.ml (indent mine):
let execve proc args env = do_exec (fun () -> _execve proc args env)
let execvp proc args = do_exec (fun () -> _execvp proc args)
let execvpe proc args = do_exec (fun () -> _execvpe proc args)

I guess that not specifying the last argument for execvpe works because of partial application but it's a bit surprising to see it written that way.

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 14, 2015

Comment author: @gasche

Could you also add the failing case to the testsuite?

(A new directory tests/lib-unix would seem fitting)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 15, 2015

Comment author: @alainfrisch

Windows has two environments

On a related note, see also #4499: libc functions don't see changes caused by native Windows calls (SetEnvironmentVariable).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Aug 1, 2015

Comment author: nbraud

Attached patch might cause build to fail on Mac OS X.
This may be related to #5693

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Aug 3, 2015

Comment author: adrien

It was actually me asking nbraud over IRC to comment here because I wasn't home and was worried I would forget.

I just checked and, from "man execvpe" on Linux:
CONFORMING TO
POSIX.1-2001, POSIX.1-2008.
The execvpe() function is a GNU extension.

This has just gotten me very very sad because I believe manipulating environ in a threaded environment is racy. Well, forking in a threaded environment is racy but it's a good idea troubles as much as possible.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Nov 28, 2015

Comment author: @xavierleroy

Right, execvpe() is not POSIX, just a GNU extension. Could you please revise your patch accordingly? At a minimum, it should be

#ifdef _GNU_SOURCE
// your new code using execvpe()
#else
// the old implementation that modifies "environ"
#endif

Alternatively, you could implement execvpe in terms of execve() (which is POSIX), using caml_search_exe_in_path (from the OCaml runtime system) to find the actual location of the executable.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 3, 2015

Comment author: adrien

NB: execvpe() is also available on Windows.

Musl-libc is MIT-licensed and has an implementation of execvpe on top of execve which can be seen at http://git.musl-libc.org/cgit/musl/tree/src/process/execvp.c .

Would it be OK to re-use that for platforms without execvpe() and to call execvpe() directly otherwise?

I don't know if I'll be able to do something myself before the next release and I don't want to "lock" the update of the patch to myself. In any case I'll do my best to review and test any patch on the topic.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 4, 2015

Comment author: @xavierleroy

No, it's not OK to reuse the Musl code, because 1- it would probably not work under Windows, and 2- the OCaml runtime already contains functions to search executables in PATH, see "caml/osdeps.h".

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 11, 2016

Comment author: adrien

Realistically I won't have time to push this forward in time for the next release: I'd like to write and test a couple patches to see and compare them but I'm too short on time for this during January and I think this issue can wait a bit more if needed.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Feb 14, 2017

Comment author: @xavierleroy

Fixed by commit [trunk 3627221], should be in 4.05.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.