fix(:restart): set job env instead of mutating parent env #40308

Problem:
- Transient mutation of the parent env is visible to any concurrent
  code. Or at least just kinda sloppy.
- Latent bug:`channel_job_start` queues the spawn and returns before
  `uv_spawn` runs, so the prior `os_unsetenv` immediately after the call
  could in principle race with the deferred spawn.

Solution:
Pass `env` to the channel.

src/nvim/channel.c

@@ -558,9 +558,9 @@ uint64_t channel_from_stdio(bool rpc, CallbackReader on_output, const char **err
     os_set_cloexec(stdout_dup_fd);
     // :restart spawns a replacement server that must not borrow the parent
     // Nvim process console, because that parent process will soon exit.
-    const bool restart_alloc_console = os_env_exists("__NVIM_RESTART_ALLOC_CONSOLE", true);
+    const bool restart_alloc_console = os_env_exists(ENV_RESTART_ALLOC_CONSOLE, true);
     if (restart_alloc_console) {
-      os_unsetenv("__NVIM_RESTART_ALLOC_CONSOLE");
+      os_unsetenv(ENV_RESTART_ALLOC_CONSOLE);
     }
     if (!GetConsoleWindow()) {
       // Borrow the parent's console so CONOUT$ resolves to the real terminal,

src/nvim/eval/funcs.c

@@ -3376,8 +3376,11 @@ static const char *required_env_vars[] = {
   NULL
 };
 
+/// Builds an environment dict for a child process (job).
+///
+/// @param set_nvim_addr  Set the $NVIM env var.
 dict_T *create_environment(const dictitem_T *job_env, const bool clear_env, const bool pty,
-                           const char * const pty_term_name)
+                           const bool set_nvim_addr, const char * const pty_term_name)
 {
   dict_T *env = tv_dict_alloc();
 
@@ -3431,13 +3434,15 @@ dict_T *create_environment(const dictitem_T *job_env, const bool clear_env, cons
   }
 
   // Set $NVIM (in the child process) to v:servername. #3118
-  char *nvim_addr = get_vim_var_str(VV_SEND_SERVER);
-  if (nvim_addr[0] != NUL) {
-    dictitem_T *dv = tv_dict_find(env, S_LEN("NVIM"));
-    if (dv) {
-      tv_dict_item_remove(env, dv);
+  if (set_nvim_addr) {
+    char *nvim_addr = get_vim_var_str(VV_SEND_SERVER);
+    if (nvim_addr[0] != NUL) {
+      dictitem_T *dv = tv_dict_find(env, S_LEN("NVIM"));
+      if (dv) {
+        tv_dict_item_remove(env, dv);
+      }
+      tv_dict_add_str(env, S_LEN("NVIM"), nvim_addr);
     }
-    tv_dict_add_str(env, S_LEN("NVIM"), nvim_addr);
   }
 
   if (job_env) {
@@ -3626,7 +3631,7 @@ void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
     term_name = term_name ? term_name : "ansi";
   }
 
-  dict_T *env = create_environment(job_env, clear_env, pty, term_name);
+  dict_T *env = create_environment(job_env, clear_env, pty, true, term_name);
   Channel *chan = channel_job_start(argv, NULL, on_stdout, on_stderr, on_exit, pty,
                                     rpc, overlapped, detach, stdin_mode, cwd,
                                     width, height, env, &rettv->vval.v_number);

src/nvim/ex_docmd.c

@@ -38,6 +38,7 @@
 #include "nvim/edit.h"
 #include "nvim/errors.h"
 #include "nvim/eval/fs.h"
+#include "nvim/eval/funcs.h"
 #include "nvim/eval/typval.h"
 #include "nvim/eval/typval_defs.h"
 #include "nvim/eval/userfunc.h"
@@ -5065,16 +5066,11 @@ static void ex_restart(exarg_T *eap)
   bool server_stopped = listen_arg ? server_stop(listen_arg, true) : false;
 #endif
 
+  dict_T *env = create_environment(NULL, false, false, false, NULL);
+  tv_dict_add_str(env, S_LEN(ENV_STARTREASON), "restart");
 #ifdef MSWIN
-  bool restart_alloc_console_env = false;
-  if (os_setenv("__NVIM_RESTART_ALLOC_CONSOLE", "1", 1) == 0) {
-    restart_alloc_console_env = true;
-  }
+  tv_dict_add_str(env, S_LEN(ENV_RESTART_ALLOC_CONSOLE), "1");
 #endif
-  bool startreason_env = false;
-  if (os_setenv(ENV_STARTREASON, "restart", 1) == 0) {
-    startreason_env = true;
-  }
 
   CallbackReader on_err = CALLBACK_READER_INIT;
 #ifdef MSWIN
@@ -5090,15 +5086,7 @@ static void ex_restart(exarg_T *eap)
   Channel *channel = channel_job_start(argv, exepath,
                                        CALLBACK_READER_INIT, on_err, CALLBACK_NONE,
                                        false, true, true, detach, kChannelStdinPipe,
-                                       NULL, 0, 0, NULL, &exit_status);
-  if (startreason_env) {
-    os_unsetenv(ENV_STARTREASON);
-  }
-#ifdef MSWIN
-  if (restart_alloc_console_env) {
-    os_unsetenv("__NVIM_RESTART_ALLOC_CONSOLE");
-  }
-#endif
+                                       NULL, 0, 0, env, &exit_status);
   if (!channel) {
     emsg("cannot create a channel job");
     goto fail_1;

src/nvim/log.c

@@ -73,7 +73,7 @@ static void log_path_init(void)
       || !log_try_create(log_file_path)) {
     if (user_set) {  // User-provided $NVIM_LOG_FILE.
       // Used by _core/log.lua:check_log_file to validate logfile on startup.
-      os_setenv("__NVIM_LOG_FILE_WANT", log_file_path, true);
+      os_setenv(ENV_LOGFILE_WANT, log_file_path, true);
     }
     // Make $XDG_STATE_HOME/logs if it does not exist.
     char *loghome = concat_fnames_realloc(get_xdg_home(kXDGStateHome), "logs", true);
@@ -91,7 +91,7 @@ static void log_path_init(void)
     if (len >= size || !log_try_create(log_file_path)) {
       if (!user_set) {  // Default fallback path.
         // Used by _core/log.lua:check_log_file to validate logfile on startup.
-        os_setenv("__NVIM_LOG_FILE_WANT", log_file_path, true);
+        os_setenv(ENV_LOGFILE_WANT, log_file_path, true);
       }
       len = xstrlcpy(log_file_path, "nvim.log", size);
     }

src/nvim/msgpack_rpc/server.c

@@ -54,7 +54,7 @@ bool server_init(const char *listen_addr)
   int rv = server_start(listen_addr);
 
   // TODO(justinmk): this is for log_spec. Can remove this after nvim_log #7062 is merged.
-  if (os_env_exists("__NVIM_TEST_LOG", false)) {
+  if (os_env_exists(ENV_TEST_LOG, false)) {
     ELOG("test log message");
   }
 

src/nvim/os/os.h

@@ -25,5 +25,8 @@ extern char *default_lib_dir;
 // IWYU pragma: end_exports
 
 #define ENV_LOGFILE "NVIM_LOG_FILE"
+#define ENV_LOGFILE_WANT "__NVIM_LOG_FILE_WANT"
 #define ENV_NVIM "NVIM"
+#define ENV_RESTART_ALLOC_CONSOLE "__NVIM_RESTART_ALLOC_CONSOLE"
 #define ENV_STARTREASON "__NVIM_STARTREASON"
+#define ENV_TEST_LOG "__NVIM_TEST_LOG"

src/nvim/ui_client.c

@@ -159,7 +159,7 @@ void ui_client_run(void)
   ui_client_attach(tui_width, tui_height, tui_term, tui_rgb);
 
   // TODO(justinmk): this is for log_spec. Can remove this after nvim_log #7062 is merged.
-  if (os_env_exists("__NVIM_TEST_LOG", true)) {
+  if (os_env_exists(ENV_TEST_LOG, true)) {
     ELOG("test log message");
   }