[PATCH v6 0/6] 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.
-- v6: msvcrt: Fix getmainargs() family to return correct environment block. msvcrt: Use msvcrt heap for allocating envionment data. msvcrt: Test adding UNICODE env variables. 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).
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/environ.c | 150 ++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 68 deletions(-)
diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 22f697a12ea..74add49e010 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,21 +83,32 @@ static void test_system(void) ok(ret == 0, "Expected system to return 0, got %d\n", ret); }
+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(void) { int argc; - char **argv, **envp = NULL; - int i, mode = 0; + char **argv, **envp = NULL, **initenv = NULL; + int mode = 0;
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" ); - - if (!p_environ || !*p_environ) - { - skip( "_environ pointers are not valid\n" ); - return; - } + ok( *p_environ != NULL, "Expected _environ to be initialized on startup\n" );
if (sizeof(void*) != sizeof(int)) ok( !p__p__environ, "__p__environ() should be 32-bit only\n"); @@ -110,32 +125,38 @@ static void test__environ(void) else win_skip( "_get_environ() is not available\n" );
+ if (p__p___initenv) + { + initenv = *p__p___initenv(); + + todo_wine + ok( initenv == *p_environ, + "Expected _environ to be equal to 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" ); - if (!envp) - { - skip( "Initial environment block pointer is not valid\n" ); - return; - } + 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" );
- for (i = 0; ; i++) + ok( _putenv("cat=dog") == 0, "failed setting cat=dog\n" ); + if (p__p___initenv) { - if ((*p_environ)[i]) - { - 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] ); - } - else - { - ok( !envp[i], "Expected environment block pointer element to be NULL, got %p\n", envp[i] ); - break; - } + char **retptr = *p__p___initenv(); + + ok( retptr != *p_environ, + "Expected _environ[] not to be equal to initial env\n" ); + ok( retptr == initenv, + "Unexpected modification of initial env\n" ); } + ok( _putenv("cat=") == 0, "failed setting cat=\n" ); }
static void test__wenviron(void) @@ -143,36 +164,38 @@ static void test__wenviron(void) int argc; char **argv, **envp = NULL; WCHAR **wargv, **wenvp = NULL; - int i, mode = 0; + int mode = 0;
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 ); - else - { - win_skip( "Pointer to _wenviron is not valid\n" ); - return; - } + ok( !*p_wenviron, "Expected _wenviron[] to be NULL, got %p\n", *p_wenviron );
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(), "Expected _wenviron to be NULL, got %p\n", *p_wenviron );
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 _wenviron pointers to be NULL\n" ); } else win_skip( "_get_wenviron() is not available\n" );
+ if (p__p___winitenv) + { + wchar_t ***retptr = p__p___winitenv(); + todo_wine + ok( !*retptr, "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); + ok( !*p_wenviron, "Expected _wenviron to be NULL\n"); ok( envp != NULL, "Expected initial environment block pointer to be non-NULL\n" ); if (!envp) { @@ -182,24 +205,33 @@ static void test__wenviron(void)
/* 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" ); + ok( !*p_wenviron, "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=dog2\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 ); } + _putenv("cat=");
/* Examine the returned pointer from __p__wenviron(), * if available, after _wenviron is initialized. */ @@ -216,22 +248,6 @@ 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; - } - } }
static void test_environment_manipulation(void) @@ -312,8 +328,6 @@ 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_environment_manipulation();
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@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 74add49e010..eeee2c8f77d 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; @@ -255,6 +264,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" ); @@ -322,6 +334,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)
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/environ.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index eeee2c8f77d..15e75a2caaf 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -366,6 +366,15 @@ 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) */ + ret = SetEnvironmentVariableA( "__winetest_cat", "meow" ); + ok( ret, "SetEnvironmentVariableA failed: %lu\n", GetLastError() ); + ok( _putenv( "__winetest_dog=bark" ) == 0, "Couldn't set env var\n" ); + todo_wine + ok( getenv( "__winetest_cat" ) == NULL, "msvcrt env cache shouldn't have been updated\n" ); + ok( _putenv( "__winetest_cat=" ) == 0, "Couldn't reset env var\n" ); + ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); }
START_TEST(environ)
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/environ.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 15e75a2caaf..a3d3156c01a 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -22,6 +22,7 @@ #include <errno.h> #include <stdlib.h> #include <process.h> +#include <winnls.h>
static const char *a_very_long_env_string = "LIBRARY_PATH=" @@ -375,6 +376,23 @@ static void test_environment_manipulation(void) ok( getenv( "__winetest_cat" ) == NULL, "msvcrt env cache shouldn't have been updated\n" ); ok( _putenv( "__winetest_cat=" ) == 0, "Couldn't reset env var\n" ); ok( _putenv( "__winetest_dog=" ) == 0, "Couldn't reset env var\n" ); + + /* test setting unicode bits */ + count = env_get_entry_countA( *p_environ ); + ret = WideCharToMultiByte( CP_ACP, 0, L"\u263a", -1, buf, ARRAY_SIZE(buf), 0, 0 ); + ok( ret, "WideCharToMultiByte failed: %lu\n", GetLastError() ); + ok( _wputenv( L"__winetest_cat=\u263a" ) == 0, "Couldn't set env var\n" ); + ok( _wgetenv( L"__winetest_cat" ) && !wcscmp( _wgetenv( L"__winetest_cat" ), L"\u263a" ), "Couldn't retrieve env var\n" ); + ok( getenv( "__winetest_cat" ) && !strcmp( getenv( "__winetest_cat" ), buf ), "Couldn't retrieve env var\n" ); + ok( _wputenv( L"__winetest_cat=" ) == 0, "Couldn't reset env var\n" ); + + ret = WideCharToMultiByte( CP_ACP, 0, L"__winetest_\u263a", -1, buf, ARRAY_SIZE(buf), 0, 0 ); + ok( ret, "WideCharToMultiByte failed: %lu\n", GetLastError() ); + ok( _wputenv( L"__winetest_\u263a=bark" ) == 0, "Couldn't set env var\n" ); + ok( _wgetenv( L"__winetest_\u263a" ) && !wcscmp( _wgetenv( L"__winetest_\u263a" ), L"bark"), "Couldn't retrieve env var\n" ); + ok( getenv( buf ) && !strcmp( getenv( buf ), "bark"), "Couldn't retrieve env var %s\n", wine_dbgstr_a(buf) ); + ok( _wputenv( L"__winetest_\u263a=" ) == 0, "Couldn't reset env var\n" ); + ok( count == env_get_entry_countA( *p_environ ), "Unexpected modification of _environ[]\n" ); }
START_TEST(environ)
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/data.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/dlls/msvcrt/data.c b/dlls/msvcrt/data.c index d1f514fd460..6312c1e0e32 100644 --- a/dlls/msvcrt/data.c +++ b/dlls/msvcrt/data.c @@ -81,10 +81,7 @@ char ** msvcrt_SnapshotOfEnvironmentA(char **blk) 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 ); + blk = realloc(blk, count * sizeof(char*) + len);
if (blk) { @@ -115,10 +112,8 @@ wchar_t ** msvcrt_SnapshotOfEnvironmentW(wchar_t **wblk) 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)); + wblk = realloc(wblk, count * sizeof(wchar_t*) + len * sizeof(wchar_t)); + if (wblk) { if (count) @@ -466,10 +461,6 @@ 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); HeapFree(GetProcessHeap(), 0, MSVCRT__pgmptr); HeapFree(GetProcessHeap(), 0, MSVCRT__wpgmptr); HeapFree(GetProcessHeap(), 0, wargv_expand);
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/data.c | 4 ++-- dlls/msvcrt/tests/environ.c | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/msvcrt/data.c b/dlls/msvcrt/data.c index 6312c1e0e32..8a8552a151e 100644 --- a/dlls/msvcrt/data.c +++ b/dlls/msvcrt/data.c @@ -559,7 +559,7 @@ int CDECL __wgetmainargs(int *argc, wchar_t** *wargv, wchar_t** *wenvp, MSVCRT__wenviron = msvcrt_SnapshotOfEnvironmentW(NULL); *argc = MSVCRT___argc; *wargv = MSVCRT___wargv; - *wenvp = MSVCRT___winitenv; + *wenvp = MSVCRT__wenviron; if (new_mode) _set_new_mode( *new_mode ); return 0; @@ -593,7 +593,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/tests/environ.c b/dlls/msvcrt/tests/environ.c index a3d3156c01a..f14764ea5a0 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -152,7 +152,6 @@ static void test__environ(void)
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" );
@@ -224,7 +223,6 @@ static void test__wenviron(void)
__wgetmainargs(&argc, &wargv, &wenvp, 0, &mode); 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)
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139750
Your paranoid android.
=== debian11b (64 bit WoW report) ===
kernel32: sync.c:1136: Test failed: ChangeTimerQueueTimer
This merge request was approved by Piotr Caban.
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Piotr Caban (＠piotr)