From 7f50c692685c92d47e6ba6c3ee5f6fdccb78fec5 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Fri, 31 Jul 2020 01:17:24 -0400 Subject: Use dict_T to pass env vars to process spawning code Co-authored-by: Matthieu Coudron --- src/nvim/os/pty_process_unix.c | 79 +++++++++++++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 21 deletions(-) (limited to 'src/nvim/os') diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index 4d7d9a45df..a3c9e726c5 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -20,6 +20,10 @@ # include #endif +#ifdef __APPLE__ +# include +#endif + #include #include "nvim/lib/klist.h" @@ -151,31 +155,24 @@ void pty_process_teardown(Loop *loop) uv_signal_stop(&loop->children_watcher); } +static const char *ignored_env_vars[] = { + "COLUMNS", + "LINES", + "TERMCAP", + "COLORFGBG" +}; + static void init_child(PtyProcess *ptyproc) FUNC_ATTR_NONNULL_ALL { +#if defined(HAVE__NSGETENVIRON) + char **environ = *_NSGetEnviron(); +#else + extern char **environ; +#endif // New session/process-group. #6530 setsid(); - os_unsetenv("COLUMNS"); - os_unsetenv("LINES"); - os_unsetenv("TERMCAP"); - os_unsetenv("COLORFGBG"); - // setting COLORTERM to "truecolor" if termguicolors is set and 256 - // otherwise, but only if it was set in the parent terminal at all - if (os_env_exists("COLORTERM")) { - const char *colorterm = os_getenv("COLORTERM"); - if (colorterm != NULL) { - if (p_tgc) { - os_setenv("COLORTERM", "truecolor", 1); - } else { - os_setenv("COLORTERM", "256", 1); - } - } else { - os_unsetenv("COLORTERM"); - } - } - signal(SIGCHLD, SIG_DFL); signal(SIGHUP, SIG_DFL); signal(SIGINT, SIG_DFL); @@ -190,9 +187,49 @@ static void init_child(PtyProcess *ptyproc) } char *prog = ptyproc->process.argv[0]; - os_setenv("TERM", ptyproc->term_name ? ptyproc->term_name : "ansi", 1); - execvp(prog, ptyproc->process.argv); + if (proc->env) { + for (size_t i = 0; i < ARRAY_SIZE(ignored_env_vars); i++) { + dictitem_T *dv = tv_dict_find(proc->env, ignored_env_vars[i], -1); + if (dv) { + tv_dict_item_remove(proc->env, dv); + } + } + tv_dict_add_str(proc->env, S_LEN("TERM"), ptyproc->term_name ? ptyproc->term_name : "ansi"); + + // setting COLORTERM to "truecolor" if termguicolors is set and 256 + // otherwise, but only if it was set in the parent terminal at all + dictitem_T *dv = tv_dict_find(proc->env, S_LEN("COLORTERM")); + if (dv) { + tv_dict_item_remove(proc->env, dv); + tv_dict_add_str(proc->env, S_LEN("COLORTERM"), p_tgc ? "truecolor" : "256"); + } + + environ = tv_dict_to_env(proc->env); + } else { + for (size_t i = 0; i < ARRAY_SIZE(ignored_env_vars); i++) { + os_unsetenv(ignored_env_vars[i]); + } + + // setting COLORTERM to "truecolor" if termguicolors is set and 256 + // otherwise, but only if it was set in the parent terminal at all + if (os_env_exists("COLORTERM")) { + const char *colorterm = os_getenv("COLORTERM"); + if (colorterm != NULL) { + if (p_tgc) { + os_setenv("COLORTERM", "truecolor", 1); + } else { + os_setenv("COLORTERM", "256", 1); + } + } else { + os_unsetenv("COLORTERM"); + } + } + + os_setenv("TERM", ptyproc->term_name ? ptyproc->term_name : "ansi", 1); + } + execvp(prog, proc->argv); ELOG("execvp failed: %s: %s", strerror(errno), prog); + _exit(122); // 122 is EXEC_FAILED in the Vim source. } -- cgit From 8eec9c7d5b398918609d8edfed3928e873fa646f Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 10 Sep 2020 07:15:32 -0400 Subject: Common handling of required/ignored env vars When starting a pty job, there are certain env vars that we need to either add or remove. Currently, there are two relevant scenarios. * Removing irrelevant env vars on Unix, mostly related to the terminal hosting nvim since they do not apply to a libvterm-hosted terminal. * Adding required env vars for Windows jobs. --- src/nvim/os/pty_process_unix.c | 50 ++++-------------------------------------- 1 file changed, 4 insertions(+), 46 deletions(-) (limited to 'src/nvim/os') diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index a3c9e726c5..0733589870 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -155,13 +155,6 @@ void pty_process_teardown(Loop *loop) uv_signal_stop(&loop->children_watcher); } -static const char *ignored_env_vars[] = { - "COLUMNS", - "LINES", - "TERMCAP", - "COLORFGBG" -}; - static void init_child(PtyProcess *ptyproc) FUNC_ATTR_NONNULL_ALL { @@ -187,46 +180,11 @@ static void init_child(PtyProcess *ptyproc) } char *prog = ptyproc->process.argv[0]; - if (proc->env) { - for (size_t i = 0; i < ARRAY_SIZE(ignored_env_vars); i++) { - dictitem_T *dv = tv_dict_find(proc->env, ignored_env_vars[i], -1); - if (dv) { - tv_dict_item_remove(proc->env, dv); - } - } - tv_dict_add_str(proc->env, S_LEN("TERM"), ptyproc->term_name ? ptyproc->term_name : "ansi"); - - // setting COLORTERM to "truecolor" if termguicolors is set and 256 - // otherwise, but only if it was set in the parent terminal at all - dictitem_T *dv = tv_dict_find(proc->env, S_LEN("COLORTERM")); - if (dv) { - tv_dict_item_remove(proc->env, dv); - tv_dict_add_str(proc->env, S_LEN("COLORTERM"), p_tgc ? "truecolor" : "256"); - } - - environ = tv_dict_to_env(proc->env); - } else { - for (size_t i = 0; i < ARRAY_SIZE(ignored_env_vars); i++) { - os_unsetenv(ignored_env_vars[i]); - } - - // setting COLORTERM to "truecolor" if termguicolors is set and 256 - // otherwise, but only if it was set in the parent terminal at all - if (os_env_exists("COLORTERM")) { - const char *colorterm = os_getenv("COLORTERM"); - if (colorterm != NULL) { - if (p_tgc) { - os_setenv("COLORTERM", "truecolor", 1); - } else { - os_setenv("COLORTERM", "256", 1); - } - } else { - os_unsetenv("COLORTERM"); - } - } - os_setenv("TERM", ptyproc->term_name ? ptyproc->term_name : "ansi", 1); - } + assert(proc->env); + tv_dict_add_str(proc->env, S_LEN("TERM"), + ptyproc->term_name ? ptyproc->term_name : "ansi"); + environ = tv_dict_to_env(proc->env); execvp(prog, proc->argv); ELOG("execvp failed: %s: %s", strerror(errno), prog); -- cgit From a199363be246dc097b74abfe2703c5058aa8ad45 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Mon, 28 Dec 2020 20:40:07 -0500 Subject: Pass environment on to pty processes on Windows vim-patch:8.2.0239: MS-Windows: 'env' job option does not override existing vars Problem: MS-Windows: 'env' job option does not override existing environment variables. (Tim Pope) Solution: Set the environment variables later. (Yasuhiro Matsumoto, closes vim/vim#5485, closes vim/vim#5608) https://github.com/vim/vim/commit/355757aed6ae2ae5446882570d89f243e4805937 Co-authored-by: erw7 --- src/nvim/os/pty_process_win.c | 78 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 2 deletions(-) (limited to 'src/nvim/os') diff --git a/src/nvim/os/pty_process_win.c b/src/nvim/os/pty_process_win.c index 6f7100e846..52d2f84ace 100644 --- a/src/nvim/os/pty_process_win.c +++ b/src/nvim/os/pty_process_win.c @@ -52,6 +52,7 @@ int pty_process_spawn(PtyProcess *ptyproc) uv_connect_t *out_req = NULL; wchar_t *cmd_line = NULL; wchar_t *cwd = NULL; + wchar_t *env = NULL; const char *emsg = NULL; assert(proc->err.closed); @@ -124,13 +125,22 @@ int pty_process_spawn(PtyProcess *ptyproc) goto cleanup; } + if (proc->env != NULL) { + status = build_env_block(proc->env, &env); + } + + if (status != 0) { + emsg = "build_env_block failed"; + goto cleanup; + } + if (ptyproc->type == kConpty) { if (!os_conpty_spawn(conpty_object, &process_handle, NULL, cmd_line, cwd, - NULL)) { + env)) { emsg = "os_conpty_spawn failed"; status = (int)GetLastError(); goto cleanup; @@ -141,7 +151,7 @@ int pty_process_spawn(PtyProcess *ptyproc) NULL, // Optional application name cmd_line, cwd, - NULL, // Optional environment variables + env, &err); if (spawncfg == NULL) { emsg = "winpty_spawn_config_new failed"; @@ -213,6 +223,7 @@ cleanup: xfree(in_req); xfree(out_req); xfree(cmd_line); + xfree(env); xfree(cwd); return status; } @@ -454,3 +465,66 @@ int translate_winpty_error(int winpty_errno) default: return UV_UNKNOWN; } } + +typedef struct EnvNode { + wchar_t *str; + size_t len; + QUEUE node; +} EnvNode; + +/// Build the environment block to pass to CreateProcessW. +/// +/// @param[in] denv Dict of environment name/value pairs +/// @param[out] env Allocated environment block +/// +/// @returns zero on success or error code of MultiByteToWideChar function. +static int build_env_block(dict_T *denv, wchar_t **env_block) +{ + const size_t denv_size = (size_t)tv_dict_len(denv); + size_t env_block_len = 0; + int rc; + char **env = tv_dict_to_env(denv); + + QUEUE *q; + QUEUE env_q; + QUEUE_INIT(&env_q); + // Convert env vars to wchar_t and calculate how big the final env block + // needs to be + for (size_t i = 0; i < denv_size; i++) { + EnvNode *env_node = xmalloc(sizeof(*env_node)); + rc = utf8_to_utf16(env[i], -1, &env_node->str); + if (rc != 0) { + goto cleanup; + } + env_node->len = wcslen(env_node->str) + 1; + env_block_len += env_node->len; + QUEUE_INSERT_TAIL(&env_q, &env_node->node); + } + + // Additional '\0' after the final entry + env_block_len++; + + *env_block = xmalloc(sizeof(**env_block) * env_block_len); + wchar_t *pos = *env_block; + + QUEUE_FOREACH(q, &env_q) { + EnvNode *env_node = QUEUE_DATA(q, EnvNode, node); + memcpy(pos, env_node->str, env_node->len * sizeof(*pos)); + pos += env_node->len; + } + + *pos = L'\0'; + +cleanup: + q = QUEUE_HEAD(&env_q); + while (q != &env_q) { + QUEUE *next = q->next; + EnvNode *env_node = QUEUE_DATA(q, EnvNode, node); + XFREE_CLEAR(env_node->str); + QUEUE_REMOVE(q); + xfree(env_node); + q = next; + } + + return rc; +} -- cgit From 035ee868ae2d9cbbf2a290ca3412946fade20833 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Sun, 31 Jan 2021 07:49:31 -0500 Subject: fix(pty_proc/macOS): Properly set the environment for the child Binding _NSGetEnviron()'s return value to a local variable and then re-binding that is incorrect. We need to directly update what _NSGetEnviron() refers to. --- src/nvim/os/pty_process_unix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/nvim/os') diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index 0733589870..348a139e79 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -159,7 +159,7 @@ static void init_child(PtyProcess *ptyproc) FUNC_ATTR_NONNULL_ALL { #if defined(HAVE__NSGETENVIRON) - char **environ = *_NSGetEnviron(); +#define environ (*_NSGetEnviron()) #else extern char **environ; #endif -- cgit From 27a7a4d38405a30611339fc663e426904bda1099 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Sun, 31 Jan 2021 10:43:03 -0500 Subject: Use abort() instead of assert(false) for things that should never happen assert() is compiled out for release builds, but we don't want to continue running in these impossible situations. This also resolves the "implicit fallthrough" warnings for the asserts in switch cases. --- src/nvim/os/time.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/nvim/os') diff --git a/src/nvim/os/time.c b/src/nvim/os/time.c index 4b6533cd0c..5cf628935f 100644 --- a/src/nvim/os/time.c +++ b/src/nvim/os/time.c @@ -97,7 +97,7 @@ void os_microdelay(uint64_t us, bool ignoreinput) const int rv = uv_cond_timedwait(&delay_cond, &delay_mutex, ns_delta); if (0 != rv && UV_ETIMEDOUT != rv) { - assert(false); + abort(); break; } // Else: Timeout proceeded normally. -- cgit