From fd973c0a4ec0246708d0fcc46e66e38dd1f89a26 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Wed, 16 Apr 2025 12:36:07 +0200 Subject: [PATCH] fix(env.c): drop envmap, free os_getenv() result #32683 Problem: vim.uv.os_setenv gets "stuck" per-key. #32550 Caused by the internal `envmap` cache. #7920 :echo $FOO <-- prints nothing :lua vim.uv.os_setenv("FOO", "bar") :echo $FOO <-- prints bar (as expected) :lua vim.uv.os_setenv("FOO", "fizz") :echo $FOO <-- prints bar, still (not expected. Should be "fizz") :lua vim.uv.os_unsetenv("FOO") :echo $FOO <-- prints bar, still (not expected. Should be nothing) :lua vim.uv.os_setenv("FOO", "buzz") :echo $FOO <-- prints bar, still (not expected. Should be "buzz") Solution: - Remove the `envmap` cache. - Callers to `os_getenv` must free the result. - Update all call-sites. - Introduce `os_getenv_noalloc`. - Extend `os_env_exists()` the `nonempty` parameter. --- src/nvim/diff.c | 2 +- src/nvim/eval/funcs.c | 6 +- src/nvim/ex_docmd.c | 4 +- src/nvim/fileio.c | 2 +- src/nvim/log.c | 2 +- src/nvim/lua/executor.c | 5 +- src/nvim/main.c | 7 +- src/nvim/mbyte.c | 7 +- src/nvim/memory.c | 1 - src/nvim/msgpack_rpc/server.c | 30 +++---- src/nvim/option.c | 7 +- src/nvim/os/env.c | 129 +++++++++++++++++------------- src/nvim/os/fs.c | 7 +- src/nvim/os/lang.c | 9 ++- src/nvim/os/os_win_console.c | 5 +- src/nvim/os/stdpaths.c | 18 ++--- src/nvim/os/time.c | 4 +- src/nvim/os/users.c | 2 +- src/nvim/path.c | 3 +- src/nvim/runtime.c | 3 +- src/nvim/tui/input.c | 3 +- src/nvim/tui/terminfo.c | 2 +- src/nvim/tui/tui.c | 48 +++++++---- src/nvim/ui_client.c | 4 +- test/functional/core/env_spec.lua | 27 +++++++ test/unit/os/env_spec.lua | 90 ++++++++++++++++++--- 26 files changed, 281 insertions(+), 146 deletions(-) create mode 100644 test/functional/core/env_spec.lua diff --git a/src/nvim/diff.c b/src/nvim/diff.c index ad3e093dde..ed2078207d 100644 --- a/src/nvim/diff.c +++ b/src/nvim/diff.c @@ -1143,7 +1143,7 @@ static int diff_file(diffio_T *dio) char *const cmd = xmalloc(len); // We don't want $DIFF_OPTIONS to get in the way. - if (os_getenv("DIFF_OPTIONS")) { + if (os_env_exists("DIFF_OPTIONS", true)) { os_unsetenv("DIFF_OPTIONS"); } diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c index 25252cdfde..d8ffaeef9f 100644 --- a/src/nvim/eval/funcs.c +++ b/src/nvim/eval/funcs.c @@ -1556,7 +1556,7 @@ static void f_exists(typval_T *argvars, typval_T *rettv, EvalFuncData fptr) const char *p = tv_get_string(&argvars[0]); if (*p == '$') { // Environment variable. // First try "normal" environment variables (fast). - if (os_env_exists(p + 1)) { + if (os_env_exists(p + 1, false)) { n = true; } else { // Try expanding things like $VIM and ${HOME}. @@ -3881,9 +3881,9 @@ dict_T *create_environment(const dictitem_T *job_env, const bool clear_env, cons size_t len = strlen(required_env_vars[i]); dictitem_T *dv = tv_dict_find(env, required_env_vars[i], (ptrdiff_t)len); if (!dv) { - const char *env_var = os_getenv(required_env_vars[i]); + char *env_var = os_getenv(required_env_vars[i]); if (env_var) { - tv_dict_add_str(env, required_env_vars[i], len, env_var); + tv_dict_add_allocated_str(env, required_env_vars[i], len, env_var); } } } diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c index 746dc15c71..030c3e8016 100644 --- a/src/nvim/ex_docmd.c +++ b/src/nvim/ex_docmd.c @@ -7810,8 +7810,8 @@ static void ex_checkhealth(exarg_T *eap) return; } - const char *vimruntime_env = os_getenv("VIMRUNTIME"); - if (vimruntime_env == NULL) { + char *vimruntime_env = os_getenv_noalloc("VIMRUNTIME"); + if (!vimruntime_env) { emsg(_("E5009: $VIMRUNTIME is empty or unset")); } else { bool rtp_ok = NULL != strstr(p_rtp, vimruntime_env); diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index b79ecf22d7..a72d0e019f 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -3278,7 +3278,7 @@ static void vim_mktempdir(void) expand_env((char *)temp_dirs[i], tmp, TEMP_FILE_PATH_MAXLEN - 64); if (!os_isdir(tmp)) { if (strequal("$TMPDIR", temp_dirs[i])) { - if (!os_getenv("TMPDIR")) { + if (!os_env_exists("TMPDIR", true)) { DLOG("$TMPDIR is unset"); } else { WLOG("$TMPDIR tempdir not a directory (or does not exist): \"%s\"", tmp); diff --git a/src/nvim/log.c b/src/nvim/log.c index ef5e21aa0a..0faf921a0d 100644 --- a/src/nvim/log.c +++ b/src/nvim/log.c @@ -336,7 +336,7 @@ static bool v_do_log_to_file(FILE *log_file, int log_level, const char *context, // TODO(justinmk): expose this as v:name ? if (regen) { // Parent servername ($NVIM). - const char *parent = path_tail(os_getenv(ENV_NVIM)); + const char *parent = path_tail(os_getenv_noalloc(ENV_NVIM)); // Servername. Empty until starting=false. const char *serv = path_tail(get_vim_var_str(VV_SEND_SERVER)); if (parent[0] != NUL) { diff --git a/src/nvim/lua/executor.c b/src/nvim/lua/executor.c index 49ff5679e2..5dac24fbc0 100644 --- a/src/nvim/lua/executor.c +++ b/src/nvim/lua/executor.c @@ -831,8 +831,7 @@ static bool nlua_state_init(lua_State *const lstate) FUNC_ATTR_NONNULL_ALL void nlua_init(char **argv, int argc, int lua_arg0) { #ifdef NLUA_TRACK_REFS - const char *env = os_getenv("NVIM_LUA_NOTRACK"); - if (!env || !*env) { + if (os_env_exists("NVIM_LUA_NOTRACK", true)) { nlua_track_refs = true; } #endif @@ -1283,7 +1282,7 @@ static int nlua_empty_dict_tostring(lua_State *lstate) /// @param lstate Lua interpreter state. static int nlua_getenv(lua_State *lstate) { - lua_pushstring(lstate, os_getenv(luaL_checkstring(lstate, 1))); + lua_pushstring(lstate, os_getenv_noalloc(luaL_checkstring(lstate, 1))); return 1; } #endif diff --git a/src/nvim/main.c b/src/nvim/main.c index bc960f239a..73e1abe6a6 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -938,12 +938,11 @@ static void remote_request(mparm_T *params, int remote_args, char *server_addr, if (!chan) { fprintf(stderr, "Remote ui failed to start: %s\n", connect_error); os_exit(1); - } else if (strequal(server_addr, os_getenv("NVIM"))) { + } else if (strequal(server_addr, os_getenv_noalloc("NVIM"))) { fprintf(stderr, "%s", "Cannot attach UI of :terminal child to its parent. "); fprintf(stderr, "%s\n", "(Unset $NVIM to skip this check)"); os_exit(1); } - ui_client_channel_id = chan; return; } @@ -2118,7 +2117,7 @@ static void source_startup_scripts(const mparm_T *const parmp) static int execute_env(char *env) FUNC_ATTR_NONNULL_ALL { - const char *initstr = os_getenv(env); + char *initstr = os_getenv(env); if (initstr == NULL) { return FAIL; } @@ -2132,6 +2131,8 @@ static int execute_env(char *env) estack_pop(); current_sctx = save_current_sctx; + + xfree(initstr); return OK; } diff --git a/src/nvim/mbyte.c b/src/nvim/mbyte.c index 18df6b7e76..aae5445805 100644 --- a/src/nvim/mbyte.c +++ b/src/nvim/mbyte.c @@ -2412,14 +2412,15 @@ char *enc_locale(void) char buf[50]; const char *s; + #ifdef HAVE_NL_LANGINFO_CODESET if (!(s = nl_langinfo(CODESET)) || *s == NUL) #endif { if (!(s = setlocale(LC_CTYPE, NULL)) || *s == NUL) { - if ((s = os_getenv("LC_ALL"))) { - if ((s = os_getenv("LC_CTYPE"))) { - s = os_getenv("LANG"); + if ((s = os_getenv_noalloc("LC_ALL"))) { + if ((s = os_getenv_noalloc("LC_CTYPE"))) { + s = os_getenv_noalloc("LANG"); } } } diff --git a/src/nvim/memory.c b/src/nvim/memory.c index af766c0d20..5464390d52 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -788,7 +788,6 @@ void free_all_mem(void) free_all_marks(); alist_clear(&global_alist); free_homedir(); - free_envmap(); free_users(); free_search_patterns(); free_old_sub(); diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index b2c6d0d007..5c89b7d3bf 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -41,28 +41,24 @@ bool server_init(const char *listen_addr) ga_init(&watchers, sizeof(SocketWatcher *), 1); // $NVIM_LISTEN_ADDRESS (deprecated) - if ((!listen_addr || listen_addr[0] == '\0') && os_env_exists(ENV_LISTEN)) { - user_arg = kFalse; // User-provided env var. - listen_addr = os_getenv(ENV_LISTEN); - } - if (!listen_addr || listen_addr[0] == '\0') { - user_arg = kNone; // Autogenerated server address. - listen_addr = server_address_new(NULL); + if (os_env_exists(ENV_LISTEN, true)) { + user_arg = kFalse; // User-provided env var. + listen_addr = os_getenv(ENV_LISTEN); + } else { + user_arg = kNone; // Autogenerated server address. + listen_addr = server_address_new(NULL); + } must_free = true; } 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")) { + if (os_env_exists("__NVIM_TEST_LOG", false)) { ELOG("test log message"); } - if (must_free) { - xfree((char *)listen_addr); - } - if (rv == 0 || user_arg == kNone) { // The autogenerated servername can fail if the user has a broken $XDG_RUNTIME_DIR. #30282 // But that is not fatal (startup will continue, logged in $NVIM_LOGFILE, empty v:servername). @@ -78,12 +74,16 @@ bool server_init(const char *listen_addr) ok = false; end: - if (os_env_exists(ENV_LISTEN)) { + if (os_env_exists(ENV_LISTEN, false)) { // Unset $NVIM_LISTEN_ADDRESS, it's a liability hereafter. It is "input only", it must not be // leaked to child jobs or :terminal. os_unsetenv(ENV_LISTEN); } + if (must_free) { + xfree((char *)listen_addr); + } + return ok; } @@ -120,12 +120,12 @@ char *server_address_new(const char *name) static uint32_t count = 0; char fmt[ADDRESS_MAX_SIZE]; #ifdef MSWIN - (void)get_appname(true); + (void)get_appname(true); // Writes appname to NameBuf. int r = snprintf(fmt, sizeof(fmt), "\\\\.\\pipe\\%s.%" PRIu64 ".%" PRIu32, name ? name : NameBuff, os_get_pid(), count++); #else char *dir = stdpaths_get_xdg_var(kXDGRuntimeDir); - (void)get_appname(true); + (void)get_appname(true); // Writes appname to NameBuf. int r = snprintf(fmt, sizeof(fmt), "%s/%s.%" PRIu64 ".%" PRIu32, dir, name ? name : NameBuff, os_get_pid(), count++); xfree(dir); diff --git a/src/nvim/option.c b/src/nvim/option.c index 4bc05ccea3..643ac24e43 100644 --- a/src/nvim/option.c +++ b/src/nvim/option.c @@ -190,7 +190,7 @@ static void set_init_default_shell(void) { // Find default value for 'shell' option. // Don't use it if it is empty. - const char *shell = os_getenv("SHELL"); + char *shell = os_getenv("SHELL"); if (shell != NULL) { if (vim_strchr(shell, ' ') != NULL) { const size_t len = strlen(shell) + 3; // two quotes and a trailing NUL @@ -198,8 +198,9 @@ static void set_init_default_shell(void) snprintf(cmd, len, "\"%s\"", shell); set_string_default(kOptShell, cmd, true); } else { - set_string_default(kOptShell, (char *)shell, false); + set_string_default(kOptShell, shell, false); } + xfree(shell); } } @@ -413,7 +414,7 @@ void set_init_1(bool clean_arg) // abilities (bidi namely). // NOTE: mlterm's author is being asked to 'set' a variable // instead of an environment variable due to inheritance. - if (os_env_exists("MLTERM")) { + if (os_env_exists("MLTERM", false)) { set_option_value_give_err(kOptTermbidi, BOOLEAN_OPTVAL(true), 0); } diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index 3126881266..4acc243a92 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -52,13 +52,11 @@ # include "os/env.c.generated.h" #endif -// Because `uv_os_getenv` requires allocating, we must manage a map to maintain -// the behavior of `os_getenv`. -static PMap(cstr_t) envmap = MAP_INIT; - /// Like getenv(), but returns NULL if the variable is empty. +/// Result must be freed by the caller. /// @see os_env_exists -const char *os_getenv(const char *name) +/// @see os_getenv_noalloc +char *os_getenv(const char *name) FUNC_ATTR_NONNULL_ALL { char *e = NULL; @@ -66,17 +64,6 @@ const char *os_getenv(const char *name) return NULL; } int r = 0; - if (map_has(cstr_t, &envmap, name) - && !!(e = (char *)pmap_get(cstr_t)(&envmap, name))) { - if (e[0] != NUL) { - // Found non-empty cached env var. - // NOTE: This risks incoherence if an in-process library changes the - // environment without going through our os_setenv() wrapper. If - // that turns out to be a problem, we can just remove this codepath. - goto end; - } - pmap_del2(&envmap, name); - } #define INIT_SIZE 64 size_t size = INIT_SIZE; char buf[INIT_SIZE]; @@ -96,7 +83,6 @@ const char *os_getenv(const char *name) // except when it does not include the NUL-terminator. e = xmemdupz(buf, size); } - pmap_put(cstr_t)(&envmap, xstrdup(name), e); end: if (r != 0 && r != UV_ENOENT && r != UV_UNKNOWN) { ELOG("uv_os_getenv(%s) failed: %d %s", name, r, uv_err_name(r)); @@ -104,9 +90,43 @@ end: return e; } +/// Like getenv(), but returns a pointer to `NameBuff` instead of allocating, or NULL on failure. +/// Value is truncated if it exceeds sizeof(NameBuff). +/// @see os_env_exists +char *os_getenv_noalloc(const char *name) + FUNC_ATTR_NONNULL_ALL +{ + if (name[0] == NUL) { + return NULL; + } + + size_t size = sizeof(NameBuff); + int r = uv_os_getenv(name, NameBuff, &size); + if (r == UV_ENOBUFS) { + char *e = xmalloc(size); + r = uv_os_getenv(name, e, &size); + if (r == 0 && size != 0 && e[0] != NUL) { + xmemcpyz(NameBuff, e, sizeof(NameBuff) - 1); + } + xfree(e); + } + + if (r != 0 || size == 0 || NameBuff[0] == NUL) { + if (r != 0 && r != UV_ENOENT && r != UV_UNKNOWN) { + ELOG("uv_os_getenv(%s) failed: %d %s", name, r, uv_err_name(r)); + } + return NULL; + } + return NameBuff; +} + /// Returns true if environment variable `name` is defined (even if empty). /// Returns false if not found (UV_ENOENT) or other failure. -bool os_env_exists(const char *name) +/// +/// @param name the environment variable in question +/// @param nonempty Require a non-empty value. Treat empty as "does not exist". +/// @return whether the variable exists +bool os_env_exists(const char *name, bool nonempty) FUNC_ATTR_NONNULL_ALL { if (name[0] == NUL) { @@ -114,14 +134,14 @@ bool os_env_exists(const char *name) } // Use a tiny buffer because we don't care about the value: if uv_os_getenv() // returns UV_ENOBUFS, the env var was found. - char buf[1]; + char buf[2]; size_t size = sizeof(buf); int r = uv_os_getenv(name, buf, &size); assert(r != UV_EINVAL); if (r != 0 && r != UV_ENOENT && r != UV_ENOBUFS) { ELOG("uv_os_getenv(%s) failed: %d %s", name, r, uv_err_name(r)); } - return (r == 0 || r == UV_ENOBUFS); + return ((r == 0 && (!nonempty || size > 0)) || r == UV_ENOBUFS); } /// Sets an environment variable. @@ -137,7 +157,7 @@ int os_setenv(const char *name, const char *value, int overwrite) return -1; } #ifdef MSWIN - if (!overwrite && os_getenv(name) != NULL) { + if (!overwrite && !os_env_exists(name, true)) { return 0; } if (value[0] == NUL) { @@ -145,7 +165,7 @@ int os_setenv(const char *name, const char *value, int overwrite) return os_unsetenv(name); } #else - if (!overwrite && os_env_exists(name)) { + if (!overwrite && os_env_exists(name, false)) { return 0; } #endif @@ -160,9 +180,6 @@ int os_setenv(const char *name, const char *value, int overwrite) #endif r = uv_os_setenv(name, value); assert(r != UV_EINVAL); - // Destroy the old map item. Do this AFTER uv_os_setenv(), because `value` - // could be a previous os_getenv() result. - pmap_del2(&envmap, name); if (r != 0) { ELOG("uv_os_setenv(%s) failed: %d %s", name, r, uv_err_name(r)); } @@ -176,7 +193,6 @@ int os_unsetenv(const char *name) if (name[0] == NUL) { return -1; } - pmap_del2(&envmap, name); int r = uv_os_unsetenv(name); if (r != 0) { ELOG("uv_os_unsetenv(%s) failed: %d %s", name, r, uv_err_name(r)); @@ -428,7 +444,8 @@ void init_homedir(void) xfree(homedir); homedir = NULL; - const char *var = os_getenv("HOME"); + char *var = os_getenv("HOME"); + char *tofree = var; #ifdef MSWIN // Typically, $HOME is not defined on Windows, unless the user has @@ -436,10 +453,10 @@ void init_homedir(void) // platforms, $HOMEDRIVE and $HOMEPATH are automatically defined for // each user. Try constructing $HOME from these. if (var == NULL) { - const char *homedrive = os_getenv("HOMEDRIVE"); - const char *homepath = os_getenv("HOMEPATH"); + char *homedrive = os_getenv("HOMEDRIVE"); + char *homepath = os_getenv("HOMEPATH"); if (homepath == NULL) { - homepath = "\\"; + homepath = xstrdup("\\"); } if (homedrive != NULL && strlen(homedrive) + strlen(homepath) < MAXPATHL) { @@ -448,6 +465,8 @@ void init_homedir(void) var = os_buf; } } + xfree(homepath); + xfree(homedrive); } if (var == NULL) { var = os_uv_homedir(); @@ -461,11 +480,13 @@ void init_homedir(void) if (p != NULL) { vim_snprintf(os_buf, (size_t)(p - var), "%s", var + 1); var = NULL; - const char *exp = os_getenv(os_buf); - if (exp != NULL && *exp != NUL - && strlen(exp) + strlen(p) < MAXPATHL) { - vim_snprintf(os_buf, MAXPATHL, "%s%s", exp, p + 1); - var = os_buf; + char *exp = os_getenv(os_buf); + if (exp != NULL) { + if (*exp != NUL && strlen(exp) + strlen(p) < MAXPATHL) { + vim_snprintf(os_buf, MAXPATHL, "%s%s", exp, p + 1); + var = os_buf; + } + xfree(exp); } } } @@ -498,6 +519,7 @@ void init_homedir(void) if (var != NULL) { homedir = xstrdup(var); } + xfree(tofree); } static char homedir_buf[MAXPATHL]; @@ -523,17 +545,6 @@ void free_homedir(void) xfree(homedir); } -void free_envmap(void) -{ - cstr_t name; - ptr_t e; - map_foreach(&envmap, name, e, { - xfree((char *)name); - xfree(e); - }); - map_destroy(cstr_t, &envmap); -} - #endif /// Call expand_env() and store the result in an allocated string. @@ -917,9 +928,9 @@ char *vim_getenv(const char *name) } #endif - const char *kos_env_path = os_getenv(name); + char *kos_env_path = os_getenv(name); if (kos_env_path != NULL) { - return xstrdup(kos_env_path); + return kos_env_path; } bool vimruntime = (strcmp(name, "VIMRUNTIME") == 0); @@ -932,11 +943,13 @@ char *vim_getenv(const char *name) char *vim_path = NULL; if (vimruntime && *default_vimruntime_dir == NUL) { - kos_env_path = os_getenv("VIM"); + kos_env_path = os_getenv("VIM"); // kos_env_path was NULL. if (kos_env_path != NULL) { vim_path = vim_version_dir(kos_env_path); if (vim_path == NULL) { - vim_path = xstrdup(kos_env_path); + vim_path = kos_env_path; + } else { + xfree(kos_env_path); } } } @@ -1059,13 +1072,13 @@ size_t home_replace(const buf_T *const buf, const char *src, char *const dst, si dirlen = strlen(homedir); } - const char *homedir_env = os_getenv("HOME"); + char *homedir_env = os_getenv("HOME"); #ifdef MSWIN if (homedir_env == NULL) { homedir_env = os_getenv("USERPROFILE"); } #endif - char *homedir_env_mod = (char *)homedir_env; + char *homedir_env_mod = homedir_env; bool must_free = false; if (homedir_env_mod != NULL && *homedir_env_mod == '~') { @@ -1142,6 +1155,8 @@ size_t home_replace(const buf_T *const buf, const char *src, char *const dst, si *dst_p = NUL; + xfree(homedir_env); + if (must_free) { xfree(homedir_env_mod); } @@ -1199,9 +1214,10 @@ bool os_setenv_append_path(const char *fname) size_t dirlen = (size_t)(tail - fname); assert(tail >= fname && dirlen + 1 < sizeof(os_buf)); xmemcpyz(os_buf, fname, dirlen); - const char *path = os_getenv("PATH"); + char *path = os_getenv("PATH"); const size_t pathlen = path ? strlen(path) : 0; const size_t newlen = pathlen + dirlen + 2; + bool retval = false; if (newlen < MAX_ENVPATHLEN) { char *temp = xmalloc(newlen); if (pathlen == 0) { @@ -1215,9 +1231,10 @@ bool os_setenv_append_path(const char *fname) xstrlcat(temp, os_buf, newlen); os_setenv("PATH", temp, 1); xfree(temp); - return true; + retval = true; } - return false; + xfree(path); + return retval; } /// Returns true if `sh` looks like it resolves to "cmd.exe". @@ -1228,7 +1245,7 @@ bool os_shell_is_cmdexe(const char *sh) return false; } if (striequal(sh, "$COMSPEC")) { - const char *comspec = os_getenv("COMSPEC"); + char *comspec = os_getenv_noalloc("COMSPEC"); return striequal("cmd.exe", path_tail(comspec)); } if (striequal(sh, "cmd.exe") || striequal(sh, "cmd")) { diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index 451994241d..f7ef3a5235 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -303,7 +303,7 @@ static bool is_executable_ext(const char *name, char **abspath) size_t nameext_len = nameext ? strlen(nameext) : 0; xstrlcpy(os_buf, name, sizeof(os_buf)); char *buf_end = xstrchrnul(os_buf, NUL); - const char *pathext = os_getenv("PATHEXT"); + const char *pathext = os_getenv_noalloc("PATHEXT"); if (!pathext) { pathext = ".com;.exe;.bat;.cmd"; } @@ -350,14 +350,14 @@ static bool is_executable_ext(const char *name, char **abspath) static bool is_executable_in_path(const char *name, char **abspath) FUNC_ATTR_NONNULL_ARG(1) { - const char *path_env = os_getenv("PATH"); + char *path_env = os_getenv("PATH"); if (path_env == NULL) { return false; } #ifdef MSWIN char *path = NULL; - if (!os_env_exists("NoDefaultCurrentDirectoryInExePath")) { + if (!os_env_exists("NoDefaultCurrentDirectoryInExePath", false)) { // Prepend ".;" to $PATH. size_t pathlen = strlen(path_env); path = xmallocz(pathlen + 2); @@ -404,6 +404,7 @@ static bool is_executable_in_path(const char *name, char **abspath) end: xfree(buf); xfree(path); + xfree(path_env); return rv; } diff --git a/src/nvim/os/lang.c b/src/nvim/os/lang.c index fb534ab2f4..6f6eba805a 100644 --- a/src/nvim/os/lang.c +++ b/src/nvim/os/lang.c @@ -70,6 +70,7 @@ char *get_mess_lang(void) } /// Get the language used for messages from the environment. +/// The function may be using NameBuff. /// /// This uses LC_MESSAGES when available, which it is for most systems we build for /// except for windows. Then fallback to get the value from the environment @@ -79,17 +80,17 @@ static char *get_mess_env(void) #ifdef LC_MESSAGES return get_locale_val(LC_MESSAGES); #else - char *p = (char *)os_getenv("LC_ALL"); + char *p = os_getenv_noalloc("LC_ALL"); if (p != NULL) { return p; } - p = (char *)os_getenv("LC_MESSAGES"); + p = os_getenv_noalloc("LC_MESSAGES"); if (p != NULL) { return p; } - p = (char *)os_getenv("LANG"); + p = os_getenv_noalloc("LANG"); if (p != NULL && ascii_isdigit(*p)) { p = NULL; // ignore something like "1043" } @@ -340,7 +341,7 @@ char *get_locales(expand_T *xp, int idx) void lang_init(void) { #ifdef __APPLE__ - if (os_getenv("LANG") == NULL) { + if (!os_env_exists("LANG", true)) { char buf[50] = { 0 }; // $LANG is not set, either because it was unset or Nvim was started diff --git a/src/nvim/os/os_win_console.c b/src/nvim/os/os_win_console.c index 953d291290..b1bbe60eab 100644 --- a/src/nvim/os/os_win_console.c +++ b/src/nvim/os/os_win_console.c @@ -82,7 +82,7 @@ void os_icon_init(void) hOrigIconSmall = (HICON)SendMessage(hWnd, WM_GETICON, (WPARAM)ICON_SMALL, (LPARAM)0); hOrigIcon = (HICON)SendMessage(hWnd, WM_GETICON, (WPARAM)ICON_BIG, (LPARAM)0); - const char *vimruntime = os_getenv("VIMRUNTIME"); + char *vimruntime = os_getenv("VIMRUNTIME"); if (vimruntime != NULL) { snprintf(NameBuff, MAXPATHL, "%s/neovim.ico", vimruntime); if (!os_path_exists(NameBuff)) { @@ -92,6 +92,7 @@ void os_icon_init(void) LR_LOADFROMFILE | LR_LOADMAP3DCOLORS); os_icon_set(hVimIcon, hVimIcon); } + xfree(vimruntime); } } @@ -117,7 +118,7 @@ void os_title_reset(void) /// @param out_fd stdout file descriptor void os_tty_guess_term(const char **term, int out_fd) { - bool conemu_ansi = strequal(os_getenv("ConEmuANSI"), "ON"); + bool conemu_ansi = strequal(os_getenv_noalloc("ConEmuANSI"), "ON"); bool vtp = false; HANDLE handle = (HANDLE)_get_osfhandle(out_fd); diff --git a/src/nvim/os/stdpaths.c b/src/nvim/os/stdpaths.c index e9a74d197f..cdb755a9c8 100644 --- a/src/nvim/os/stdpaths.c +++ b/src/nvim/os/stdpaths.c @@ -70,19 +70,19 @@ static const char *const xdg_defaults[] = { /// @return $NVIM_APPNAME value const char *get_appname(bool namelike) { - const char *env_val = os_getenv("NVIM_APPNAME"); - if (env_val == NULL || *env_val == NUL) { - env_val = "nvim"; + const char *env_val = os_getenv_noalloc("NVIM_APPNAME"); + + if (!env_val) { + xstrlcpy(NameBuff, "nvim", sizeof(NameBuff)); } if (namelike) { // Appname may be a relative path, replace slashes to make it name-like. - xstrlcpy(NameBuff, env_val, sizeof(NameBuff)); memchrsub(NameBuff, '/', '-', sizeof(NameBuff)); memchrsub(NameBuff, '\\', '-', sizeof(NameBuff)); } - return env_val; + return NameBuff; } /// Ensure that APPNAME is valid. Must be a name or relative path. @@ -157,21 +157,21 @@ char *stdpaths_get_xdg_var(const XDGVarType idx) const char *const env = xdg_env_vars[idx]; const char *const fallback = xdg_defaults[idx]; - const char *env_val = os_getenv(env); + char *env_val = os_getenv(env); #ifdef MSWIN if (env_val == NULL && xdg_defaults_env_vars[idx] != NULL) { env_val = os_getenv(xdg_defaults_env_vars[idx]); } #else - if (env_val == NULL && os_env_exists(env)) { - env_val = ""; + if (env_val == NULL && os_env_exists(env, false)) { + env_val = xstrdup(""); } #endif char *ret = NULL; if (env_val != NULL) { - ret = xstrdup(env_val); + ret = env_val; } else if (fallback) { ret = expand_env_save((char *)fallback); } else if (idx == kXDGRuntimeDir) { diff --git a/src/nvim/os/time.c b/src/nvim/os/time.c index 16118028b4..febd2fa2f2 100644 --- a/src/nvim/os/time.c +++ b/src/nvim/os/time.c @@ -98,8 +98,8 @@ struct tm *os_localtime_r(const time_t *restrict clock, // Call tzset(3) to update the global timezone variables if it has. // POSIX standard doesn't require localtime_r() implementations to do that // as it does with localtime(), and we don't want to call tzset() every time. - const char *tz = os_getenv("TZ"); - if (tz == NULL) { + const char *tz = os_getenv_noalloc("TZ"); + if (!tz) { tz = ""; } if (strncmp(tz_cache, tz, sizeof(tz_cache) - 1) != 0) { diff --git a/src/nvim/os/users.c b/src/nvim/os/users.c index d5a8355470..3d79a47824 100644 --- a/src/nvim/os/users.c +++ b/src/nvim/os/users.c @@ -89,7 +89,7 @@ int os_get_usernames(garray_T *users) #endif #ifdef HAVE_PWD_FUNCS { - const char *user_env = os_getenv("USER"); + char *user_env = os_getenv_noalloc("USER"); // The $USER environment variable may be a valid remote user name (NIS, // LDAP) not already listed by getpwent(), as getpwent() only lists diff --git a/src/nvim/path.c b/src/nvim/path.c index 1f1ee42a87..680af198a8 100644 --- a/src/nvim/path.c +++ b/src/nvim/path.c @@ -2392,7 +2392,7 @@ bool path_is_absolute(const char *fname) void path_guess_exepath(const char *argv0, char *buf, size_t bufsize) FUNC_ATTR_NONNULL_ALL { - const char *path = os_getenv("PATH"); + char *path = os_getenv("PATH"); if (path == NULL || path_is_absolute(argv0)) { xstrlcpy(buf, argv0, bufsize); @@ -2427,4 +2427,5 @@ void path_guess_exepath(const char *argv0, char *buf, size_t bufsize) // Not found in $PATH, fall back to argv0. xstrlcpy(buf, argv0, bufsize); } + xfree(path); } diff --git a/src/nvim/runtime.c b/src/nvim/runtime.c index 86ca4f08dc..9b8a527a1e 100644 --- a/src/nvim/runtime.c +++ b/src/nvim/runtime.c @@ -1746,8 +1746,7 @@ char *runtimepath_default(bool clean_arg) size_t config_len = 0; size_t vimruntime_len = 0; size_t libdir_len = 0; - const char *appname = get_appname(false); - size_t appname_len = strlen(appname); + size_t appname_len = strlen(get_appname(false)); if (data_home != NULL) { data_len = strlen(data_home); size_t nvim_data_size = appname_len; diff --git a/src/nvim/tui/input.c b/src/nvim/tui/input.c index 1f73f2d135..5a69732241 100644 --- a/src/nvim/tui/input.c +++ b/src/nvim/tui/input.c @@ -139,7 +139,8 @@ void tinput_init(TermInput *input, Loop *loop) input->in_fd = STDIN_FILENO; - const char *term = os_getenv("TERM"); + const char *term = os_getenv_noalloc("TERM"); + if (!term) { term = ""; // termkey_new_abstract assumes non-null (#2745) } diff --git a/src/nvim/tui/terminfo.c b/src/nvim/tui/terminfo.c index 657bd6dd10..bf87ea4ec5 100644 --- a/src/nvim/tui/terminfo.c +++ b/src/nvim/tui/terminfo.c @@ -49,7 +49,7 @@ bool terminfo_is_bsd_console(const char *term) # if defined(__FreeBSD__) // FreeBSD console sets TERM=xterm, but it does not support xterm features // like cursor-shaping. Assume that TERM=xterm is degraded. #8644 - return strequal(term, "xterm") && !!os_getenv("XTERM_VERSION"); + return strequal(term, "xterm") && os_env_exists("XTERM_VERSION", true); # endif #endif return false; diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index c19e419cc0..16f3d308d8 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -379,7 +379,7 @@ static void terminfo_start(TUIData *tui) tui->out_isatty = os_isatty(tui->out_fd); tui->input.tui_data = tui; - const char *term = os_getenv("TERM"); + char *term = os_getenv("TERM"); #ifdef MSWIN os_tty_guess_term(&term, tui->out_fd); os_setenv("TERM", term, 1); @@ -401,23 +401,25 @@ static void terminfo_start(TUIData *tui) } // None of the following work over SSH; see :help TERM . - const char *colorterm = os_getenv("COLORTERM"); - const char *termprg = os_getenv("TERM_PROGRAM"); - const char *vte_version_env = os_getenv("VTE_VERSION"); + char *colorterm = os_getenv("COLORTERM"); + char *termprg = os_getenv("TERM_PROGRAM"); + char *vte_version_env = os_getenv("VTE_VERSION"); + char *konsolev_env = os_getenv("KONSOLE_VERSION"); + char *term_program_version_env = os_getenv("TERM_PROGRAM_VERSION"); + int vtev = vte_version_env ? (int)strtol(vte_version_env, NULL, 10) : 0; bool iterm_env = termprg && strstr(termprg, "iTerm.app"); bool nsterm = (termprg && strstr(termprg, "Apple_Terminal")) || terminfo_is_term_family(term, "nsterm"); bool konsole = terminfo_is_term_family(term, "konsole") - || os_getenv("KONSOLE_PROFILE_NAME") - || os_getenv("KONSOLE_DBUS_SESSION"); - const char *konsolev_env = os_getenv("KONSOLE_VERSION"); + || os_env_exists("KONSOLE_PROFILE_NAME", true) + || os_env_exists("KONSOLE_DBUS_SESSION", true); int konsolev = konsolev_env ? (int)strtol(konsolev_env, NULL, 10) : (konsole ? 1 : 0); bool wezterm = strequal(termprg, "WezTerm"); - const char *weztermv = wezterm ? os_getenv("TERM_PROGRAM_VERSION") : NULL; + const char *weztermv = wezterm ? term_program_version_env : NULL; bool screen = terminfo_is_term_family(term, "screen"); - bool tmux = terminfo_is_term_family(term, "tmux") || !!os_getenv("TMUX"); + bool tmux = terminfo_is_term_family(term, "tmux") || os_env_exists("TMUX", true); // truecolor support must be checked before patching/augmenting terminfo tui->rgb = term_has_truecolor(tui, colorterm); @@ -503,6 +505,13 @@ static void terminfo_start(TUIData *tui) } } flush_buf(tui); + + xfree(term); + xfree(colorterm); + xfree(termprg); + xfree(vte_version_env); + xfree(konsolev_env); + xfree(term_program_version_env); } /// Disable the alternate screen and prepare for the TUI to close. @@ -1772,6 +1781,8 @@ void tui_guess_size(TUIData *tui) { int width = 0; int height = 0; + char *lines = NULL; + char *columns = NULL; // 1 - try from a system call (ioctl/TIOCGWINSZ on unix) if (tui->out_isatty @@ -1782,9 +1793,9 @@ void tui_guess_size(TUIData *tui) // 2 - use $LINES/$COLUMNS if available const char *val; int advance; - if ((val = os_getenv("LINES")) + if ((val = os_getenv_noalloc("LINES")) && sscanf(val, "%d%n", &height, &advance) != EOF && advance - && (val = os_getenv("COLUMNS")) + && (val = os_getenv_noalloc("COLUMNS")) && sscanf(val, "%d%n", &width, &advance) != EOF && advance) { goto end; } @@ -1804,6 +1815,9 @@ void tui_guess_size(TUIData *tui) // Redraw on SIGWINCH event if size didn't change. #23411 ui_client_set_size(width, height); + + xfree(lines); + xfree(columns); } static void unibi_goto(TUIData *tui, int row, int col) @@ -1965,7 +1979,7 @@ static void patch_terminfo_bugs(TUIData *tui, const char *term, const char *colo int vte_version, int konsolev, bool iterm_env, bool nsterm) { unibi_term *ut = tui->ut; - const char *xterm_version = os_getenv("XTERM_VERSION"); + char *xterm_version = os_getenv("XTERM_VERSION"); bool xterm = terminfo_is_term_family(term, "xterm") // Treat Terminal.app as generic xterm-like, for now. || nsterm; @@ -1977,7 +1991,7 @@ static void patch_terminfo_bugs(TUIData *tui, const char *term, const char *colo bool teraterm = terminfo_is_term_family(term, "teraterm"); bool putty = terminfo_is_term_family(term, "putty"); bool screen = terminfo_is_term_family(term, "screen"); - bool tmux = terminfo_is_term_family(term, "tmux") || !!os_getenv("TMUX"); + bool tmux = terminfo_is_term_family(term, "tmux") || os_env_exists("TMUX", true); bool st = terminfo_is_term_family(term, "st"); bool gnome = terminfo_is_term_family(term, "gnome") || terminfo_is_term_family(term, "vte"); @@ -2276,6 +2290,8 @@ static void patch_terminfo_bugs(TUIData *tui, const char *term, const char *colo "\x1b]50;\x07"); } } + + xfree(xterm_version); } /// This adds stuff that is not in standard terminfo as extended unibilium @@ -2284,6 +2300,7 @@ static void augment_terminfo(TUIData *tui, const char *term, int vte_version, in const char *weztermv, bool iterm_env, bool nsterm) { unibi_term *ut = tui->ut; + char *xterm_version = os_getenv("XTERM_VERSION"); bool xterm = terminfo_is_term_family(term, "xterm") // Treat Terminal.app as generic xterm-like, for now. || nsterm; @@ -2294,7 +2311,7 @@ static void augment_terminfo(TUIData *tui, const char *term, int vte_version, in bool teraterm = terminfo_is_term_family(term, "teraterm"); bool putty = terminfo_is_term_family(term, "putty"); bool screen = terminfo_is_term_family(term, "screen"); - bool tmux = terminfo_is_term_family(term, "tmux") || !!os_getenv("TMUX"); + bool tmux = terminfo_is_term_family(term, "tmux") || os_env_exists("TMUX", true); bool st = terminfo_is_term_family(term, "st"); bool iterm = terminfo_is_term_family(term, "iterm") || terminfo_is_term_family(term, "iterm2") @@ -2305,7 +2322,6 @@ static void augment_terminfo(TUIData *tui, const char *term, int vte_version, in // None of the following work over SSH; see :help TERM . bool iterm_pretending_xterm = xterm && iterm_env; - const char *xterm_version = os_getenv("XTERM_VERSION"); bool true_xterm = xterm && !!xterm_version && !bsdvt; // Only define this capability for terminal types that we know understand it. @@ -2468,6 +2484,8 @@ static void augment_terminfo(TUIData *tui, const char *term, int vte_version, in // Kitty keyboard protocol tui->input.key_encoding = kKeyEncodingXterm; } + + xfree(xterm_version); } static bool should_invisible(TUIData *tui) diff --git a/src/nvim/ui_client.c b/src/nvim/ui_client.c index 44fc645a04..f5da11203e 100644 --- a/src/nvim/ui_client.c +++ b/src/nvim/ui_client.c @@ -63,7 +63,7 @@ uint64_t ui_client_start_server(int argc, char **argv) #ifdef MSWIN // TODO(justinmk): detach breaks `tt.setup_child_nvim` tests on Windows? - bool detach = os_env_exists("__NVIM_DETACH"); + bool detach = os_env_exists("__NVIM_DETACH", true); #else bool detach = true; #endif @@ -172,7 +172,7 @@ void ui_client_run(bool remote_ui) ui_client_attach(width, height, term, rgb); // TODO(justinmk): this is for log_spec. Can remove this after nvim_log #7062 is merged. - if (os_env_exists("__NVIM_TEST_LOG")) { + if (os_env_exists("__NVIM_TEST_LOG", true)) { ELOG("test log message"); } diff --git a/test/functional/core/env_spec.lua b/test/functional/core/env_spec.lua new file mode 100644 index 0000000000..4d5409248b --- /dev/null +++ b/test/functional/core/env_spec.lua @@ -0,0 +1,27 @@ +local t = require('test.testutil') +local n = require('test.functional.testnvim')() + +local eq = t.eq +local clear = n.clear +local exec_capture = n.exec_capture +local command = n.command + +describe('vim.uv', function() + before_each(function() + clear() + end) + + -- Subsequential env var assignment consistency + -- see: issue 32550 + it('vim.uv.os_setenv(), vim.uv.os_unsetenv() consistency', function() + eq('', exec_capture('echo $FOO')) + command('lua vim.uv.os_setenv("FOO", "bar")') + eq('bar', exec_capture('echo $FOO')) + command('lua vim.uv.os_setenv("FOO", "fizz")') + eq('fizz', exec_capture('echo $FOO')) + command('lua vim.uv.os_unsetenv("FOO")') + eq('', exec_capture('echo $FOO')) + command('lua vim.uv.os_setenv("FOO", "buzz")') + eq('buzz', exec_capture('echo $FOO')) + end) +end) diff --git a/test/unit/os/env_spec.lua b/test/unit/os/env_spec.lua index d8231545f3..a0dee35159 100644 --- a/test/unit/os/env_spec.lua +++ b/test/unit/os/env_spec.lua @@ -13,8 +13,8 @@ local OK = 0 local cimp = cimport('./src/nvim/os/os.h') describe('env.c', function() - local function os_env_exists(name) - return cimp.os_env_exists(to_cstr(name)) + local function os_env_exists(name, nonempty) + return cimp.os_env_exists(to_cstr(name), nonempty) end local function os_setenv(name, value, override) @@ -34,17 +34,45 @@ describe('env.c', function() end end - itp('os_env_exists', function() - eq(false, os_env_exists('')) - eq(false, os_env_exists(' ')) - eq(false, os_env_exists('\t')) - eq(false, os_env_exists('\n')) - eq(false, os_env_exists('AaあB <= very weird name...')) + local function os_getenv_noalloc(name) + local rval = cimp.os_getenv_noalloc(to_cstr(name)) + if rval ~= NULL then + return ffi.string(rval) + else + return NULL + end + end + + itp('os_env_exists(..., false)', function() + eq(false, os_env_exists('', false)) + eq(false, os_env_exists(' ', false)) + eq(false, os_env_exists('\t', false)) + eq(false, os_env_exists('\n', false)) + eq(false, os_env_exists('AaあB <= very weird name...', false)) local varname = 'NVIM_UNIT_TEST_os_env_exists' - eq(false, os_env_exists(varname)) + eq(false, os_env_exists(varname, false)) eq(OK, os_setenv(varname, 'foo bar baz ...', 1)) - eq(true, os_env_exists(varname)) + eq(true, os_env_exists(varname, false)) + eq(OK, os_setenv(varname, 'f', 1)) + eq(true, os_env_exists(varname, true)) + end) + + itp('os_env_exists(..., true)', function() + eq(false, os_env_exists('', true)) + eq(false, os_env_exists(' ', true)) + eq(false, os_env_exists('\t', true)) + eq(false, os_env_exists('\n', true)) + eq(false, os_env_exists('AaあB <= very weird name...', true)) + + local varname = 'NVIM_UNIT_TEST_os_env_defined' + eq(false, os_env_exists(varname, true)) + eq(OK, os_setenv(varname, '', 1)) + eq(false, os_env_exists(varname, true)) + eq(OK, os_setenv(varname, 'foo bar baz ...', 1)) + eq(true, os_env_exists(varname, true)) + eq(OK, os_setenv(varname, 'f', 1)) + eq(true, os_env_exists(varname, true)) end) describe('os_setenv', function() @@ -130,6 +158,10 @@ describe('env.c', function() os_setenv(name, value, 1) eq(value, os_getenv(name)) + -- Shortest non-empty value + os_setenv(name, 'z', 1) + eq('z', os_getenv(name)) + -- Get a big value. local bigval = ('x'):rep(256) eq(OK, os_setenv(name, bigval, 1)) @@ -147,6 +179,42 @@ describe('env.c', function() end) end) + describe('os_getenv_noalloc', function() + itp('reads an env var without memory allocation', function() + local name = 'NVIM_UNIT_TEST_GETENV_1N' + local value = 'NVIM_UNIT_TEST_GETENV_1V' + eq(NULL, os_getenv_noalloc(name)) + -- Use os_setenv because Lua doesn't have setenv. + os_setenv(name, value, 1) + eq(value, os_getenv_noalloc(name)) + + -- Shortest non-empty value + os_setenv(name, 'z', 1) + eq('z', os_getenv(name)) + + local bigval = ('x'):rep(256) + eq(OK, os_setenv(name, bigval, 1)) + eq(bigval, os_getenv_noalloc(name)) + + -- Variable size above NameBuff size gets truncated + -- This assumes MAXPATHL is 4096 bytes. + local verybigval = ('y'):rep(4096) + local trunc = string.sub(verybigval, 0, 4095) + eq(OK, os_setenv(name, verybigval, 1)) + eq(trunc, os_getenv_noalloc(name)) + + -- Set non-empty, then set empty. + eq(OK, os_setenv(name, 'non-empty', 1)) + eq('non-empty', os_getenv(name)) + eq(OK, os_setenv(name, '', 1)) + eq(NULL, os_getenv_noalloc(name)) + end) + + itp('returns NULL if the env var is not found', function() + eq(NULL, os_getenv_noalloc('NVIM_UNIT_TEST_GETENV_NOTFOUND')) + end) + end) + itp('os_unsetenv', function() local name = 'TEST_UNSETENV' local value = 'TESTVALUE' @@ -156,7 +224,7 @@ describe('env.c', function() -- Depending on the platform the var might be unset or set as '' assert.True(os_getenv(name) == nil or os_getenv(name) == '') if os_getenv(name) == nil then - eq(false, os_env_exists(name)) + eq(false, os_env_exists(name, false)) end end)