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/eval/funcs.c | 54 +++++++++++++++++--------------------------------- src/nvim/eval/typval.c | 27 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 36 deletions(-) (limited to 'src/nvim/eval') diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 8235d74cbb..04e539d309 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -4887,7 +4887,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) bool executable = true; char **argv = tv_to_argv(&argvars[0], NULL, &executable); - char **env = NULL; + dict_T *env = NULL; if (!argv) { rettv->vval.v_number = executable ? 0 : -1; return; // Did error message in tv_to_argv. @@ -4936,7 +4936,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) #endif char *new_cwd = tv_dict_get_string(job_opts, "cwd", false); - if (new_cwd && strlen(new_cwd) > 0) { + if (new_cwd && *new_cwd != NUL) { cwd = new_cwd; // The new cwd must be a directory. if (!os_isdir_executable((const char *)cwd)) { @@ -4945,48 +4945,30 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) return; } } - dictitem_T *job_env = tv_dict_find(job_opts, S_LEN("env")); - if (job_env) { - if (job_env->di_tv.v_type != VAR_DICT) { - EMSG2(_(e_invarg2), "env"); - shell_free_argv(argv); - return; - } - - size_t custom_env_size = (size_t)tv_dict_len(job_env->di_tv.vval.v_dict); - size_t i = 0; - size_t env_size = 0; - - if (clear_env) { - // + 1 for last null entry - env = xmalloc((custom_env_size + 1) * sizeof(*env)); - env_size = 0; - } else { - env_size = os_get_fullenv_size(); - env = xmalloc((custom_env_size + env_size + 1) * sizeof(*env)); - - os_copy_fullenv(env, env_size); - i = env_size; - } - assert(env); // env must be allocated at this point + dictitem_T *job_env = tv_dict_find(job_opts, S_LEN("env")); + if (job_env && job_env->di_tv.v_type != VAR_DICT) { + EMSG2(_(e_invarg2), "env"); + shell_free_argv(argv); + return; + } - TV_DICT_ITER(job_env->di_tv.vval.v_dict, var, { - const char *str = tv_get_string(&var->di_tv); - assert(str); - size_t len = STRLEN(var->di_key) + strlen(str) + strlen("=") + 1; - env[i] = xmalloc(len); - snprintf(env[i], len, "%s=%s", (char *)var->di_key, str); - i++; - }); + env = tv_dict_alloc(); - // must be null terminated - env[env_size + custom_env_size] = NULL; + if (!clear_env) { + typval_T temp_env = TV_INITIAL_VALUE; + f_environ(NULL, &temp_env, NULL); + tv_dict_extend(env, temp_env.vval.v_dict, "force"); + tv_dict_free(temp_env.vval.v_dict); } + if (job_env) { + tv_dict_extend(env, job_env->di_tv.vval.v_dict, "force"); + } if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { shell_free_argv(argv); + tv_dict_free(env); return; } } diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c index 02d32a4f86..2f8776def4 100644 --- a/src/nvim/eval/typval.c +++ b/src/nvim/eval/typval.c @@ -1523,6 +1523,33 @@ varnumber_T tv_dict_get_number(const dict_T *const d, const char *const key) return tv_get_number(&di->di_tv); } +/// Converts a dict to an environment +/// +/// +char **tv_dict_to_env(dict_T *denv) +{ + size_t env_size = (size_t)tv_dict_len(denv); + + size_t i = 0; + char **env = NULL; + + // + 1 for NULL + env = xmalloc((env_size + 1) * sizeof(*env)); + + TV_DICT_ITER(denv, var, { + const char *str = tv_get_string(&var->di_tv); + assert(str); + size_t len = STRLEN(var->di_key) + strlen(str) + strlen("=") + 1; + env[i] = xmalloc(len); + snprintf(env[i], len, "%s=%s", (char *)var->di_key, str); + i++; + }); + + // must be null terminated + env[env_size] = NULL; + return env; +} + /// Get a string item from a dictionary /// /// @param[in] d Dictionary to get item from. -- cgit From ef7c6b972a1100fc5b173b7273f2c29017557669 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 10 Sep 2020 06:31:00 -0400 Subject: Support specifying "env" option for termopen() Co-authored-by: erw7 --- src/nvim/eval/funcs.c | 52 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 14 deletions(-) (limited to 'src/nvim/eval') diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 04e539d309..61f14b41e3 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -4875,6 +4875,24 @@ static void f_jobresize(typval_T *argvars, typval_T *rettv, FunPtr fptr) rettv->vval.v_number = 1; } +static dict_T *create_environment(const dictitem_T *job_env, const bool clear_env) +{ + dict_T * env = tv_dict_alloc(); + + if (!clear_env) { + typval_T temp_env = TV_INITIAL_VALUE; + f_environ(NULL, &temp_env, NULL); + tv_dict_extend(env, temp_env.vval.v_dict, "force"); + tv_dict_free(temp_env.vval.v_dict); + } + + if (job_env) { + tv_dict_extend(env, job_env->di_tv.vval.v_dict, "force"); + } + + return env; +} + // "jobstart()" function static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) { @@ -4953,18 +4971,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) return; } - env = tv_dict_alloc(); - - if (!clear_env) { - typval_T temp_env = TV_INITIAL_VALUE; - f_environ(NULL, &temp_env, NULL); - tv_dict_extend(env, temp_env.vval.v_dict, "force"); - tv_dict_free(temp_env.vval.v_dict); - } - - if (job_env) { - tv_dict_extend(env, job_env->di_tv.vval.v_dict, "force"); - } + env = create_environment(job_env, clear_env); if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { shell_free_argv(argv); @@ -10500,6 +10507,8 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr) Callback on_exit = CALLBACK_NONE; dict_T *job_opts = NULL; const char *cwd = "."; + dict_T *env = NULL; + if (argvars[1].v_type == VAR_DICT) { job_opts = argvars[1].vval.v_dict; @@ -10514,17 +10523,32 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr) } } + dictitem_T *job_env = tv_dict_find(job_opts, S_LEN("env")); + if (job_env && job_env->di_tv.v_type != VAR_DICT) { + EMSG2(_(e_invarg2), "env"); + shell_free_argv(argv); + return; + } + + bool clear_env = tv_dict_get_number(job_opts, "clear_env") != 0; + + env = create_environment(job_env, clear_env); + if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { shell_free_argv(argv); return; } } + const bool pty = true; + const bool rpc = false; + const bool overlapped = false; + const bool detach = false; uint16_t term_width = MAX(0, curwin->w_width_inner - win_col_off(curwin)); Channel *chan = channel_job_start(argv, on_stdout, on_stderr, on_exit, - true, false, false, false, cwd, + pty, rpc, overlapped, detach, cwd, term_width, curwin->w_height_inner, - xstrdup("xterm-256color"), NULL, + xstrdup("xterm-256color"), env, &rettv->vval.v_number); if (rettv->vval.v_number <= 0) { return; -- 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/eval/funcs.c | 93 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 83 insertions(+), 10 deletions(-) (limited to 'src/nvim/eval') diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 61f14b41e3..4486cd8f0d 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -4875,7 +4875,38 @@ static void f_jobresize(typval_T *argvars, typval_T *rettv, FunPtr fptr) rettv->vval.v_number = 1; } -static dict_T *create_environment(const dictitem_T *job_env, const bool clear_env) +static const char *ignored_env_vars[] = { +#ifndef WIN32 + "COLUMNS", + "LINES", + "TERMCAP", + "COLORFGBG", +#endif + NULL +}; + +/// According to comments in src/win/process.c of libuv, Windows has a few +/// "essential" environment variables. +static const char *required_env_vars[] = { +#ifdef WIN32 + "HOMEDRIVE", + "HOMEPATH", + "LOGONSERVER", + "PATH", + "SYSTEMDRIVE", + "SYSTEMROOT", + "TEMP", + "USERDOMAIN", + "USERNAME", + "USERPROFILE", + "WINDIR", +#endif + NULL +}; + +static dict_T *create_environment(const dictitem_T *job_env, + const bool clear_env, + const bool pty) { dict_T * env = tv_dict_alloc(); @@ -4884,12 +4915,52 @@ static dict_T *create_environment(const dictitem_T *job_env, const bool clear_en f_environ(NULL, &temp_env, NULL); tv_dict_extend(env, temp_env.vval.v_dict, "force"); tv_dict_free(temp_env.vval.v_dict); + + if (pty) { + // These environment variables generally shouldn't be propagated to the + // child process. We're removing them here so the user can still decide + // they want to explicitly set them. + for (size_t i = 0; + i < ARRAY_SIZE(ignored_env_vars) && ignored_env_vars[i]; + i++) { + dictitem_T *dv = tv_dict_find(env, ignored_env_vars[i], -1); + if (dv) { + tv_dict_item_remove(env, dv); + } + } +#ifndef WIN32 + // Set 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(env, S_LEN("COLORTERM")); + if (dv) { + tv_dict_item_remove(env, dv); + tv_dict_add_str(env, S_LEN("COLORTERM"), p_tgc ? "truecolor" : "256"); + } +#endif + } } if (job_env) { tv_dict_extend(env, job_env->di_tv.vval.v_dict, "force"); } + if (pty) { + // Now that the custom environment is configured, we need to ensure certain + // environment variables are present. + for (size_t i = 0; + i < ARRAY_SIZE(required_env_vars) && required_env_vars[i]; + i++) { + size_t len = strlen(required_env_vars[i]); + dictitem_T *dv = tv_dict_find(env, required_env_vars[i], len); + if (!dv) { + const char *env_var = os_getenv(required_env_vars[i]); + if (env_var) { + tv_dict_add_str(env, required_env_vars[i], len, env_var); + } + } + } + } + return env; } @@ -4929,6 +5000,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) on_stderr = CALLBACK_READER_INIT; Callback on_exit = CALLBACK_NONE; char *cwd = NULL; + dictitem_T *job_env = NULL; if (argvars[1].v_type == VAR_DICT) { job_opts = argvars[1].vval.v_dict; @@ -4964,22 +5036,21 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr) } } - dictitem_T *job_env = tv_dict_find(job_opts, S_LEN("env")); + job_env = tv_dict_find(job_opts, S_LEN("env")); if (job_env && job_env->di_tv.v_type != VAR_DICT) { EMSG2(_(e_invarg2), "env"); shell_free_argv(argv); return; } - env = create_environment(job_env, clear_env); - if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { shell_free_argv(argv); - tv_dict_free(env); return; } } + env = create_environment(job_env, clear_env, pty); + uint16_t width = 0, height = 0; char *term_name = NULL; @@ -10508,6 +10579,9 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr) dict_T *job_opts = NULL; const char *cwd = "."; dict_T *env = NULL; + const bool pty = true; + bool clear_env = false; + dictitem_T *job_env = NULL; if (argvars[1].v_type == VAR_DICT) { job_opts = argvars[1].vval.v_dict; @@ -10523,16 +10597,14 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr) } } - dictitem_T *job_env = tv_dict_find(job_opts, S_LEN("env")); + job_env = tv_dict_find(job_opts, S_LEN("env")); if (job_env && job_env->di_tv.v_type != VAR_DICT) { EMSG2(_(e_invarg2), "env"); shell_free_argv(argv); return; } - bool clear_env = tv_dict_get_number(job_opts, "clear_env") != 0; - - env = create_environment(job_env, clear_env); + clear_env = tv_dict_get_number(job_opts, "clear_env") != 0; if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { shell_free_argv(argv); @@ -10540,7 +10612,8 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr) } } - const bool pty = true; + env = create_environment(job_env, clear_env, pty); + const bool rpc = false; const bool overlapped = false; const bool detach = false; -- cgit From a54ac073fb0a636717cdd8791e043d301a642726 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Mon, 28 Sep 2020 23:05:48 -0400 Subject: eval/environ: Prefer the last definition of an env var It's possible for the environment variable block given to nvim to contain multiple definitions for the same env var. In this case, nvim should preserve the last one defined. --- src/nvim/eval/funcs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'src/nvim/eval') diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 4486cd8f0d..7121d413ea 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -1798,7 +1798,7 @@ static void f_environ(typval_T *argvars, typval_T *rettv, FunPtr fptr) os_copy_fullenv(env, env_size); - for (size_t i = 0; i < env_size; i++) { + for (ssize_t i = env_size - 1; i >= 0; i--) { const char * str = env[i]; const char * const end = strchr(str + (str[0] == '=' ? 1 : 0), '='); @@ -1806,6 +1806,12 @@ static void f_environ(typval_T *argvars, typval_T *rettv, FunPtr fptr) ptrdiff_t len = end - str; assert(len > 0); const char * value = str + len + 1; + if (tv_dict_find(rettv->vval.v_dict, str, len) != NULL) { + // Since we're traversing from the end of the env block to the front, any + // duplicate names encountered should be ignored. This preserves the + // semantics of env vars defined later in the env block taking precedence. + continue; + } tv_dict_add_str(rettv->vval.v_dict, str, len, value); -- 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/eval/decode.c | 4 ++-- src/nvim/eval/encode.c | 4 ++-- src/nvim/eval/executor.c | 2 +- src/nvim/eval/funcs.c | 4 ++-- src/nvim/eval/typval.c | 10 +++++----- 5 files changed, 12 insertions(+), 12 deletions(-) (limited to 'src/nvim/eval') diff --git a/src/nvim/eval/decode.c b/src/nvim/eval/decode.c index 638fef331a..bd4dc87d31 100644 --- a/src/nvim/eval/decode.c +++ b/src/nvim/eval/decode.c @@ -147,7 +147,7 @@ static inline int json_decoder_pop(ValuesStackItem obj, tv_clear(&key.val); if (tv_dict_add(last_container.container.vval.v_dict, obj_di) == FAIL) { - assert(false); + abort(); } obj_di->di_tv = obj.val; } else { @@ -480,7 +480,7 @@ static inline int parse_json_string(const char *const buf, const size_t buf_len, break; } default: { - assert(false); + abort(); } } } else { diff --git a/src/nvim/eval/encode.c b/src/nvim/eval/encode.c index 9a9f2e4287..a4d7af7971 100644 --- a/src/nvim/eval/encode.c +++ b/src/nvim/eval/encode.c @@ -174,7 +174,7 @@ static int conv_error(const char *const msg, const MPConvStack *const mpstack, case kMPConvPartial: { switch (v.data.p.stage) { case kMPConvPartialArgs: { - assert(false); + abort(); break; } case kMPConvPartialSelf: { @@ -237,7 +237,7 @@ bool encode_vim_list_to_buf(const list_T *const list, size_t *const ret_len, char *const buf = xmalloc(len); size_t read_bytes; if (encode_read_from_list(&lrstate, buf, len, &read_bytes) != OK) { - assert(false); + abort(); } assert(len == read_bytes); *ret_buf = buf; diff --git a/src/nvim/eval/executor.c b/src/nvim/eval/executor.c index da05ecda43..bbba9d12f2 100644 --- a/src/nvim/eval/executor.c +++ b/src/nvim/eval/executor.c @@ -118,7 +118,7 @@ int eexe_mod_op(typval_T *const tv1, const typval_T *const tv2, return OK; } case VAR_UNKNOWN: { - assert(false); + abort(); } } } diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 7121d413ea..1a7bbf2d0e 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -3307,7 +3307,7 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv, FunPtr fptr) } break; case kCdScopeInvalid: // We should never get here - assert(false); + abort(); } if (from) { @@ -4360,7 +4360,7 @@ static void f_haslocaldir(typval_T *argvars, typval_T *rettv, FunPtr fptr) break; case kCdScopeInvalid: // We should never get here - assert(false); + abort(); } } diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c index 2f8776def4..9be487f4fd 100644 --- a/src/nvim/eval/typval.c +++ b/src/nvim/eval/typval.c @@ -2521,7 +2521,7 @@ void tv_item_lock(typval_T *const tv, const int deep, const bool lock) break; } case VAR_UNKNOWN: { - assert(false); + abort(); } } #undef CHANGE_LOCK @@ -2693,7 +2693,7 @@ bool tv_equal(typval_T *const tv1, typval_T *const tv2, const bool ic, } } - assert(false); + abort(); return false; } @@ -2746,7 +2746,7 @@ bool tv_check_str_or_nr(const typval_T *const tv) return false; } } - assert(false); + abort(); return false; } @@ -2791,7 +2791,7 @@ bool tv_check_num(const typval_T *const tv) return false; } } - assert(false); + abort(); return false; } @@ -2836,7 +2836,7 @@ bool tv_check_str(const typval_T *const tv) return false; } } - assert(false); + abort(); return false; } -- cgit