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
Comments
Comment author: @gasche Could you also add the failing case to the testsuite? (A new directory tests/lib-unix would seem fitting) |
Comment author: @alainfrisch
On a related note, see also #4499: libc functions don't see changes caused by native Windows calls (SetEnvironmentVariable). |
Comment author: nbraud Attached patch might cause build to fail on Mac OS X. |
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: 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. |
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 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. |
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. |
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". |
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. |
Comment author: @xavierleroy Fixed by commit [trunk 3627221], should be in 4.05. |
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
The text was updated successfully, but these errors were encountered: