_(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.
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/tests/environ.c | 95 +++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)
diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index 22f697a12ea..54f8650db80 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -136,6 +136,55 @@ static void test__environ(void) break; } } + + /* force change in _environ[], seems like native needs a first update to set allocation per entry */ + _putenv( "__fish=cat" ); + _putenv( "__fish=" ); + if ((*p_environ)[0] && (*p_environ)[1]) + { + char *first = (*p_environ)[0]; + char *second = (*p_environ)[1]; + char *new_first; + char *equal; + size_t len = strlen(first); + + new_first = malloc( len + 1 + 1 ); + ok( new_first != NULL, "allocation failed\n" ); + strcpy( new_first, first ); + strcat( new_first, "A" ); + _putenv( new_first ); + + todo_wine + ok( second == (*p_environ)[1], "_environ[1] shouldn't have changed\n" ); + ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[1] shouldn't have changed\n" ); + todo_wine + ok( !strcmp( second, (*p_environ)[1] ), "_environ[1] shouldn't have changed\n" ); + + /* msvcrt _environ isn't updated after direct kernel environ API */ + equal = strchr( new_first, '=' ); + if (equal) + { + BOOL ret; + const char* new_value; + size_t new_value_len; + + *equal = '\0'; + new_first[len] = 'B'; + ret = SetEnvironmentVariableA( new_first, equal + 1 ); + ok(ret, "SetEnvironmentVariableA failed: %lu\n", GetLastError()); + new_value = getenv(new_first); + new_value_len = strlen(new_value); + ok(!strcmp(&new_value[new_value_len - 1], "A"), "_environ[] shouldn't have been updated\n"); + *equal = '='; + } + + /* reset original value */ + new_first[len] = '\0'; + _putenv( new_first ); + + free( new_first ); + } + else skip( "Not enough env variables to run this test\n" ); }
static void test__wenviron(void) @@ -232,6 +281,52 @@ static void test__wenviron(void) break; } } + + if ((*p_wenviron)[0] && (*p_wenviron)[1]) + { + WCHAR *first = (*p_wenviron)[0]; + WCHAR *second = (*p_wenviron)[1]; + WCHAR *new_first; + WCHAR *equal; + size_t len = wcslen(first); + + new_first = malloc( (len + 1 + 1) * sizeof(WCHAR) ); + ok( new_first != NULL, "allocation failed\n" ); + wcscpy( new_first, first ); + wcscat( new_first, L"A" ); + _wputenv( new_first ); + + todo_wine + ok( second == (*p_wenviron)[1], "_wenviron[1] shouldn't have changed\n" ); + ok( !wcscmp( new_first, (*p_wenviron)[0] ), "_wenviron[1] shouldn't have changed\n" ); + todo_wine + ok( !wcscmp( second, (*p_wenviron)[1] ), "_wenviron[1] shouldn't have changed\n" ); + + /* msvcrt _environ isn't updated after direct kernel environ API */ + equal = wcschr( new_first, L'=' ); + if (equal) + { + BOOL ret; + const WCHAR* new_value; + size_t new_value_len; + + *equal = L'\0'; + new_first[len] = 'B'; + ret = SetEnvironmentVariableW( new_first, equal + 1 ); + ok(ret, "SetEnvironmentVariableW failed: %lu\n", GetLastError()); + new_value = _wgetenv(new_first); + new_value_len = wcslen(new_value); + ok(!wcscmp(&new_value[new_value_len - 1], L"A"), "_wenviron[] shouldn't have been updated\n"); + *equal = L'='; + } + + /* reset original value */ + new_first[len] = L'\0'; + _wputenv( new_first ); + + free( new_first ); + } + else skip( "Not enough env variables to run this test\n" ); }
static void test_environment_manipulation(void)
From: Eric Pouech epouech@codeweavers.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55835
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/msvcrt/data.c | 271 ++++++++++++++++++++++++++---------- dlls/msvcrt/environ.c | 19 +-- dlls/msvcrt/msvcrt.h | 5 +- dlls/msvcrt/tests/environ.c | 4 - 4 files changed, 209 insertions(+), 90 deletions(-)
diff --git a/dlls/msvcrt/data.c b/dlls/msvcrt/data.c index d1f514fd460..3037128bb8b 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,189 @@ 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_createA( 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 */ + 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++; + } + if ((blk = HeapAlloc( GetProcessHeap(), 0, count * sizeof(char*) ))) + { + int i = 0; + for (ptr = environ_strings; *ptr; ptr += strlen( ptr ) + 1) + { + /* Skip special environment strings set by the command shell */ + if (*ptr != '=') blk[i++] = strdup( ptr ); + } + blk[i] = NULL; + } + FreeEnvironmentStringsA( environ_strings ); + return blk; +}
- if (blk) +int msvcrt_update_environmentA( const char *name, const char *value ) +{ + int i = 0; + size_t len = strlen( name ); + char *set; + char **new; + + if (value[0]) { - 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; + size_t sz = strlen( name ) + 1 + strlen( value ) + 1; + set = malloc( sz ); + if (!set) return -1; + snprintf( set, sz, "%s=%s", name, value ); } - FreeEnvironmentStringsA(environ_strings); - return blk; + 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_SnapshotOfEnvironmentW(wchar_t **wblk) +wchar_t **msvcrt_clone_environmentA2W( char **env ) { - wchar_t* wenviron_strings = GetEnvironmentStringsW(); - int count = 1, len = 1, i = 0; /* keep space for the trailing NULLS */ - wchar_t *wptr; + int count; + wchar_t **ret;
- 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) + 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__wenviron) 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) { - 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; + free( set ); + return -1; } - FreeEnvironmentStringsW(wenviron_strings); - return wblk; + 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 +551,9 @@ 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__environ = env_createA(); + MSVCRT___initenv = env_createA(); + MSVCRT___winitenv = msvcrt_clone_environmentA2W(MSVCRT___initenv);
MSVCRT__pgmptr = HeapAlloc(GetProcessHeap(), 0, MAX_PATH); if (MSVCRT__pgmptr) @@ -461,15 +574,31 @@ 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); + env_freeA(MSVCRT___initenv); + env_freeW(MSVCRT___winitenv); + env_freeA(MSVCRT__environ); + env_freeW(MSVCRT__wenviron); HeapFree(GetProcessHeap(), 0, MSVCRT__pgmptr); HeapFree(GetProcessHeap(), 0, MSVCRT__wpgmptr); HeapFree(GetProcessHeap(), 0, wargv_expand); @@ -565,7 +694,7 @@ 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); + MSVCRT__wenviron = msvcrt_clone_environmentA2W(MSVCRT__environ); *argc = MSVCRT___argc; *wargv = MSVCRT___wargv; *wenvp = MSVCRT___winitenv; 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 54f8650db80..d08304b3bd6 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -154,10 +154,8 @@ static void test__environ(void) strcat( new_first, "A" ); _putenv( new_first );
- todo_wine ok( second == (*p_environ)[1], "_environ[1] shouldn't have changed\n" ); ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[1] shouldn't have changed\n" ); - todo_wine ok( !strcmp( second, (*p_environ)[1] ), "_environ[1] shouldn't have changed\n" );
/* msvcrt _environ isn't updated after direct kernel environ API */ @@ -296,10 +294,8 @@ static void test__wenviron(void) wcscat( new_first, L"A" ); _wputenv( new_first );
- todo_wine ok( second == (*p_wenviron)[1], "_wenviron[1] shouldn't have changed\n" ); ok( !wcscmp( new_first, (*p_wenviron)[0] ), "_wenviron[1] shouldn't have changed\n" ); - todo_wine ok( !wcscmp( second, (*p_wenviron)[1] ), "_wenviron[1] shouldn't have changed\n" );
/* msvcrt _environ isn't updated after direct kernel environ API */
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=139604
Your paranoid android.
=== debian11b (64 bit WoW report) ===
ddraw: ddraw1.c:14077: Test failed: 4294967295 references left. ddraw2.c:15068: Test failed: 4294967295 references left. ddraw4.c:17805: Test failed: 4294967295 references left. ddraw7.c:17834: Test failed: 4294967295 references left.
pdh: pdh.c:104: Test failed: PdhCloseQuery failed 0x00000000 pdh.c:128: Test failed: PdhCloseQuery failed 0x00000000
Bartosz Kosiorek (@gang65) commented about dlls/msvcrt/tests/environ.c:
strcat( new_first, "A" ); _putenv( new_first );
todo_wine ok( second == (*p_environ)[1], "_environ[1] shouldn't have changed\n" ); ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[1] shouldn't have changed\n" );
```suggestion:-0+0 ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[0] shouldn't have changed\n" ); ```
Bartosz Kosiorek (@gang65) commented about dlls/msvcrt/tests/environ.c:
- {
WCHAR *first = (*p_wenviron)[0];
WCHAR *second = (*p_wenviron)[1];
WCHAR *new_first;
WCHAR *equal;
size_t len = wcslen(first);
new_first = malloc( (len + 1 + 1) * sizeof(WCHAR) );
ok( new_first != NULL, "allocation failed\n" );
wcscpy( new_first, first );
wcscat( new_first, L"A" );
_wputenv( new_first );
todo_wine
ok( second == (*p_wenviron)[1], "_wenviron[1] shouldn't have changed\n" );
ok( !wcscmp( new_first, (*p_wenviron)[0] ), "_wenviron[1] shouldn't have changed\n" );
```suggestion:-0+0 ok( !wcscmp( new_first, (*p_wenviron)[0] ), "_wenviron[0] shouldn't have changed\n" ); ```
Minor, but could you please correct "distrinct" to "distinct" in the commit message?