Permalink
Browse files

augment do not replace environment when calling

Summary:
This is one potential solution to guard ourselves against future environment
variable issues.

Augmenting the environment, potentially shadowing whatever was underneath is
probably the behavior that most programmers expect when passing in environment
variables.

However, there are a few problems with doing this

1) It's not how exec* works
2) It's not possible to explicitly remove things from the environment anymore
3) naming the parameter "env" does not make it obvious whether we're dealing with
an augmentation or replacement

Potential other solutions:

1) remove the ability to set environment variables in the process api. The alternative
would be setting things like HGBLAME in the master process. That way all the processes
always have the same environment
2) make the API explicit about whether you want to replace the environment or "upsert"
variables

Reviewed By: dabek

Differential Revision: D7406487

fbshipit-source-id: 5dc90796730c8db7fffb0239d425c040ac7993d7
  • Loading branch information...
Greg Nisbet authored and hhvm-bot committed Apr 3, 2018
1 parent 7de96ed commit f99ed331c556f442ae4f27c728378e00bf92cbc5
@@ -249,8 +249,11 @@ let exec_no_chdir prog ?input ?env args =
| None ->
Unix.create_process prog args stdin_child stdout_child stderr_child
| Some env ->
(* deduping the env is not necessary. glibc putenv/getenv will grab the first
* one *)
let fullenv = Array.append (Array.of_list env) (Unix.environment ()) in
Unix.create_process_env prog args
(Array.of_list env) stdin_child stdout_child stderr_child
fullenv stdin_child stdout_child stderr_child
in
Unix.close stdin_child;
Unix.close stdout_child;
@@ -294,8 +297,8 @@ let exec_ ~cwd ~prog ~(input:string option) ~env ~args =
| Some cwd ->
run_entry ?input chdir_entry (cwd, prog, env, args)
let exec ?cwd prog ?input args =
exec_ ~cwd ~prog ~input ~env:None ~args
let exec ?cwd prog ?input ?env args =
exec_ ~cwd ~prog ~input ~env ~args
let exec_with_replacement_env ?cwd prog ~env ?input args =
exec_ ~cwd ~prog ~input ~env:(Some env) ~args
@@ -24,7 +24,7 @@ end
* Sets its current working directory if given.
* Sends input to stdin of spawned process if given.
*)
val exec : ?cwd:string -> string -> ?input:string ->
val exec : ?cwd:string -> string -> ?input:string -> ?env:string list ->
string list -> Process_types.t
(* spawns a process just like exec, EXCEPT that the environment is REPLACED instead
@@ -0,0 +1,50 @@
(**
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the "hack" directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
*)
open Process
(* check that B is not shadowed when we set A.
*
* set B=1 outside and then run:
*
* case "$B" in
* 1) exit 0;;
* * ) exit 1;;
* esac
*
* except with * and ) next to each other
*
* *)
let test_proc_env () =
let () = Unix.putenv "B" "1" in
let proc_t = exec "bash" ~env:["A=1"] ["-c"; "case \"$B\" in 1) exit 0;; *) exit 1;; esac"] in
let proc_stat_ref = proc_t.Process_types.process_status in
let proc_stat = !proc_stat_ref in
match proc_stat with
| Process_types.Process_aborted _ -> failwith "process aborted"
| Process_types.Process_running pid -> begin
match Unix.waitpid [] pid with
| caught_pid, Unix.WEXITED(status) -> begin
match caught_pid = pid, status with
| false, _ -> failwith "impossible: caught wrong child process"
| true, 0 -> true (* success! *)
| true, _ -> failwith "child exited abnormally"
end
| _ -> failwith "child was killed by signal or something"
end
| _ -> failwith "process failed for a different reason"
let tests = [
"test_proc_env", test_proc_env;
]
let () =
Unit_test.run_all tests

0 comments on commit f99ed33

Please sign in to comment.