[PATCH v3 0/4] MR4313: MSVCRT _(w)environ[] has a separate allocation chunk for each entry.
_(w)environ[] do have a distinct allocation chunk for each entry, so that _(w)environ[i] pointer and (pointed) string don't change when updating/deleting any other entry. Proposed implementation still differs from native: - allocation is done on process heap, while native uses msvcrt's heap - first ANSI allocated _environ[] doesn't have per entry allocation. This is only activated after a change (update/deletion) to _environ[] is made. -- v3: msvcrt: Let each _environ[] entry have a distinct allocation block. msvcrt/tests: Add tests about intricating kernel32 and msvcrt env calls. msvcrt/tests: Add tests about allocation of environment entries. msvcrt: Improve environment tests (initial conditions). https://gitlab.winehq.org/wine/wine/-/merge_requests/4313
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/tests/environ.c | 170 ++++++++++++++++++++++++------------ 1 file changed, 113 insertions(+), 57 deletions(-) diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 22f697a12ea..dd0765edc55 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -46,6 +46,8 @@ static const char *a_very_long_env_string = static char ***(__cdecl *p__p__environ)(void); static WCHAR ***(__cdecl *p__p__wenviron)(void); +static char ***(__cdecl *p__p___initenv)(void); +static wchar_t ***(__cdecl *p__p___winitenv)(void); static void (__cdecl *p_get_environ)(char ***); static void (__cdecl *p_get_wenviron)(WCHAR ***); static errno_t (__cdecl *p_putenv_s)(const char*, const char*); @@ -61,6 +63,8 @@ static void init(void) p__p__environ = (void *)GetProcAddress(hmod, "__p__environ"); p__p__wenviron = (void *)GetProcAddress(hmod, "__p__wenviron"); + p__p___initenv = (void *)GetProcAddress(hmod, "__p___initenv"); + p__p___winitenv = (void *)GetProcAddress(hmod, "__p___winitenv"); p_environ = (void *)GetProcAddress(hmod, "_environ"); p_wenviron = (void *)GetProcAddress(hmod, "_wenviron"); p_get_environ = (void *)GetProcAddress(hmod, "_get_environ"); @@ -79,12 +83,31 @@ static void test_system(void) ok(ret == 0, "Expected system to return 0, got %d\n", ret); } -static void test__environ(void) +static wchar_t *env_get_valueW( wchar_t **envp, const wchar_t *var ) +{ + unsigned i; + size_t len = wcslen( var ); + + if (!envp) return NULL; + for (i = 0; envp[i] != NULL; i++) + { + wchar_t *ptr; + + if (!(ptr = wcschr( envp[i], L'=' ))) continue; + + if (ptr - envp[i] == len && !memcmp( envp[i], var, len * sizeof(wchar_t) )) + return ptr + 1; + } + return NULL; +} + +static void test__environ(BOOL first_time) { int argc; char **argv, **envp = NULL; - int i, mode = 0; + int mode = 0; + winetest_push_context("%s call", first_time ? "first" : "second"); ok( p_environ != NULL, "Expected the pointer to _environ to be non-NULL\n" ); if (p_environ) ok( *p_environ != NULL, "Expected _environ to be initialized on startup\n" ); @@ -92,6 +115,7 @@ static void test__environ(void) if (!p_environ || !*p_environ) { skip( "_environ pointers are not valid\n" ); + winetest_pop_context(); return; } @@ -110,96 +134,143 @@ static void test__environ(void) else win_skip( "_get_environ() is not available\n" ); - /* Note that msvcrt from Windows versions older than Vista - * expects the mode pointer parameter to be valid.*/ - __getmainargs(&argc, &argv, &envp, 0, &mode); - - ok( envp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); - if (!envp) + if (p__p___initenv) { - skip( "Initial environment block pointer is not valid\n" ); - return; - } + static char **prev_initenv; + char ***retptr = p__p___initenv(); - for (i = 0; ; i++) - { - if ((*p_environ)[i]) + if (first_time) { - ok( envp[i] != NULL, "Expected environment block pointer element to be non-NULL\n" ); - ok( !strcmp((*p_environ)[i], envp[i]), - "Expected _environ and environment block pointer strings (%s vs. %s) to match\n", - (*p_environ)[i], envp[i] ); + todo_wine + ok( *retptr == *p_environ, + "Expected _environ to be equal to initial env\n" ); + prev_initenv = *retptr; } else { - ok( !envp[i], "Expected environment block pointer element to be NULL, got %p\n", envp[i] ); - break; + ok( *retptr != *p_environ, + "Expected _environ[] not to be equal to initial env\n" ); + ok( *retptr == prev_initenv, + "Unexpected modification of initial env\n"); } } + else + skip( "__p___initenv() is not available\n" ); + + /* Note that msvcrt from Windows versions older than Vista + * expects the mode pointer parameter to be valid.*/ + __getmainargs(&argc, &argv, &envp, 0, &mode); + + ok( envp != NULL, + "Expected initial environment block pointer to be non-NULL\n" ); + todo_wine + ok( envp == *p_environ, + "Expected initial environment to be equal to _environ\n"); + winetest_pop_context(); } -static void test__wenviron(void) +static void test__wenviron(BOOL first_time) { int argc; char **argv, **envp = NULL; WCHAR **wargv, **wenvp = NULL; - int i, mode = 0; + int mode = 0; + static wchar_t **prev_initenv; + winetest_push_context("%s call", first_time ? "first" : "second"); ok( p_wenviron != NULL, "Expected the pointer to _wenviron to be non-NULL\n" ); - if (p_wenviron) - ok( *p_wenviron == NULL, "Expected _wenviron to be NULL, got %p\n", *p_wenviron ); + if (first_time) + { + if (!p_wenviron) + { + win_skip( "Pointer to _wenviron is not valid\n" ); + winetest_pop_context(); + return; + } + ok( *p_wenviron == NULL, "Expected _wenviron[] to be NULL, got %p\n", *p_wenviron ); + } else { - win_skip( "Pointer to _wenviron is not valid\n" ); - return; + ok( *p_wenviron != NULL, "Expected _wenviron[] to be non NULL\n" ); } if (sizeof(void*) != sizeof(int)) ok( !p__p__wenviron, "__p__wenviron() should be 32-bit only\n"); else - ok( *p__p__wenviron() == NULL, "Expected _wenviron pointers to be NULL\n" ); + ok( *p__p__wenviron() == *p_wenviron, "Expected environment pointers to be identical\n" ); if (p_get_wenviron) { WCHAR **retptr; p_get_wenviron(&retptr); - ok( retptr == NULL, - "Expected _wenviron pointers to be NULL\n" ); + ok( retptr == *p_wenviron, "Expected _environ pointers to be identical\n" ); } else win_skip( "_get_wenviron() is not available\n" ); + if (p__p___winitenv) + { + wchar_t ***retptr = p__p___winitenv(); + if (first_time) + { + todo_wine + ok( *retptr == NULL, + "Expected initial env to be NULL\n" ); + } + } + else + skip( "__p___winitenv() is not available\n" ); + /* __getmainargs doesn't initialize _wenviron. */ __getmainargs(&argc, &argv, &envp, 0, &mode); - ok( *p_wenviron == NULL, "Expected _wenviron to be NULL, got %p\n", *p_wenviron); + if (first_time) + ok( *p_wenviron == NULL, "Expected _wenviron to be NULL\n"); ok( envp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); if (!envp) { skip( "Initial environment block pointer is not valid\n" ); + winetest_pop_context(); return; } /* Neither does calling the non-Unicode environment manipulation functions. */ ok( _putenv("cat=dog") == 0, "failed setting cat=dog\n" ); - ok( *p_wenviron == NULL, "Expected _wenviron to be NULL, got %p\n", *p_wenviron); - ok( _putenv("cat=") == 0, "failed deleting cat\n" ); + if (first_time) + ok( *p_wenviron == NULL, "Expected _wenviron to be NULL\n" ); /* _wenviron isn't initialized until __wgetmainargs is called or * one of the Unicode environment manipulation functions is called. */ - ok( _wputenv(L"cat=dog") == 0, "failed setting cat=dog\n" ); + ok( _wputenv(L"cat=dog2") == 0, "failed setting cat=dog\n" ); ok( *p_wenviron != NULL, "Expected _wenviron to be non-NULL\n" ); - ok( _wputenv(L"cat=") == 0, "failed deleting cat\n" ); __wgetmainargs(&argc, &wargv, &wenvp, 0, &mode); ok( *p_wenviron != NULL, "Expected _wenviron to be non-NULL\n" ); ok( wenvp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); - if (!wenvp) + todo_wine + ok( wenvp == *p_wenviron, "Expected initial environment to be _wenviron[]\n"); + + if (p__p___winitenv) { - skip( "Initial environment block pointer is not valid\n" ); - return; + wchar_t ***retptr = p__p___winitenv(); + wchar_t *value; + + ok( *retptr != NULL, + "Expected *__p___winitenv() to be NULL\n" ); + ok( *retptr != *p_wenviron, + "Expected _wenviron to be different from __p___winitenv() %p %p\n", *retptr, *p_wenviron ); + /* test that w-initial env is derived from current _environ[] and not from ansi initial env */ + value = env_get_valueW( *retptr, L"cat" ); + todo_wine + ok( value && !wcscmp( value, L"dog" ), "Expecting initial env to be derived from current env (got %ls)\n", value); + + if (first_time) + prev_initenv = *retptr; + else + ok (*retptr == prev_initenv, "Unexpected modification of initial env\n"); } + _putenv("cat="); /* Examine the returned pointer from __p__wenviron(), * if available, after _wenviron is initialized. */ @@ -216,22 +287,7 @@ static void test__wenviron(void) ok( retptr == *p_wenviron, "Expected _wenviron pointers to be identical\n" ); } - - for (i = 0; ; i++) - { - if ((*p_wenviron)[i]) - { - ok( wenvp[i] != NULL, "Expected environment block pointer element to be non-NULL\n" ); - ok( !wcscmp((*p_wenviron)[i], wenvp[i]), - "Expected _wenviron and environment block pointer strings (%s vs. %s) to match\n", - wine_dbgstr_w((*p_wenviron)[i]), wine_dbgstr_w(wenvp[i]) ); - } - else - { - ok( !wenvp[i], "Expected environment block pointer element to be NULL, got %p\n", wenvp[i] ); - break; - } - } + winetest_pop_context(); } static void test_environment_manipulation(void) @@ -312,10 +368,10 @@ START_TEST(environ) { init(); - /* The environ tests should always be run first, as they assume - * that the process has not manipulated the environment. */ - test__environ(); - test__wenviron(); + test__environ(TRUE); + test__wenviron(TRUE); test_environment_manipulation(); + test__environ(FALSE); + test__wenviron(FALSE); test_system(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4313
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/tests/environ.c | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index dd0765edc55..8f53f093093 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -83,6 +83,15 @@ static void test_system(void) ok(ret == 0, "Expected system to return 0, got %d\n", ret); } +static unsigned env_get_entry_countA( char **env ) +{ + unsigned count; + + if (!env) return 0; + for (count = 0; env[count] != NULL; count++) {} + return count; +} + static wchar_t *env_get_valueW( wchar_t **envp, const wchar_t *var ) { unsigned i; @@ -295,6 +304,9 @@ static void test_environment_manipulation(void) char buf[256]; errno_t ret; size_t len; + unsigned count; + char* first; + char* second; ok( _putenv("cat=") == 0, "_putenv failed on deletion of nonexistent environment variable\n" ); ok( _putenv("cat=dog") == 0, "failed setting cat=dog\n" ); @@ -362,6 +374,38 @@ static void test_environment_manipulation(void) ok( !buf[0], "buf = %s\n", buf); ok( errno == 0xdeadbeef, "errno = %d\n", errno); } + + /* test stability of _environ[] pointers */ + ok( _putenv( "__winetest_cat=" ) == 0, "Couldn't reset env var\n" ); + ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); + count = env_get_entry_countA( *p_environ ); + ok( _putenv( "__winetest_cat=mew") == 0, "Couldn't set env var\n" ); + ok( !strcmp( (*p_environ)[count], "__winetest_cat=mew"), "Unexpected env var value\n" ); + first = (*p_environ)[count]; + ok( getenv("__winetest_cat") == strchr( (*p_environ)[count], '=') + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); + ok( _putenv( "__winetest_dog=bark" ) == 0, "Couldn't set env var\n" ); + ok( !strcmp( (*p_environ)[count + 1], "__winetest_dog=bark" ), "Unexpected env var value\n" ); + ok( getenv( "__winetest_dog" ) == strchr( (*p_environ)[count + 1], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); + todo_wine + ok( first == (*p_environ)[count], "Expected stability of _environ[count] pointer\n" ); + second = (*p_environ)[count + 1]; + ok( count + 2 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); + + ok( _putenv( "__winetest_cat=purr" ) == 0, "Couldn't set env var\n" ); + ok( !strcmp( (*p_environ)[count], "__winetest_cat=purr" ), "Unexpected env var value\n" ); + ok( getenv( "__winetest_cat" ) == strchr( (*p_environ)[count], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); + todo_wine + ok( second == (*p_environ)[count + 1], "Expected stability of _environ[count] pointer\n" ); + ok( !strcmp( (*p_environ)[count + 1], "__winetest_dog=bark" ), "Couldn't get env var value\n" ); + ok( getenv( "__winetest_dog" ) == strchr( (*p_environ)[count + 1], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); + ok( count + 2 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); + ok( _putenv( "__winetest_cat=" ) == 0, "Couldn't reset env vat\n" ); + todo_wine + ok( second == (*p_environ)[count], "Expected _environ[count] to be second\n" ); + ok( !strcmp( (*p_environ)[count], "__winetest_dog=bark" ), "Unexpected env var value\n" ); + ok( count + 1 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); + ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); + ok( count == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); } START_TEST(environ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4313
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/tests/environ.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 8f53f093093..3288c5ab99c 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -406,6 +406,17 @@ static void test_environment_manipulation(void) ok( count + 1 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); ok( count == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); + + /* in putenv, only changed variable is updated (no other reload of kernel info is done) */ + ok( _putenv( "__winetest_duck=" ) == 0, "Couldn't reset env var\n" ); + ok( _putenv( "__winetest_elephant=" ) == 0, "Couldn't reset env var\n" ); + ret = SetEnvironmentVariableA( "__winetest_duck", "quack" ); + ok( ret, "SetEnvironmentVariableA failed: %lu\n", GetLastError() ); + ok( _putenv( "__winetest_elephant=trumpet" ) == 0, "Couldn't set env var\n" ); + todo_wine + ok( getenv( "__winetest_duck" ) == NULL, "msvcrt env cache shouldn't have been updated\n" ); + ok( _putenv( "__winetest_duck=" ) == 0, "Couldn't reset env var\n" ); + ok( _putenv( "__winetest_elephant=" ) == 0, "Couldn't reset env var\n" ); } START_TEST(environ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4313
From: Eric Pouech <epouech(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55835 Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/msvcrt/data.c | 321 ++++++++++++++++++++++++++++-------- dlls/msvcrt/environ.c | 19 +-- dlls/msvcrt/msvcrt.h | 5 +- dlls/msvcrt/tests/environ.c | 11 -- 4 files changed, 257 insertions(+), 99 deletions(-) diff --git a/dlls/msvcrt/data.c b/dlls/msvcrt/data.c index d1f514fd460..0bf22c76664 100644 --- a/dlls/msvcrt/data.c +++ b/dlls/msvcrt/data.c @@ -18,6 +18,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#include <stdio.h> #include <fcntl.h> #include <math.h> #include "msvcrt.h" @@ -52,6 +53,9 @@ wchar_t **MSVCRT___wargv = NULL; static wchar_t **wargv_expand; char *MSVCRT__acmdln = NULL; wchar_t *MSVCRT__wcmdln = NULL; +/* The entries in in MSVCRT__environ[] and in MSVCRT__wenviron[] must be + * kept in sync. + */ char **MSVCRT__environ = NULL; wchar_t **MSVCRT__wenviron = NULL; char **MSVCRT___initenv = NULL; @@ -60,80 +64,229 @@ int MSVCRT_app_type = 0; char* MSVCRT__pgmptr = NULL; WCHAR* MSVCRT__wpgmptr = NULL; -/* Get a snapshot of the current environment - * and construct the __p__environ array - * - * The pointer returned from GetEnvironmentStrings may get invalid when - * some other module cause a reallocation of the env-variable block - * - * blk is an array of pointers to environment strings, ending with a NULL - * and after that the actual copy of the environment strings, ending in a \0 - */ -char ** msvcrt_SnapshotOfEnvironmentA(char **blk) +static char **env_snapshot( void ) { - char* environ_strings = GetEnvironmentStringsA(); - int count = 1, len = 1, i = 0; /* keep space for the trailing NULLS */ - char *ptr; + char *environ_strings = GetEnvironmentStringsA(); + int count = 1; /* keep space for the trailing NULL */ + size_t size = 1; + char *ptr; + char **blk; - for (ptr = environ_strings; *ptr; ptr += strlen(ptr) + 1) - { - /* Don't count environment variables starting with '=' which are command shell specific */ - if (*ptr != '=') count++; - len += strlen(ptr) + 1; - } - if (blk) - blk = HeapReAlloc( GetProcessHeap(), 0, blk, count* sizeof(char*) + len ); - else - blk = HeapAlloc(GetProcessHeap(), 0, count* sizeof(char*) + len ); + for (ptr = environ_strings; *ptr; ptr += strlen( ptr ) + 1) + { + /* Don't count environment variables starting with '=' which are command shell specific */ + if (*ptr != '=') + { + count++; + size += strlen( ptr ) + 1; + } + } + if ((blk = HeapAlloc( GetProcessHeap(), 0, count * sizeof(char *) + size * sizeof(wchar_t) ))) + { + int i = 0; + char* ptr_blk = (char *)(blk + count); + size_t len; + for (ptr = environ_strings; *ptr; ptr += len + 1) + { + len = strlen( ptr ); + /* Skip special environment strings set by the command shell */ + if (*ptr != '=') + { + strcpy( ptr_blk, ptr ); + blk[i] = ptr_blk; + ptr_blk += len + 1; + i++; + } + } + *ptr_blk = '\0'; + blk[i] = NULL; + } + FreeEnvironmentStringsA( environ_strings ); + return blk; +} - if (blk) +static char **env_cloneA( char** env ) +{ + int i; + char **blk; + + for (i = 0; env[i]; i++) {} + if ((blk = HeapAlloc( GetProcessHeap(), 0, (i + 1) * sizeof(char *) ))) { - if (count) - { - memcpy(&blk[count],environ_strings,len); - for (ptr = (char*) &blk[count]; *ptr; ptr += strlen(ptr) + 1) - { - /* Skip special environment strings set by the command shell */ - if (*ptr != '=') blk[i++] = ptr; - } - } - blk[i] = NULL; + for (i = 0; env[i]; i++) + blk[i] = strdup( env[i] ); + blk[i] = NULL; } - FreeEnvironmentStringsA(environ_strings); - return blk; + return blk; } -wchar_t ** msvcrt_SnapshotOfEnvironmentW(wchar_t **wblk) +int msvcrt_update_environmentA( const char *name, const char *value ) { - wchar_t* wenviron_strings = GetEnvironmentStringsW(); - int count = 1, len = 1, i = 0; /* keep space for the trailing NULLS */ - wchar_t *wptr; + int i = 0; + size_t len = strlen( name ); + char *set; + char **new; - for (wptr = wenviron_strings; *wptr; wptr += wcslen(wptr) + 1) - { - /* Don't count environment variables starting with '=' which are command shell specific */ - if (*wptr != '=') count++; - len += wcslen(wptr) + 1; - } - if (wblk) - wblk = HeapReAlloc( GetProcessHeap(), 0, wblk, count* sizeof(wchar_t*) + len * sizeof(wchar_t)); - else - wblk = HeapAlloc(GetProcessHeap(), 0, count* sizeof(wchar_t*) + len * sizeof(wchar_t)); - if (wblk) + if (MSVCRT__environ == MSVCRT___initenv) + MSVCRT__environ = env_cloneA(MSVCRT___initenv); + + if (value[0]) { - if (count) - { - memcpy(&wblk[count],wenviron_strings,len * sizeof(wchar_t)); - for (wptr = (wchar_t*)&wblk[count]; *wptr; wptr += wcslen(wptr) + 1) - { - /* Skip special environment strings set by the command shell */ - if (*wptr != '=') wblk[i++] = wptr; - } - } - wblk[i] = NULL; + size_t sz = strlen( name ) + 1 + strlen( value ) + 1; + set = malloc( sz ); + if (!set) return -1; + snprintf( set, sz, "%s=%s", name, value ); } - FreeEnvironmentStringsW(wenviron_strings); - return wblk; + else set = NULL; + + for (i = 0; MSVCRT__environ[i] != NULL; i++) + { + if (!memcmp( name, MSVCRT__environ[i], len ) && MSVCRT__environ[i][len] == '=') + { + free( MSVCRT__environ[i] ); + if (MSVCRT__wenviron) free( MSVCRT__wenviron[i] ); + if (set) + { + MSVCRT__environ[i] = set; + if (MSVCRT__wenviron) MSVCRT__wenviron[i] = msvcrt_wstrdupa( set ); + } + else + { + do + { + MSVCRT__environ[i] = MSVCRT__environ[i + 1]; + if (MSVCRT__wenviron) MSVCRT__wenviron[i] = MSVCRT__wenviron[i + 1]; + } while (MSVCRT__environ[++i] != NULL); + } + return 0; + } + } + if (!set) return 0; + new = HeapReAlloc( GetProcessHeap(), 0, MSVCRT__environ, (i + 2) * sizeof(char *) ); + if (!new) + { + free(set); + return -1; + } + if (MSVCRT__wenviron) + { + WCHAR** wnew = HeapReAlloc( GetProcessHeap(), 0, MSVCRT__wenviron, (i + 2) * sizeof(wchar_t *) ); + if (!wnew) + { + HeapFree( GetProcessHeap(), 0, new ); + free( set ); + return -1; + } + MSVCRT__wenviron = wnew; + } + MSVCRT__environ = new; + MSVCRT__environ[i] = set; + MSVCRT__environ[i + 1] = NULL; + if (MSVCRT__wenviron) + { + MSVCRT__wenviron[i] = msvcrt_wstrdupa( set ); + MSVCRT__wenviron[i + 1] = NULL; + } + return 0; +} + +wchar_t **msvcrt_clone_environmentA2W( char **env ) +{ + int count; + wchar_t **ret; + + for (count = 0; env[count] != NULL; count++) {} + if ((ret = HeapAlloc( GetProcessHeap(), 0, (count + 1) * sizeof(wchar_t *))) ) + { + for (count = 0; env[count] != NULL; count++) + { + ret[count] = msvcrt_wstrdupa( env[count] ); + } + ret[count] = NULL; + } + return ret; +} + +/* INTERNAL: Create an ascii string from a wide string */ +static char *msvcrt_astrdupw( const wchar_t *wstr ) +{ + const unsigned int len = WideCharToMultiByte( CP_ACP, 0, wstr, -1, NULL, 0, NULL, NULL ); + char *str = malloc( len * sizeof(char) ); + if (!str) + return NULL; + WideCharToMultiByte( CP_ACP, 0, wstr, -1, str, len, NULL, NULL ); + return str; +} + +int msvcrt_update_environmentW( const wchar_t *name, const wchar_t *value ) +{ + int i = 0; + size_t len = wcslen( name ); + wchar_t *set; + wchar_t **new; + char** anew; + + if (MSVCRT__environ == MSVCRT___initenv) + MSVCRT__environ = env_cloneA(MSVCRT___initenv); + if (!MSVCRT__wenviron) + { + /* yes that looks strange, but it's how native does it */ + if (!MSVCRT___winitenv) MSVCRT___winitenv = msvcrt_clone_environmentA2W( MSVCRT__environ ); + MSVCRT__wenviron = msvcrt_clone_environmentA2W( MSVCRT__environ ); + } + + if (value[0]) + { + size_t sz = wcslen( name ) + 1 + wcslen( value ) + 1; + set = malloc( sz * sizeof(wchar_t) ); + if (!set) return -1; + swprintf(set, sz, L"%s=%s", name, value); + } + else set = NULL; + + for (i = 0; MSVCRT__wenviron[i] != NULL; i++) + { + if (!memcmp(name, MSVCRT__wenviron[i], len * sizeof(wchar_t)) && MSVCRT__wenviron[i][len] == L'=') + { + free(MSVCRT__environ[i]); + free(MSVCRT__wenviron[i]); + if (set) + { + MSVCRT__environ[i] = msvcrt_astrdupw( set ); + MSVCRT__wenviron[i] = set; + } + else + { + do + { + MSVCRT__environ[i] = MSVCRT__environ[i + 1]; + MSVCRT__wenviron[i] = MSVCRT__wenviron[i + 1]; + } while (MSVCRT__environ[++i] != NULL); + } + return 0; + } + } + if (!set) return 0; + new = HeapReAlloc( GetProcessHeap(), 0, MSVCRT__wenviron, (i + 2) * sizeof(wchar_t *) ); + if (!new) + { + free( set ); + return -1; + } + anew = HeapReAlloc( GetProcessHeap(), 0, MSVCRT__environ, (i + 2) * sizeof(char *) ); + if (!anew) + { + HeapFree( GetProcessHeap(), 0, new ); + free(set); + return -1; + } + MSVCRT__environ = anew; + MSVCRT__wenviron = new; + MSVCRT__environ[i] = msvcrt_astrdupw( set ); + MSVCRT__wenviron[i] = set; + MSVCRT__environ[i + 1] = NULL; + MSVCRT__wenviron[i + 1] = NULL; + return 0; } static char **build_argv( WCHAR **wargv ) @@ -438,9 +591,7 @@ void msvcrt_init_args(void) MSVCRT___unguarded_readlc_active = 0; MSVCRT__fmode = _O_TEXT; - MSVCRT__environ = msvcrt_SnapshotOfEnvironmentA(NULL); - MSVCRT___initenv = msvcrt_SnapshotOfEnvironmentA(NULL); - MSVCRT___winitenv = msvcrt_SnapshotOfEnvironmentW(NULL); + MSVCRT___initenv = MSVCRT__environ = env_snapshot(); MSVCRT__pgmptr = HeapAlloc(GetProcessHeap(), 0, MAX_PATH); if (MSVCRT__pgmptr) @@ -461,15 +612,35 @@ void msvcrt_init_args(void) } } +static void env_freeA(char** env) +{ + int i; + for (i = 0; env[i] != NULL; i++) + free( env[i] ); + HeapFree( GetProcessHeap(), 0, env ); +} + +static void env_freeW(wchar_t** wenv) +{ + int i; + for (i = 0; wenv[i] != NULL; i++) + free( wenv[i] ); + HeapFree( GetProcessHeap(), 0, wenv ); +} + /* INTERNAL: free memory used by args */ void msvcrt_free_args(void) { /* FIXME: more things to free */ HeapFree(GetProcessHeap(), 0, MSVCRT___argv); - HeapFree(GetProcessHeap(), 0, MSVCRT___initenv); - HeapFree(GetProcessHeap(), 0, MSVCRT___winitenv); - HeapFree(GetProcessHeap(), 0, MSVCRT__environ); - HeapFree(GetProcessHeap(), 0, MSVCRT__wenviron); + + if (MSVCRT__environ != MSVCRT___initenv) + env_freeA( MSVCRT__environ ); + HeapFree( GetProcessHeap(), 0, MSVCRT___initenv ); + if (MSVCRT__wenviron && MSVCRT__wenviron != MSVCRT___winitenv) + env_freeW( MSVCRT__wenviron ); + HeapFree( GetProcessHeap(), 0, MSVCRT___winitenv ); + HeapFree(GetProcessHeap(), 0, MSVCRT__pgmptr); HeapFree(GetProcessHeap(), 0, MSVCRT__wpgmptr); HeapFree(GetProcessHeap(), 0, wargv_expand); @@ -565,10 +736,14 @@ int CDECL __wgetmainargs(int *argc, wchar_t** *wargv, wchar_t** *wenvp, /* Initialize the _wenviron array if it's not already created. */ if (!MSVCRT__wenviron) - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(NULL); + { + /* yes that looks strange, but it's how native does it */ + if (!MSVCRT___winitenv) MSVCRT___winitenv = msvcrt_clone_environmentA2W( MSVCRT__environ ); + MSVCRT__wenviron = msvcrt_clone_environmentA2W( MSVCRT__environ ); + } *argc = MSVCRT___argc; *wargv = MSVCRT___wargv; - *wenvp = MSVCRT___winitenv; + *wenvp = MSVCRT__wenviron; if (new_mode) _set_new_mode( *new_mode ); return 0; @@ -602,7 +777,7 @@ int CDECL __getmainargs(int *argc, char** *argv, char** *envp, *argc = MSVCRT___argc; *argv = MSVCRT___argv; - *envp = MSVCRT___initenv; + *envp = MSVCRT__environ; if (new_mode) _set_new_mode( *new_mode ); diff --git a/dlls/msvcrt/environ.c b/dlls/msvcrt/environ.c index 2c2a3353582..94d67982e19 100644 --- a/dlls/msvcrt/environ.c +++ b/dlls/msvcrt/environ.c @@ -66,7 +66,7 @@ static wchar_t * wgetenv_helper(const wchar_t *name) /* Initialize the _wenviron array if it's not already created. */ if (!MSVCRT__wenviron) - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(NULL); + MSVCRT__wenviron = msvcrt_clone_environmentA2W(MSVCRT__environ); for (env = MSVCRT__wenviron; *env; env++) { @@ -127,11 +127,8 @@ int CDECL _putenv(const char *str) /* _putenv returns success on deletion of nonexistent variable, unlike [Rtl]SetEnvironmentVariable */ if ((ret == -1) && (GetLastError() == ERROR_ENVVAR_NOT_FOUND)) ret = 0; - MSVCRT__environ = msvcrt_SnapshotOfEnvironmentA(MSVCRT__environ); - /* Update the __p__wenviron array only when already initialized */ - if (MSVCRT__wenviron) - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(MSVCRT__wenviron); - + if (ret == 0) ret = msvcrt_update_environmentA(name, value); + finish: HeapFree(GetProcessHeap(), 0, name); return ret; @@ -172,8 +169,7 @@ int CDECL _wputenv(const wchar_t *str) /* _putenv returns success on deletion of nonexistent variable, unlike [Rtl]SetEnvironmentVariable */ if ((ret == -1) && (GetLastError() == ERROR_ENVVAR_NOT_FOUND)) ret = 0; - MSVCRT__environ = msvcrt_SnapshotOfEnvironmentA(MSVCRT__environ); - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(MSVCRT__wenviron); + if (ret == 0) ret = msvcrt_update_environmentW(name, value); finish: HeapFree(GetProcessHeap(), 0, name); @@ -201,9 +197,7 @@ errno_t CDECL _putenv_s(const char *name, const char *value) ret = *_errno(); } } - - MSVCRT__environ = msvcrt_SnapshotOfEnvironmentA(MSVCRT__environ); - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(MSVCRT__wenviron); + if (ret == 0) ret = msvcrt_update_environmentA(name, value); return ret; } @@ -230,8 +224,7 @@ errno_t CDECL _wputenv_s(const wchar_t *name, const wchar_t *value) } } - MSVCRT__environ = msvcrt_SnapshotOfEnvironmentA(MSVCRT__environ); - MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(MSVCRT__wenviron); + if (ret == 0) ret = msvcrt_update_environmentW(name, value); return ret; } diff --git a/dlls/msvcrt/msvcrt.h b/dlls/msvcrt/msvcrt.h index 7c2dd4835f5..d6ddf6812ab 100644 --- a/dlls/msvcrt/msvcrt.h +++ b/dlls/msvcrt/msvcrt.h @@ -206,8 +206,9 @@ void __cdecl _amsg_exit(int errnum); extern char **MSVCRT__environ; extern wchar_t **MSVCRT__wenviron; -extern char ** msvcrt_SnapshotOfEnvironmentA(char **); -extern wchar_t ** msvcrt_SnapshotOfEnvironmentW(wchar_t **); +int msvcrt_update_environmentA(const char *name, const char *value); +int msvcrt_update_environmentW(const wchar_t *name, const wchar_t * value); +wchar_t **msvcrt_clone_environmentA2W(char**); wchar_t *msvcrt_wstrdupa(const char *); diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 3288c5ab99c..e97ab8d0c3a 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -150,7 +150,6 @@ static void test__environ(BOOL first_time) if (first_time) { - todo_wine ok( *retptr == *p_environ, "Expected _environ to be equal to initial env\n" ); prev_initenv = *retptr; @@ -172,7 +171,6 @@ static void test__environ(BOOL first_time) ok( envp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); - todo_wine ok( envp == *p_environ, "Expected initial environment to be equal to _environ\n"); winetest_pop_context(); @@ -221,11 +219,8 @@ static void test__wenviron(BOOL first_time) { wchar_t ***retptr = p__p___winitenv(); if (first_time) - { - todo_wine ok( *retptr == NULL, "Expected initial env to be NULL\n" ); - } } else skip( "__p___winitenv() is not available\n" ); @@ -257,7 +252,6 @@ static void test__wenviron(BOOL first_time) ok( *p_wenviron != NULL, "Expected _wenviron to be non-NULL\n" ); ok( wenvp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); - todo_wine ok( wenvp == *p_wenviron, "Expected initial environment to be _wenviron[]\n"); if (p__p___winitenv) @@ -271,7 +265,6 @@ static void test__wenviron(BOOL first_time) "Expected _wenviron to be different from __p___winitenv() %p %p\n", *retptr, *p_wenviron ); /* test that w-initial env is derived from current _environ[] and not from ansi initial env */ value = env_get_valueW( *retptr, L"cat" ); - todo_wine ok( value && !wcscmp( value, L"dog" ), "Expecting initial env to be derived from current env (got %ls)\n", value); if (first_time) @@ -386,7 +379,6 @@ static void test_environment_manipulation(void) ok( _putenv( "__winetest_dog=bark" ) == 0, "Couldn't set env var\n" ); ok( !strcmp( (*p_environ)[count + 1], "__winetest_dog=bark" ), "Unexpected env var value\n" ); ok( getenv( "__winetest_dog" ) == strchr( (*p_environ)[count + 1], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); - todo_wine ok( first == (*p_environ)[count], "Expected stability of _environ[count] pointer\n" ); second = (*p_environ)[count + 1]; ok( count + 2 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); @@ -394,13 +386,11 @@ static void test_environment_manipulation(void) ok( _putenv( "__winetest_cat=purr" ) == 0, "Couldn't set env var\n" ); ok( !strcmp( (*p_environ)[count], "__winetest_cat=purr" ), "Unexpected env var value\n" ); ok( getenv( "__winetest_cat" ) == strchr( (*p_environ)[count], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); - todo_wine ok( second == (*p_environ)[count + 1], "Expected stability of _environ[count] pointer\n" ); ok( !strcmp( (*p_environ)[count + 1], "__winetest_dog=bark" ), "Couldn't get env var value\n" ); ok( getenv( "__winetest_dog" ) == strchr( (*p_environ)[count + 1], '=' ) + 1, "Expected getenv() to return pointer inside _environ[] entry\n" ); ok( count + 2 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); ok( _putenv( "__winetest_cat=" ) == 0, "Couldn't reset env vat\n" ); - todo_wine ok( second == (*p_environ)[count], "Expected _environ[count] to be second\n" ); ok( !strcmp( (*p_environ)[count], "__winetest_dog=bark" ), "Unexpected env var value\n" ); ok( count + 1 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); @@ -413,7 +403,6 @@ static void test_environment_manipulation(void) ret = SetEnvironmentVariableA( "__winetest_duck", "quack" ); ok( ret, "SetEnvironmentVariableA failed: %lu\n", GetLastError() ); ok( _putenv( "__winetest_elephant=trumpet" ) == 0, "Couldn't set env var\n" ); - todo_wine ok( getenv( "__winetest_duck" ) == NULL, "msvcrt env cache shouldn't have been updated\n" ); ok( _putenv( "__winetest_duck=" ) == 0, "Couldn't reset env var\n" ); ok( _putenv( "__winetest_elephant=" ) == 0, "Couldn't reset env var\n" ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4313
V3 pushed: * reworked initial env status with more tests * moved out cache tests into a separated test patch * integrated Piotr's comment Note: ucrtbase init env are different (both ansi & unicode are initialized at startup, to be integrated into another MR) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51506
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/environ.c:
ok( count + 1 == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); ok( count == env_get_entry_countA( *p_environ ), "Unexpected count\n" ); + + /* in putenv, only changed variable is updated (no other reload of kernel info is done) */ + ok( _putenv( "__winetest_duck=" ) == 0, "Couldn't reset env var\n" ); + ok( _putenv( "__winetest_elephant=" ) == 0, "Couldn't reset env var\n" );
It's a minor thing but I think it's better to limit number of environment variables that are used in test, please reuse `__winetest_{dog,cat}` here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51519
Piotr Caban (@piotr) commented about dlls/msvcrt/data.c:
+ } + *ptr_blk = '\0'; + blk[i] = NULL; + } + FreeEnvironmentStringsA( environ_strings ); + return blk; +}
- if (blk) +static char **env_cloneA( char** env ) +{ + int i; + char **blk; + + for (i = 0; env[i]; i++) {} + if ((blk = HeapAlloc( GetProcessHeap(), 0, (i + 1) * sizeof(char *) ))) It can be changed later but since you're rewriting all the functions why not change it to use the malloc/free as native does?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51520
Piotr Caban (@piotr) commented about dlls/msvcrt/data.c:
+ { + if (!memcmp( name, MSVCRT__environ[i], len ) && MSVCRT__environ[i][len] == '=') + { + free( MSVCRT__environ[i] ); + if (MSVCRT__wenviron) free( MSVCRT__wenviron[i] ); + if (set) + { + MSVCRT__environ[i] = set; + if (MSVCRT__wenviron) MSVCRT__wenviron[i] = msvcrt_wstrdupa( set ); + } + else + { + do + { + MSVCRT__environ[i] = MSVCRT__environ[i + 1]; + if (MSVCRT__wenviron) MSVCRT__wenviron[i] = MSVCRT__wenviron[i + 1]; This code is making assumption that _environ and _wenviron are always in sync, I think it's OK but we should make sure that's true. We would at least need to make sure that msvcrt_wstrdupa failure is handled correctly. I didn't dig into it but if msvcrt_wstrdupa fails it can cause some environment variables being removed. It can also cause some leaks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51521
There's one more thing that the MR changes that needs addressing. We no longer use GetEnvironmentStringsW and depend on A->W conversion. It would be good to test what happens with variables that can't be correctly represented in ansi case. It's also interesting to check what happens when there are duplicated variable names due to conversion error. I would also like to make sure it doesn't introduce regression in ucrtbase (assuming it handles it differently). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51522
On Wed Nov 8 17:05:16 2023 +0000, Piotr Caban wrote:
There's one more thing that the MR changes that needs addressing. We no longer use GetEnvironmentStringsW and depend on A->W conversion. It would be good to test what happens with variables that can't be correctly represented in ansi case. It's also interesting to check what happens when there are duplicated variable names due to conversion error. I would also like to make sure it doesn't introduce regression in ucrtbase (assuming it handles it differently). on the first point, I'd rather first test how \\\_winitenv is constructed. I derived it from \\\_environ\[\], but it could also well be directly from GetEnvironmentStringsW (using same logic as how \\\_initenv is created) (\*). If it's the case, that would likely solve the question about unicode variable names. (not sure yet if such a test can be kept around init testing, but I was thinking of something like: SetEnvironmentVariableW("foo"n "bar") before winitenv is created and check afterwards if foo is present in \\\_winitenv or \\\_wenviron, then it means it's derived from kernel data; otherwise from \_environ\[\]).
for ucrtbase, I started looking at it. all the 4 env (ansi & unicode) + init seem to be created at startup (when dynamic linking ucrtbase, didn't try with static linking on MSVC). That's one difference from msvcrt (but this MR doesn't change this behavior). What's the best practice for sharing the tests between msvcrt and ucrtbase? (tests need adaptation as some APIs like \__getmainargs doesn't exist in ucrtbase) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51537
On Wed Nov 8 17:05:15 2023 +0000, eric pouech wrote:
> on the first point, I'd rather first test how \\\_winitenv is
> constructed. I derived it from \\\_environ\[\], but it could also well
> be directly from GetEnvironmentStringsW (using same logic as how
> \\\_initenv is created) (\*). If it's the case, that would likely solve
> the question about unicode variable names. (not sure yet if such a test
> can be kept around init testing, but I was thinking of something like:
> SetEnvironmentVariableW("foo"n "bar") before winitenv is created and
> check afterwards if foo is present in \\\_winitenv or \\\_wenviron, then
> it means it's derived from kernel data; otherwise from \_environ\[\]).
> for ucrtbase, I started looking at it. all the 4 env (ansi & unicode) +
> init seem to be created at startup (when dynamic linking ucrtbase,
> didn't try with static linking on MSVC). That's one difference from
> msvcrt (but this MR doesn't change this behavior). What's the best
> practice for sharing the tests between msvcrt and ucrtbase? (tests need
> adaptation as some APIs like \__getmainargs doesn't exist in ucrtbase)
1. What I really want to achieve here is to not break something that is currently working. If there is any version of msvcrXX that handles unicode environment variables correctly I want to preserve that to avoid regressions. Regarding testing it, if needed, it should be possible to test it with `CreateProcess` call with `lpEnvironment` argument.
2. There's currently no way of sharing the tests (so you will need to copy the tests).
Note that I don't think it all needs to be fixed so it's working exactly as in native. My concern is that proposed implementation looks risky in regard to A->W conversion.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51538
On Wed Nov 8 17:21:19 2023 +0000, Piotr Caban wrote:
1. What I really want to achieve here is to not break something that is currently working. If there is any version of msvcrXX that handles unicode environment variables correctly I want to preserve that to avoid regressions. Regarding testing it, if needed, it should be possible to test it with `CreateProcess` call with `lpEnvironment` argument. 2. There's currently no way of sharing the tests (so you will need to copy the tests). Note that I don't think it all needs to be fixed so it's working exactly as in native. My concern is that proposed implementation looks risky in regard to A->W conversion. sure. I tested how native works and the creation of wenviron and winitenv are done using GetEnvironmentStringsW data, so I'll rewrite accordingly. But this means that the ansi and unicode initenv will be created at different point in time, and could be out of sync. but more importantly, if an env variable is added/updated via kernel API, we will get a difference in the environ / wenviron arrays (it will be absent from environ, but inserted when creating wenviron). so the assumption of keeping the two arrays in sync seems more than fragile (not even speaking of the OOM conditions)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4313#note_51545
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Piotr Caban (＠piotr)