[PATCH v2 0/2] 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.
-- v2: msvcrt: Let each _environ[] entry have a distinct allocation block. msvcrt: Add test about _environ[] entries allocation.
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..c23852ad7b2 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[0] 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[0] 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 | 5 +- 4 files changed, 210 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 c23852ad7b2..884cb510e57 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -154,10 +154,9 @@ 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[0] shouldn't have changed\n" ); - todo_wine + ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[1] shouldn't have changed\n" ); 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 +295,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[0] 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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139682
Your paranoid android.
=== build (build log) ===
error: patch failed: dlls/msvcrt/tests/environ.c:154 Task: Patch failed to apply
=== debian11 (build log) ===
error: patch failed: dlls/msvcrt/tests/environ.c:154 Task: Patch failed to apply
=== debian11b (build log) ===
error: patch failed: dlls/msvcrt/tests/environ.c:154 Task: Patch failed to apply
On Tue Nov 7 09:12:21 2023 +0000, Bartosz Kosiorek wrote:
ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[0] shouldn't have changed\n" );
done in v2
On Tue Nov 7 09:11:01 2023 +0000, eric pouech wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/4313/diffs?diff_id=81863&start_sha=dd957f0475ffd13bbe2f3267a39f97cd3dca5116#83168949dbef962f714c49143372dbcb672bee8d_298_299)
done in V2
On Tue Nov 7 09:12:42 2023 +0000, Alex Henrie wrote:
Minor, but could you please correct "distrinct" to "distinct" in the commit message?
done in V2
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/environ.c:
break; } }
- /* force change in _environ[], seems like native needs a first update to set allocation per entry */
It suggest we're missing something. Please add following test to show that it initially returns environment block address: `ok(envp == *p_environ, "envp = %p, *p_environ = %p\n", envp, *p_environ);` It will also make some of our current test redundant since there's no need to compare envp and p_environ content if it points to the same buffer.
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/environ.c:
break; } }
- /* force change in _environ[], seems like native needs a first update to set allocation per entry */
- _putenv( "__fish=cat" );
- _putenv( "__fish=" );
Please temporarily add 2 variables to the block so the test doesn't have to be skipped in any case. Also it looks that you're not gaining anything by modifying original environment. After adding new environment variables, you can do the tests on them. I think it will be cleaner.
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/environ.c:
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 = '=';}
Please add this test in a separate patch. Also please move it to `test_environment_manipulation` function. It would be also nice to change the test so it fails in current wine, I guess that something like: ```c ret = SetEnvironmentVariableA("cat", "test"); ok(ret, ...); _putenv("dog", "test"); val = getenv("cat"); ok(!val, ...); ``` should fail.
Piotr Caban (@piotr) commented about dlls/msvcrt/tests/environ.c:
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_wineok( second == (*p_environ)[1], "_environ[1] shouldn't have changed\n" );ok( !strcmp( new_first, (*p_environ)[0] ), "_environ[0] shouldn't have changed\n" );todo_wineok( !strcmp( second, (*p_environ)[1] ), "_environ[1] shouldn't have changed\n" );
If `second == (*p_environ)[1]` it doesn't make sense to compare the buffers.
On Tue Nov 7 12:26:23 2023 +0000, Piotr Caban wrote:
It suggest we're missing something. Please add following test to show that it initially returns environment block address: `ok(envp == *p_environ, "envp = %p, *p_environ = %p\n", envp, *p_environ);` It will also make some of our current test redundant since there's no need to compare envp and p_environ content if it points to the same buffer.
as I wrote in cover of MR, IMO the creation of per-entry allocation is done **after** the first *putenv call
I'll rewrite the test & patch accordingly then
On Tue Nov 7 12:26:25 2023 +0000, Piotr Caban wrote:
If `second == (*p_environ)[1]` it doesn't make sense to compare the buffers.
I don't agree. It shows that the memory hasn't been (re)allocated elsewhere, so that keeping the pointer around is valid across putenv calls. and we need also to ensure string content hasn't been changed.
for clarity I'll add the same test for getenv()which behaves the same way
On Tue Nov 7 13:09:06 2023 +0000, eric pouech wrote:
I don't agree. It shows that the memory hasn't been (re)allocated elsewhere, so that keeping the pointer around is valid across putenv calls. and we need also to ensure string content hasn't been changed. for clarity I'll add the same test for getenv()which behaves the same way
In order to check that string content didn't change you will need to copy the buffer earlier. Calling strcmp(ptr, ptr) doesn't make sense.
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Piotr Caban (＠piotr)