From 9153062095f34ec8dcdc8862da1ab9abfe560e3f Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sat, 27 Jan 2018 12:50:10 +0100 Subject: os_setenv: use _wputenv_s; remove vestigial code #7920 _putenv_s variant was left over from 810d31a43001, should have been removed in cd5b1315757e. --- config/config.h.in | 1 - src/nvim/os/env.c | 36 ++++++++++++++++++------------------ test/functional/eval/let_spec.lua | 16 +++++++++++++++- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/config/config.h.in b/config/config.h.in index 56d46e9f14..3f2f68da83 100644 --- a/config/config.h.in +++ b/config/config.h.in @@ -27,7 +27,6 @@ #cmakedefine HAVE_LOCALE_H #cmakedefine HAVE_NL_LANGINFO_CODESET #cmakedefine HAVE_NL_MSG_CAT_CNTR -#cmakedefine HAVE_PUTENV_S #cmakedefine HAVE_PWD_H #cmakedefine HAVE_READLINK #cmakedefine HAVE_UV_TRANSLATE_SYS_ERROR diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index c6794e4be5..6cf48eb814 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -52,29 +52,29 @@ int os_setenv(const char *name, const char *value, int overwrite) FUNC_ATTR_NONNULL_ALL { #ifdef WIN32 - size_t envbuflen = strlen(name) + strlen(value) + 2; - char *envbuf = xmalloc(envbuflen); - snprintf(envbuf, envbuflen, "%s=%s", name, value); - - wchar_t *p; - utf8_to_utf16(envbuf, &p); - xfree(envbuf); - if (p == NULL) { + if (!overwrite && os_getenv(name) != NULL) { + return 0; + } + wchar_t *wname; + utf8_to_utf16(name, &wname); + if (wname == NULL) { + return -1; + } + wchar_t *wvalue; + utf8_to_utf16(value, &wvalue); + if (wvalue == NULL) { + return -1; + } + int rv = (int)_wputenv_s(wname, wvalue); + xfree(wname); // Unlike unix putenv(), we can free after _wputenv_s(). + xfree(wvalue); + if (rv != 0) { + ELOG("_wputenv_s failed: %d: %s", rv, uv_strerror(rv)); return -1; } - _wputenv(p); - xfree(p); // Unlike Unix systems, we can free the string for _wputenv(). return 0; #elif defined(HAVE_SETENV) return setenv(name, value, overwrite); -#elif defined(HAVE_PUTENV_S) - if (!overwrite && os_getenv(name) != NULL) { - return 0; - } - if (_putenv_s(name, value) == 0) { - return 0; - } - return -1; #else # error "This system has no implementation available for os_setenv()" #endif diff --git a/test/functional/eval/let_spec.lua b/test/functional/eval/let_spec.lua index 1bd3405698..050cff3c22 100644 --- a/test/functional/eval/let_spec.lua +++ b/test/functional/eval/let_spec.lua @@ -2,13 +2,15 @@ local helpers = require('test.functional.helpers')(after_each) local eq = helpers.eq local clear = helpers.clear +local command = helpers.command +local eval = helpers.eval local meths = helpers.meths local redir_exec = helpers.redir_exec local source = helpers.source before_each(clear) -describe(':let command', function() +describe(':let', function() it('correctly lists variables with curly-braces', function() meths.set_var('v', {0}) eq('\nv [0]', redir_exec('let {"v"}')) @@ -42,4 +44,16 @@ describe(':let command', function() call feedkeys(":\e:echo l1 l3\n:echo 42\n:cq\n", "t") ]=]) end) + + it("sets environment variables", function() + local multibyte_multiline = [[\p* .ม .ม .ม .ม่ .ม่ .ม่ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ ֹֻ ֹֻ ֹֻ .ֹֻ .ֹֻ .ֹֻ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ a a a ca ca ca à à à]] + command("let $NVIM_TEST1 = 'AìaB'") + command("let $NVIM_TEST2 = 'AaあB'") + command("let $NVIM_TEST3 = '"..multibyte_multiline.."'") + eq('AìaB', eval('$NVIM_TEST1')) + eq('AaあB', eval('$NVIM_TEST2')) + eq(multibyte_multiline, eval('$NVIM_TEST3')) + end) end) -- cgit From 76562fa19269efb693d952568bccfbb65692dc8d Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sun, 28 Jan 2018 02:55:25 +0100 Subject: utf16_to_utf8: minor fixes --- src/nvim/mbyte.c | 3 ++- src/nvim/memory.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nvim/mbyte.c b/src/nvim/mbyte.c index ead6b4405d..1cf045d7e0 100644 --- a/src/nvim/mbyte.c +++ b/src/nvim/mbyte.c @@ -1375,6 +1375,7 @@ int utf8_to_utf16(const char *str, wchar_t **strw) int utf16_to_utf8(const wchar_t *strw, char **str) FUNC_ATTR_NONNULL_ALL { + *str = NULL; // Compute the space required to store the string as UTF-8. DWORD utf8_len = WideCharToMultiByte(CP_UTF8, 0, @@ -1400,7 +1401,7 @@ int utf16_to_utf8(const wchar_t *strw, char **str) NULL, NULL); if (utf8_len == 0) { - free(*str); + xfree(*str); *str = NULL; return GetLastError(); } diff --git a/src/nvim/memory.c b/src/nvim/memory.c index b49b521bc9..0f402611df 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -109,7 +109,7 @@ void *xmalloc(size_t size) return ret; } -/// free wrapper that returns delegates to the backing memory manager +/// free() wrapper that delegates to the backing memory manager void xfree(void *ptr) { free(ptr); -- cgit From 865584dd0c5b34530e44f03d4b42349a83cbca47 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sun, 28 Jan 2018 02:59:44 +0100 Subject: win: os_getenv(): use _wgetenv() --- src/nvim/os/env.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index 6cf48eb814..4f14e4eee3 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -36,8 +36,25 @@ const char *os_getenv(const char *name) FUNC_ATTR_NONNULL_ALL { +#if !defined(WIN32) const char *e = getenv(name); return e == NULL || *e == NUL ? NULL : e; +#else + wchar_t *wname; + utf8_to_utf16(name, &wname); + if (wname == NULL) { + xfree(wname); + return NULL; + } + wchar_t *wvalue = _wgetenv(wname); + char *value; + int rv = utf16_to_utf8(wvalue, &value); + if (rv != 0 || *value == NUL) { + xfree(value); + return NULL; + } + return value; // TODO(jmk): this was allocated, but callers don't free it ... +#endif } /// Returns `true` if the environment variable, `name`, has been defined -- cgit From 1d8e7683604828592bd41cdac5a351145cd93487 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sun, 28 Jan 2018 11:02:15 +0100 Subject: os_getenv, os_setenv: revert "widechar" impl It's reported that the Windows widechar variants do automatically convert from the current codepage to UTF16, which is very helpful. So the "widechar" impls are a good direction. But libuv v1.12 does that for us, so the next commit will use that instead. ref #8398 ref #9267 --- src/nvim/os/env.c | 33 ++------------------------------- test/functional/eval/let_spec.lua | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 41 deletions(-) diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index 4f14e4eee3..cbbd36dc8e 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -36,25 +36,8 @@ const char *os_getenv(const char *name) FUNC_ATTR_NONNULL_ALL { -#if !defined(WIN32) const char *e = getenv(name); return e == NULL || *e == NUL ? NULL : e; -#else - wchar_t *wname; - utf8_to_utf16(name, &wname); - if (wname == NULL) { - xfree(wname); - return NULL; - } - wchar_t *wvalue = _wgetenv(wname); - char *value; - int rv = utf16_to_utf8(wvalue, &value); - if (rv != 0 || *value == NUL) { - xfree(value); - return NULL; - } - return value; // TODO(jmk): this was allocated, but callers don't free it ... -#endif } /// Returns `true` if the environment variable, `name`, has been defined @@ -72,21 +55,9 @@ int os_setenv(const char *name, const char *value, int overwrite) if (!overwrite && os_getenv(name) != NULL) { return 0; } - wchar_t *wname; - utf8_to_utf16(name, &wname); - if (wname == NULL) { - return -1; - } - wchar_t *wvalue; - utf8_to_utf16(value, &wvalue); - if (wvalue == NULL) { - return -1; - } - int rv = (int)_wputenv_s(wname, wvalue); - xfree(wname); // Unlike unix putenv(), we can free after _wputenv_s(). - xfree(wvalue); + int rv = (int)_putenv_s(name, value); if (rv != 0) { - ELOG("_wputenv_s failed: %d: %s", rv, uv_strerror(rv)); + ELOG("_putenv_s failed: %d: %s", rv, uv_strerror(rv)); return -1; } return 0; diff --git a/test/functional/eval/let_spec.lua b/test/functional/eval/let_spec.lua index 050cff3c22..ff71daab74 100644 --- a/test/functional/eval/let_spec.lua +++ b/test/functional/eval/let_spec.lua @@ -45,15 +45,15 @@ describe(':let', function() ]=]) end) - it("sets environment variables", function() - local multibyte_multiline = [[\p* .ม .ม .ม .ม่ .ม่ .ม่ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ ֹֻ - .ֹֻ .ֹֻ .ֹֻ ֹֻ ֹֻ ֹֻ .ֹֻ .ֹֻ .ֹֻ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ - .ֹֻ .ֹֻ .ֹֻ a a a ca ca ca à à à]] - command("let $NVIM_TEST1 = 'AìaB'") - command("let $NVIM_TEST2 = 'AaあB'") - command("let $NVIM_TEST3 = '"..multibyte_multiline.."'") - eq('AìaB', eval('$NVIM_TEST1')) - eq('AaあB', eval('$NVIM_TEST2')) - eq(multibyte_multiline, eval('$NVIM_TEST3')) + it("multibyte environment variables", function() + command("let $NVIM_TEST = 'AìaB'") + eq('AìaB', eval('$NVIM_TEST')) + command("let $NVIM_TEST = 'AaあB'") + eq('AaあB', eval('$NVIM_TEST')) + local mbyte = [[\p* .ม .ม .ม .ม่ .ม่ .ม่ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ ֹֻ ֹֻ ֹֻ .ֹֻ .ֹֻ .ֹֻ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ a a a ca ca ca à à à]] + command("let $NVIM_TEST = '"..mbyte.."'") + eq(mbyte, eval('$NVIM_TEST')) end) end) -- cgit From 89515304e4eb81ff9eb65f3a582136fc658de139 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sun, 24 Feb 2019 20:09:14 +0100 Subject: os/env: use libuv v1.12 getenv/setenv API - Minimum required libuv is now v1.12 - Because `uv_os_getenv` requires allocating, we must manage a map (`envmap` in `env.c`) to maintain the old behavior of `os_getenv` . - free() map-items after removal. khash.h does not make copies of anything, so even its keys must be memory-managed by the caller. closes #8398 closes #9267 --- CMakeLists.txt | 2 +- config/CMakeLists.txt | 9 --- config/config.h.in | 2 - src/nvim/hashtab.h | 2 +- src/nvim/lib/khash.h | 30 +++++----- src/nvim/main.c | 7 ++- src/nvim/map.c | 32 ++++++++++ src/nvim/map.h | 9 +++ src/nvim/os/env.c | 129 +++++++++++++++++++++++++++++++---------- src/nvim/os/fs_defs.h | 4 +- src/nvim/os/pty_process_unix.c | 2 +- src/nvim/path.c | 2 +- test/unit/helpers.lua | 12 ++-- test/unit/os/env_spec.lua | 79 ++++++++++++++++++------- 14 files changed, 226 insertions(+), 95 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 988c69cd2d..848e100b02 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -379,7 +379,7 @@ endif() include_directories("${PROJECT_BINARY_DIR}/config") include_directories("${PROJECT_SOURCE_DIR}/src") -find_package(LibUV REQUIRED) +find_package(LibUV REQUIRED) # minimum version: v1.12 include_directories(SYSTEM ${LIBUV_INCLUDE_DIRS}) find_package(Msgpack 1.0.0 REQUIRED) diff --git a/config/CMakeLists.txt b/config/CMakeLists.txt index 442d91524b..91c1e7c629 100644 --- a/config/CMakeLists.txt +++ b/config/CMakeLists.txt @@ -47,17 +47,8 @@ if(Iconv_FOUND) set(HAVE_ICONV 1) endif() -check_function_exists(_putenv_s HAVE_PUTENV_S) -if(WIN32 AND NOT HAVE_PUTENV_S) - message(SEND_ERROR "_putenv_s() function not found on your system.") -endif() check_function_exists(opendir HAVE_OPENDIR) check_function_exists(readlink HAVE_READLINK) -check_function_exists(setenv HAVE_SETENV) -if(UNIX AND NOT HAVE_SETENV) - message(SEND_ERROR "setenv() function not found on your system.") -endif() -check_function_exists(unsetenv HAVE_UNSETENV) check_function_exists(setpgid HAVE_SETPGID) check_function_exists(setsid HAVE_SETSID) check_function_exists(sigaction HAVE_SIGACTION) diff --git a/config/config.h.in b/config/config.h.in index 3f2f68da83..ad636632f4 100644 --- a/config/config.h.in +++ b/config/config.h.in @@ -32,8 +32,6 @@ #cmakedefine HAVE_UV_TRANSLATE_SYS_ERROR // TODO: add proper cmake check // #define HAVE_SELINUX 1 -#cmakedefine HAVE_SETENV -#cmakedefine HAVE_UNSETENV #cmakedefine HAVE_SETPGID #cmakedefine HAVE_SETSID #cmakedefine HAVE_SIGACTION diff --git a/src/nvim/hashtab.h b/src/nvim/hashtab.h index 973b97d476..a70a8bea63 100644 --- a/src/nvim/hashtab.h +++ b/src/nvim/hashtab.h @@ -19,7 +19,7 @@ typedef size_t hash_T; #define HASHITEM_EMPTY(hi) ((hi)->hi_key == NULL \ || (hi)->hi_key == (char_u *)&hash_removed) -/// A hastable item. +/// Hashtable item. /// /// Each item has a NUL terminated string key. /// A key can appear only once in the table. diff --git a/src/nvim/lib/khash.h b/src/nvim/lib/khash.h index 8287cb14da..b2994a3159 100644 --- a/src/nvim/lib/khash.h +++ b/src/nvim/lib/khash.h @@ -24,24 +24,24 @@ */ /* - An example: + Example: #include "nvim/khash.h" KHASH_MAP_INIT_INT(32, char) int main() { - int ret, is_missing; - khiter_t k; - khash_t(32) *h = kh_init(32); - k = kh_put(32, h, 5, &ret); - kh_value(h, k) = 10; - k = kh_get(32, h, 10); - is_missing = (k == kh_end(h)); - k = kh_get(32, h, 5); - kh_del(32, h, k); - for (k = kh_begin(h); k != kh_end(h); ++k) - if (kh_exist(h, k)) kh_value(h, k) = 1; - kh_destroy(32, h); - return 0; + int ret, is_missing; + khiter_t k; + khash_t(32) *h = kh_init(32); + k = kh_put(32, h, 5, &ret); + kh_value(h, k) = 10; + k = kh_get(32, h, 10); + is_missing = (k == kh_end(h)); + k = kh_get(32, h, 5); + kh_del(32, h, k); + for (k = kh_begin(h); k != kh_end(h); ++k) + if (kh_exist(h, k)) kh_value(h, k) = 1; + kh_destroy(32, h); + return 0; } */ @@ -539,7 +539,7 @@ static kh_inline khint_t __ac_Wang_hash(khint_t key) @param r Extra return code: -1 if the operation failed; 0 if the key is present in the hash table; 1 if the bucket is empty (never used); 2 if the element in - the bucket has been deleted [int*] + the bucket has been deleted [int*] @return Iterator to the inserted element [khint_t] */ #define kh_put(name, h, k, r) kh_put_##name(h, k, r) diff --git a/src/nvim/main.c b/src/nvim/main.c index 251a54ad5b..79513b1c21 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -184,6 +184,7 @@ bool event_teardown(void) void early_init(void) { log_init(); + env_init(); fs_init(); handle_init(); eval_init(); // init global variables @@ -1769,7 +1770,7 @@ static bool do_user_initialization(void) FUNC_ATTR_WARN_UNUSED_RESULT { bool do_exrc = p_exrc; - if (process_env("VIMINIT") == OK) { + if (execute_env("VIMINIT") == OK) { do_exrc = p_exrc; return do_exrc; } @@ -1814,7 +1815,7 @@ static bool do_user_initialization(void) } while (iter != NULL); xfree(config_dirs); } - if (process_env("EXINIT") == OK) { + if (execute_env("EXINIT") == OK) { do_exrc = p_exrc; return do_exrc; } @@ -1878,7 +1879,7 @@ static void source_startup_scripts(const mparm_T *const parmp) /// /// @return FAIL if the environment variable was not executed, /// OK otherwise. -static int process_env(char *env) +static int execute_env(char *env) FUNC_ATTR_NONNULL_ALL { const char *initstr = os_getenv(env); diff --git a/src/nvim/map.c b/src/nvim/map.c index 53ab734802..9b6f57a56f 100644 --- a/src/nvim/map.c +++ b/src/nvim/map.c @@ -1,6 +1,13 @@ // This is an open source non-commercial project. Dear PVS-Studio, please check // it. PVS-Studio Static Code Analyzer for C, C++ and C#: http://www.viva64.com +/// +/// map.c: khash.h wrapper +/// +/// NOTE: Callers must manage memory (allocate) for keys and values. +/// khash.h does not make its own copy of the key or value. +/// + #include #include #include @@ -72,6 +79,16 @@ return kh_get(T##_##U##_map, map->table, key) != kh_end(map->table); \ } \ \ + T map_##T##_##U##_key(Map(T, U) *map, T key) \ + { \ + khiter_t k; \ + \ + if ((k = kh_get(T##_##U##_map, map->table, key)) == kh_end(map->table)) { \ + abort(); /* Caller must check map_has(). */ \ + } \ + \ + return kh_key(map->table, k); \ + } \ U map_##T##_##U##_put(Map(T, U) *map, T key, U value) \ { \ int ret; \ @@ -167,3 +184,18 @@ MAP_IMPL(String, MsgpackRpcRequestHandler, MSGPACK_HANDLER_INITIALIZER) #define KVEC_INITIALIZER { .size = 0, .capacity = 0, .items = NULL } MAP_IMPL(HlEntry, int, DEFAULT_INITIALIZER) MAP_IMPL(String, handle_T, 0) + + +/// Deletes a key:value pair from a string:pointer map, and frees the +/// storage of both key and value. +/// +void pmap_del2(PMap(cstr_t) *map, const char *key) +{ + if (pmap_has(cstr_t)(map, key)) { + void *k = (void *)pmap_key(cstr_t)(map, key); + void *v = pmap_get(cstr_t)(map, key); + pmap_del(cstr_t)(map, key); + xfree(k); + xfree(v); + } +} diff --git a/src/nvim/map.h b/src/nvim/map.h index 0e4308b953..75ab64cca4 100644 --- a/src/nvim/map.h +++ b/src/nvim/map.h @@ -25,11 +25,15 @@ void map_##T##_##U##_free(Map(T, U) *map); \ U map_##T##_##U##_get(Map(T, U) *map, T key); \ bool map_##T##_##U##_has(Map(T, U) *map, T key); \ + T map_##T##_##U##_key(Map(T, U) *map, T key); \ U map_##T##_##U##_put(Map(T, U) *map, T key, U value); \ U *map_##T##_##U##_ref(Map(T, U) *map, T key, bool put); \ U map_##T##_##U##_del(Map(T, U) *map, T key); \ void map_##T##_##U##_clear(Map(T, U) *map); +// +// NOTE: Keys AND values must be allocated! khash.h does not make a copy. +// MAP_DECLS(int, int) MAP_DECLS(cstr_t, ptr_t) MAP_DECLS(ptr_t, ptr_t) @@ -43,6 +47,7 @@ MAP_DECLS(String, handle_T) #define map_free(T, U) map_##T##_##U##_free #define map_get(T, U) map_##T##_##U##_get #define map_has(T, U) map_##T##_##U##_has +#define map_key(T, U) map_##T##_##U##_key #define map_put(T, U) map_##T##_##U##_put #define map_ref(T, U) map_##T##_##U##_ref #define map_del(T, U) map_##T##_##U##_del @@ -52,7 +57,9 @@ MAP_DECLS(String, handle_T) #define pmap_free(T) map_free(T, ptr_t) #define pmap_get(T) map_get(T, ptr_t) #define pmap_has(T) map_has(T, ptr_t) +#define pmap_key(T) map_key(T, ptr_t) #define pmap_put(T) map_put(T, ptr_t) +/// @see pmap_del2 #define pmap_del(T) map_del(T, ptr_t) #define pmap_clear(T) map_clear(T, ptr_t) @@ -62,4 +69,6 @@ MAP_DECLS(String, handle_T) #define map_foreach_value(map, value, block) \ kh_foreach_value(map->table, value, block) +void pmap_del2(PMap(cstr_t) *map, const char *key); + #endif // NVIM_MAP_H diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index cbbd36dc8e..8a8220b6e7 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -19,6 +19,7 @@ #include "nvim/eval.h" #include "nvim/ex_getln.h" #include "nvim/version.h" +#include "nvim/map.h" #ifdef WIN32 #include "nvim/mbyte.h" // for utf8_to_utf16, utf16_to_utf8 @@ -32,53 +33,119 @@ #include #endif +// Because `uv_os_getenv` requires allocating, we must manage a map to maintain +// the behavior of `os_getenv`. +static PMap(cstr_t) *envmap; +static uv_mutex_t mutex; + +void env_init(void) +{ + envmap = pmap_new(cstr_t)(); + uv_mutex_init(&mutex); +} + /// Like getenv(), but returns NULL if the variable is empty. const char *os_getenv(const char *name) FUNC_ATTR_NONNULL_ALL { - const char *e = getenv(name); - return e == NULL || *e == NUL ? NULL : e; + char *e; + size_t size = 64; + if (name[0] == '\0') { + return NULL; + } + uv_mutex_lock(&mutex); + if (pmap_has(cstr_t)(envmap, name) + && !!(e = (char *)pmap_get(cstr_t)(envmap, name))) { + if (e[0] != '\0') { + // 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); + } + e = xmalloc(size); + int r = uv_os_getenv(name, e, &size); + if (r == UV_ENOBUFS) { + e = xrealloc(e, size); + r = uv_os_getenv(name, e, &size); + } + if (r != 0 || size == 0 || e[0] == '\0') { + xfree(e); + e = NULL; + if (r != 0 && r != UV_ENOENT && r != UV_UNKNOWN) { + ELOG("uv_os_getenv(%s) failed: %d %s", name, r, uv_err_name(r)); + } + goto end; + } + pmap_put(cstr_t)(envmap, xstrdup(name), e); +end: + uv_mutex_unlock(&mutex); + return (e == NULL || size == 0 || e[0] == '\0') ? NULL : e; } -/// Returns `true` if the environment variable, `name`, has been defined -/// (even if empty). +/// 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) FUNC_ATTR_NONNULL_ALL { - return getenv(name) != NULL; + if (name[0] == '\0') { + return false; + } + // 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]; + 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); } int os_setenv(const char *name, const char *value, int overwrite) FUNC_ATTR_NONNULL_ALL { + if (name[0] == '\0') { + return -1; + } #ifdef WIN32 if (!overwrite && os_getenv(name) != NULL) { return 0; } - int rv = (int)_putenv_s(name, value); - if (rv != 0) { - ELOG("_putenv_s failed: %d: %s", rv, uv_strerror(rv)); - return -1; - } - return 0; -#elif defined(HAVE_SETENV) - return setenv(name, value, overwrite); #else -# error "This system has no implementation available for os_setenv()" + if (!overwrite && os_env_exists(name)) { + return 0; + } #endif + uv_mutex_lock(&mutex); + pmap_del2(envmap, name); + int r = uv_os_setenv(name, value); + assert(r != UV_EINVAL); + if (r != 0) { + ELOG("uv_os_setenv(%s) failed: %d %s", name, r, uv_err_name(r)); + } + uv_mutex_unlock(&mutex); + return r == 0 ? 0 : -1; } /// Unset environment variable -/// -/// For systems where unsetenv() is not available the value will be set as an -/// empty string int os_unsetenv(const char *name) + FUNC_ATTR_NONNULL_ALL { -#ifdef HAVE_UNSETENV - return unsetenv(name); -#else - return os_setenv(name, "", 1); -#endif + if (name[0] == '\0') { + return -1; + } + uv_mutex_lock(&mutex); + 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)); + } + uv_mutex_unlock(&mutex); + return r == 0 ? 0 : -1; } char *os_getenvname_at_index(size_t index) @@ -515,7 +582,7 @@ static char *remove_tail(char *path, char *pend, char *dirname) return pend; } -/// Iterate over a delimited list. +/// Iterates $PATH-like delimited list `val`. /// /// @note Environment variables must not be modified during iteration. /// @@ -550,7 +617,7 @@ const void *vim_env_iter(const char delim, } } -/// Iterate over a delimited list in reverse order. +/// Iterates $PATH-like delimited list `val` in reverse order. /// /// @note Environment variables must not be modified during iteration. /// @@ -587,11 +654,12 @@ const void *vim_env_iter_rev(const char delim, } } -/// Vim's version of getenv(). -/// Special handling of $HOME, $VIM and $VIMRUNTIME, allowing the user to -/// override the vim runtime directory at runtime. Also does ACP to 'enc' -/// conversion for Win32. Result must be freed by the caller. +/// Vim getenv() wrapper with special handling of $HOME, $VIM, $VIMRUNTIME, +/// allowing the user to override the Nvim runtime directory at runtime. +/// Result must be freed by the caller. +/// /// @param name Environment variable to expand +/// @return [allocated] Expanded environment variable, or NULL char *vim_getenv(const char *name) { // init_path() should have been called before now. @@ -857,9 +925,8 @@ char_u * home_replace_save(buf_T *buf, char_u *src) FUNC_ATTR_NONNULL_RET return dst; } -/// Our portable version of setenv. -/// Has special handling for $VIMRUNTIME to keep the localization machinery -/// sane. +/// Vim setenv() wrapper with special handling for $VIMRUNTIME to keep the +/// localization machinery sane. void vim_setenv(const char *name, const char *val) { os_setenv(name, val, 1); diff --git a/src/nvim/os/fs_defs.h b/src/nvim/os/fs_defs.h index 2277d926b3..c9b148a5aa 100644 --- a/src/nvim/os/fs_defs.h +++ b/src/nvim/os/fs_defs.h @@ -21,9 +21,7 @@ typedef struct { uv_dirent_t ent; ///< @private The entry information. } Directory; -/// Function to convert libuv error to char * error description -/// -/// negative libuv error codes are returned by a number of os functions. +/// Converts libuv error (negative int) to error description string. #define os_strerror uv_strerror // Values returned by os_nodetype() diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index bcf57e1b5b..9ff0399511 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -177,7 +177,7 @@ static void init_child(PtyProcess *ptyproc) } char *prog = ptyproc->process.argv[0]; - setenv("TERM", ptyproc->term_name ? ptyproc->term_name : "ansi", 1); + os_setenv("TERM", ptyproc->term_name ? ptyproc->term_name : "ansi", 1); execvp(prog, ptyproc->process.argv); ELOG("execvp failed: %s: %s", strerror(errno), prog); _exit(122); // 122 is EXEC_FAILED in the Vim source. diff --git a/src/nvim/path.c b/src/nvim/path.c index 7903e3f4f4..a706e32773 100644 --- a/src/nvim/path.c +++ b/src/nvim/path.c @@ -2271,7 +2271,7 @@ int path_is_absolute(const char_u *fname) void path_guess_exepath(const char *argv0, char *buf, size_t bufsize) FUNC_ATTR_NONNULL_ALL { - char *path = getenv("PATH"); + const char *path = os_getenv("PATH"); if (path == NULL || path_is_absolute((char_u *)argv0)) { xstrlcpy(buf, argv0, bufsize); diff --git a/test/unit/helpers.lua b/test/unit/helpers.lua index f8143a0125..beb25f25db 100644 --- a/test/unit/helpers.lua +++ b/test/unit/helpers.lua @@ -645,16 +645,16 @@ local function itp_child(wr, func) s = s:sub(1, hook_msglen - 2) sc.write(wr, '>' .. s .. (' '):rep(hook_msglen - 2 - #s) .. '\n') end - local err, emsg = pcall(init) - if err then + local status, result = pcall(init) + if status then collectgarbage('stop') child_sethook(wr) - err, emsg = pcall(func) + status, result = pcall(func) debug.sethook() end - emsg = tostring(emsg) sc.write(wr, trace_end_msg) - if not err then + if not status then + local emsg = tostring(result) if #emsg > 99999 then emsg = emsg:sub(1, 99999) end @@ -668,7 +668,7 @@ local function itp_child(wr, func) collectgarbage() sc.write(wr, '$\n') sc.close(wr) - sc.exit(err and 0 or 1) + sc.exit(status and 0 or 1) end local function check_child_err(rd) diff --git a/test/unit/os/env_spec.lua b/test/unit/os/env_spec.lua index c54d5a9b77..c543551607 100644 --- a/test/unit/os/env_spec.lua +++ b/test/unit/os/env_spec.lua @@ -8,17 +8,22 @@ local ffi = helpers.ffi local cstr = helpers.cstr local to_cstr = helpers.to_cstr local NULL = helpers.NULL +local OK = 0 require('lfs') 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)) + end + local function os_setenv(name, value, override) return cimp.os_setenv(to_cstr(name), to_cstr(value), override) end - local function os_unsetenv(name, _, _) + local function os_unsetenv(name) return cimp.os_unsetenv(to_cstr(name)) end @@ -31,25 +36,44 @@ describe('env.c', function() end end - describe('os_setenv', function() - local OK = 0 + 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...')) - itp('sets an env variable and returns OK', function() + local varname = 'NVIM_UNIT_TEST_os_env_exists' + eq(false, os_env_exists(varname)) + eq(OK, os_setenv(varname, 'foo bar baz ...', 1)) + eq(true, os_env_exists(varname)) + end) + + describe('os_setenv', function() + itp('sets an env var and returns success', function() local name = 'NVIM_UNIT_TEST_SETENV_1N' local value = 'NVIM_UNIT_TEST_SETENV_1V' eq(nil, os.getenv(name)) - eq(OK, (os_setenv(name, value, 1))) + eq(OK, os_setenv(name, value, 1)) eq(value, os.getenv(name)) + + -- Set empty, then set non-empty, then retrieve. + eq(OK, os_setenv(name, '', 1)) + eq('', os.getenv(name)) + eq(OK, os_setenv(name, 'non-empty', 1)) + eq('non-empty', os.getenv(name)) end) - itp("dosn't overwrite an env variable if overwrite is 0", function() + itp("`overwrite` behavior", function() local name = 'NVIM_UNIT_TEST_SETENV_2N' local value = 'NVIM_UNIT_TEST_SETENV_2V' local value_updated = 'NVIM_UNIT_TEST_SETENV_2V_UPDATED' - eq(OK, (os_setenv(name, value, 0))) + eq(OK, os_setenv(name, value, 0)) eq(value, os.getenv(name)) - eq(OK, (os_setenv(name, value_updated, 0))) + eq(OK, os_setenv(name, value_updated, 0)) eq(value, os.getenv(name)) + eq(OK, os_setenv(name, value_updated, 1)) + eq(value_updated, os.getenv(name)) end) end) @@ -93,31 +117,42 @@ describe('env.c', function() end) describe('os_getenv', function() - itp('reads an env variable', function() + itp('reads an env var', function() local name = 'NVIM_UNIT_TEST_GETENV_1N' local value = 'NVIM_UNIT_TEST_GETENV_1V' eq(NULL, os_getenv(name)) -- Use os_setenv because Lua dosen't have setenv. os_setenv(name, value, 1) eq(value, os_getenv(name)) + + -- Get a big value. + local bigval = ('x'):rep(256) + eq(OK, os_setenv(name, bigval, 1)) + eq(bigval, os_getenv(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(name)) end) - itp('returns NULL if the env variable is not found', function() - local name = 'NVIM_UNIT_TEST_GETENV_NOTFOUND' - return eq(NULL, os_getenv(name)) + itp('returns NULL if the env var is not found', function() + eq(NULL, os_getenv('NVIM_UNIT_TEST_GETENV_NOTFOUND')) end) end) - describe('os_unsetenv', function() - itp('unsets environment variable', function() - local name = 'TEST_UNSETENV' - local value = 'TESTVALUE' - os_setenv(name, value, 1) - os_unsetenv(name) - neq(os_getenv(name), value) - -- Depending on the platform the var might be unset or set as '' - assert.True(os_getenv(name) == nil or os_getenv(name) == '') - end) + itp('os_unsetenv', function() + local name = 'TEST_UNSETENV' + local value = 'TESTVALUE' + os_setenv(name, value, 1) + eq(OK, os_unsetenv(name)) + neq(os_getenv(name), value) + -- 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)) + end end) describe('os_getenvname_at_index', function() -- cgit From 900e96781f09f5e4d6b89be07391b35fcec1d1f4 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 26 Feb 2019 02:43:06 +0100 Subject: clint: check env functions --- src/clint.py | 38 +++++++++++++++++++++++++++++++++++--- src/nvim/os/pty_process_unix.c | 10 +++++----- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/clint.py b/src/clint.py index 34af5d15fd..1ef31820ee 100755 --- a/src/clint.py +++ b/src/clint.py @@ -29,10 +29,10 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -"""Does neovim-lint on c files. +"""Lints C files in the Neovim source tree. The goal of this script is to identify places in the code that *may* -be in non-compliance with neovim style. It does not attempt to fix +be in non-compliance with Neovim style. It does not attempt to fix up these problems -- the point is to educate. It does also not attempt to find all problems, or to ensure that everything it does find is legitimately a problem. @@ -88,7 +88,7 @@ Syntax: clint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] * [whitespace/braces] { should almost always be at the end of the previous line * [build/include] Include the directory when naming .h files - * [runtime/int] Use int16/int64/etc, rather than the C type. + * [runtime/int] Use int16_t/int64_t/etc, rather than the C type. Every problem is given a confidence score from 1-5, with 5 meaning we are certain of the problem, and 1 meaning it could be a legitimate construct. @@ -1487,6 +1487,37 @@ def CheckMemoryFunctions(filename, clean_lines, linenum, error): '...) instead of ' + function + '...).') +os_functions = ( + ('setenv(', 'os_setenv('), + ('getenv(', 'os_getenv('), + ('_wputenv(', 'os_setenv('), + ('_putenv_s(', 'os_setenv('), + ('putenv(', 'os_setenv('), + ('unsetenv(', 'os_unsetenv('), +) + + +def CheckOSFunctions(filename, clean_lines, linenum, error): + """Checks for calls to invalid functions. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + for function, suggested_function in os_functions: + ix = line.find(function) + # Comparisons made explicit for clarity -- pylint: + # disable=g-explicit-bool-comparison + if ix >= 0 and (ix == 0 or (not line[ix - 1].isalnum() and + line[ix - 1] not in ('_', '.', '>'))): + error(filename, linenum, 'runtime/os_fn', 2, + 'Use ' + suggested_function + + '...) instead of ' + function + '...).') + + # Matches invalid increment: *count++, which moves pointer instead of # incrementing a value. _RE_PATTERN_INVALID_INCREMENT = re.compile( @@ -3370,6 +3401,7 @@ def ProcessLine(filename, file_extension, clean_lines, line, nesting_state, error) CheckPosixThreading(filename, clean_lines, line, error) CheckMemoryFunctions(filename, clean_lines, line, error) + CheckOSFunctions(filename, clean_lines, line, error) for check_fn in extra_check_functions: check_fn(filename, clean_lines, line, error) diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index 9ff0399511..5fdf0e6181 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -157,11 +157,11 @@ static void init_child(PtyProcess *ptyproc) // New session/process-group. #6530 setsid(); - unsetenv("COLUMNS"); - unsetenv("LINES"); - unsetenv("TERMCAP"); - unsetenv("COLORTERM"); - unsetenv("COLORFGBG"); + os_unsetenv("COLUMNS"); + os_unsetenv("LINES"); + os_unsetenv("TERMCAP"); + os_unsetenv("COLORTERM"); + os_unsetenv("COLORFGBG"); signal(SIGCHLD, SIG_DFL); signal(SIGHUP, SIG_DFL); -- cgit From 67535b5940b70de327d1a9ce6af4a311406eb62f Mon Sep 17 00:00:00 2001 From: erw7 Date: Thu, 28 Feb 2019 08:34:10 +0100 Subject: test/env: multibyte env var to child process Note: the test fails on non-Windows CI (Travis linux, Quickbuild bsd): even on master before the env.c changes in this patch-series. Maybe the unix part of printenv-test.c needs to be revisited. Signed-off-by: Justin M. Keyes --- test/functional/eval/let_spec.lua | 25 +++++++++++++- test/functional/fixtures/CMakeLists.txt | 4 +++ test/functional/fixtures/printenv-test.c | 59 ++++++++++++++++++++++++++++++++ test/helpers.lua | 1 + 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 test/functional/fixtures/printenv-test.c diff --git a/test/functional/eval/let_spec.lua b/test/functional/eval/let_spec.lua index ff71daab74..0cbf40137e 100644 --- a/test/functional/eval/let_spec.lua +++ b/test/functional/eval/let_spec.lua @@ -7,6 +7,7 @@ local eval = helpers.eval local meths = helpers.meths local redir_exec = helpers.redir_exec local source = helpers.source +local nvim_dir = helpers.nvim_dir before_each(clear) @@ -45,7 +46,7 @@ describe(':let', function() ]=]) end) - it("multibyte environment variables", function() + it("multibyte env var #8398 #9267", function() command("let $NVIM_TEST = 'AìaB'") eq('AìaB', eval('$NVIM_TEST')) command("let $NVIM_TEST = 'AaあB'") @@ -56,4 +57,26 @@ describe(':let', function() command("let $NVIM_TEST = '"..mbyte.."'") eq(mbyte, eval('$NVIM_TEST')) end) + + it("multibyte env var to child process #8398 #9267", function() + if (not helpers.iswin()) and require('test.helpers').isCI() then + -- Fails on non-Windows CI. Buffering/timing issue? + pending('fails on unix CI', function() end) + end + local cmd_get_child_env = "let g:env_from_child = system(['"..nvim_dir.."/printenv-test', 'NVIM_TEST'])" + command("let $NVIM_TEST = 'AìaB'") + command(cmd_get_child_env) + eq(eval('$NVIM_TEST'), eval('g:env_from_child')) + + command("let $NVIM_TEST = 'AaあB'") + command(cmd_get_child_env) + eq(eval('$NVIM_TEST'), eval('g:env_from_child')) + + local mbyte = [[\p* .ม .ม .ม .ม่ .ม่ .ม่ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ ֹֻ ֹֻ ֹֻ .ֹֻ .ֹֻ .ֹֻ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹ ֹ ֹ .ֹ .ֹ .ֹ ֹֻ ֹֻ + .ֹֻ .ֹֻ .ֹֻ a a a ca ca ca à à à]] + command("let $NVIM_TEST = '"..mbyte.."'") + command(cmd_get_child_env) + eq(eval('$NVIM_TEST'), eval('g:env_from_child')) + end) end) diff --git a/test/functional/fixtures/CMakeLists.txt b/test/functional/fixtures/CMakeLists.txt index 8537ea390f..a7cd214b6b 100644 --- a/test/functional/fixtures/CMakeLists.txt +++ b/test/functional/fixtures/CMakeLists.txt @@ -3,3 +3,7 @@ target_link_libraries(tty-test ${LIBUV_LIBRARIES}) add_executable(shell-test shell-test.c) add_executable(printargs-test printargs-test.c) +add_executable(printenv-test printenv-test.c) +if(WIN32) + set_target_properties(printenv-test PROPERTIES LINK_FLAGS -municode) +endif() diff --git a/test/functional/fixtures/printenv-test.c b/test/functional/fixtures/printenv-test.c new file mode 100644 index 0000000000..5ac076f653 --- /dev/null +++ b/test/functional/fixtures/printenv-test.c @@ -0,0 +1,59 @@ +// This is an open source non-commercial project. Dear PVS-Studio, please check +// it. PVS-Studio Static Code Analyzer for C, C++ and C#: http://www.viva64.com + +#include + +#ifdef WIN32 +# include +#else +# include +#endif + +#ifdef WIN32 +int wmain(int argc, wchar_t **argv) +#else +int main(int argc, char **argv) +#endif +{ + if (argc != 2) { + return 1; + } + +#ifdef WIN32 + wchar_t *value = _wgetenv(argv[1]); + if (value == NULL) { + return 1; + } + int utf8_len = WideCharToMultiByte(CP_UTF8, + 0, + value, + -1, + NULL, + 0, + NULL, + NULL); + if (utf8_len == 0) { + return (int)GetLastError(); + } + char *utf8_value = (char *)calloc((size_t)utf8_len, sizeof(char)); + utf8_len = WideCharToMultiByte(CP_UTF8, + 0, + value, + -1, + utf8_value, + utf8_len, + NULL, + NULL); + fprintf(stderr, "%s", utf8_value); + free(utf8_value); +#else + char *value = getenv(argv[1]); + if (value == NULL) { + fprintf(stderr, "env var not found: %s", argv[1]); + return 1; + } + // Print to stderr to avoid buffering. + fprintf(stderr, "%s", value); +#endif + return 0; +} diff --git a/test/helpers.lua b/test/helpers.lua index 59da274e87..7c3654680a 100644 --- a/test/helpers.lua +++ b/test/helpers.lua @@ -751,6 +751,7 @@ local module = { hasenv = hasenv, hexdump = hexdump, intchar2lua = intchar2lua, + isCI = isCI, map = map, matches = matches, mergedicts_copy = mergedicts_copy, -- cgit From 403922b1b47c1f03272e2bdd600dcc02db481389 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Fri, 1 Mar 2019 01:48:21 +0100 Subject: test: fix isCI() for Quickbuild --- test/helpers.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers.lua b/test/helpers.lua index 7c3654680a..9f998ef919 100644 --- a/test/helpers.lua +++ b/test/helpers.lua @@ -708,7 +708,7 @@ end local function isCI() local is_travis = nil ~= os.getenv('TRAVIS') local is_appveyor = nil ~= os.getenv('APPVEYOR') - local is_quickbuild = nil ~= os.getenv('PR_NUMBER') + local is_quickbuild = nil ~= lfs.attributes('/usr/home/quickbuild') return is_travis or is_appveyor or is_quickbuild end -- cgit