[PATCH v2 0/2] MR4774: msvcrt: Compare environmental variable names case insensitively
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56134 -- v2: msvcrt/tests: Test case insensitivity of getenv() and _wgetenv() msvcrt: Compare environmental variable names case insensitively https://gitlab.winehq.org/wine/wine/-/merge_requests/4774
From: Zsolt Vadasz <zsolt_vadasz(a)protonmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56134 --- dlls/msvcrt/environ.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/msvcrt/environ.c b/dlls/msvcrt/environ.c index 063a9254c67..aa857afa615 100644 --- a/dlls/msvcrt/environ.c +++ b/dlls/msvcrt/environ.c @@ -125,7 +125,7 @@ static int env_get_index(const char *name) len = strlen(name); for (i = 0; MSVCRT__environ[i]; i++) { - if (!strncmp(name, MSVCRT__environ[i], len) && MSVCRT__environ[i][len] == '=') + if (!strnicmp(name, MSVCRT__environ[i], len) && MSVCRT__environ[i][len] == '=') return i; } return i; @@ -138,7 +138,7 @@ static int wenv_get_index(const wchar_t *name) len = wcslen(name); for (i = 0; MSVCRT__wenviron[i]; i++) { - if (!wcsncmp(name, MSVCRT__wenviron[i], len) && MSVCRT__wenviron[i][len] == '=') + if (!wcsnicmp(name, MSVCRT__wenviron[i], len) && MSVCRT__wenviron[i][len] == '=') return i; } return i; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4774
From: Zsolt Vadasz <zsolt_vadasz(a)protonmail.com> --- dlls/msvcrt/tests/environ.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/msvcrt/tests/environ.c b/dlls/msvcrt/tests/environ.c index aab14a422b3..afc1f13ce06 100644 --- a/dlls/msvcrt/tests/environ.c +++ b/dlls/msvcrt/tests/environ.c @@ -421,6 +421,18 @@ static void test_child_env(char** argv) free( env ); } +static void test_case_insensitive(void) +{ + const char *uppercase_env = getenv("APPDATA"); + const char *lowercase_env = getenv("appdata"); + const wchar_t *uppercase_wenv = _wgetenv(L"APPDATA"); + const wchar_t *lowercase_wenv = _wgetenv(L"appdata"); + ok (uppercase_env == lowercase_env, "getenv() must be case insensitive, %p should be %p\n", + lowercase_env, uppercase_env); + ok (uppercase_wenv == lowercase_wenv, "_wgetenv() must be case insensitive, %p should be %p\n", + lowercase_wenv, uppercase_wenv); +} + START_TEST(environ) { char **argv; @@ -443,4 +455,5 @@ START_TEST(environ) test_environment_manipulation(); test_child_env(argv); test_system(); + test_case_insensitive(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4774
On Tue Jan 2 17:38:41 2024 +0000, eric pouech wrote:
thanks, looks like a regression added recently please update your patch with: * proper test case so that this error doesn't creep in again (like changing case in some of existing tests) * add similar change to ANSI version of APIs which suffers from same bug TIA applied the same fix to getenv() and added tests, although i'm not sure if they're sufficient enough.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4774#note_56582
On Tue Jan 2 17:38:41 2024 +0000, Zsolt Vadász wrote:
applied the same fix to getenv() and added tests, although i'm not sure if they're sufficient enough. the first patch is supposed to pass the tests without the second applied
so you need a todo_wine clause in from of the failing tests in first patch, and to remove it in second patch when the fix is applied I'll let @piotr decide on how far we need the tests to go. A few remarks (don't take them for granted until Piotr jumps in) * we can either have dedicated function to test case sensitvity (or the lack thereof) as you did, or we could insert these bits in the existing ones, by adding (or modifying) a new test with case sensitivity * I'm not fully convinced we need to test all the APIs. at least one per ANSI/UNICODE family is needed to ensure that we don't break things. it would be surprising that getenv and putenv have different case sensitivity. We don't tend to 100% coverage in the tests -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4774#note_56583
the first patch is supposed to pass the tests without the second applied
so you need a todo_wine clause in from of the failing tests in first patch, and to remove it in second patch when the fix is applied
i'm not sure i understand. the failing tests are unrelated to my patch. could you rephrase this part? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4774#note_56584
On Tue Jan 2 18:37:46 2024 +0000, Zsolt Vadász wrote:
the first patch is supposed to pass the tests without the second applied
so you need a todo_wine clause in from of the failing tests in first patch, and to remove it in second patch when the fix is applied i'm not sure i understand. the failing tests are unrelated to my patch. could you rephrase this part? First patch is fixing the implementation, second patch adds tests so there's no need to add todo_wine.
I would have changed existing tests to add case-sensitive case but adding a dedicated function is also OK. Environment variables are not case-sensitive on Windows. I think it's OK to assume that C-runtime is not doing it differently. I think we can also add a test for putenv mainly to ensure the implementation is correct in this regard. The patch looks good for me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4774#note_56585
participants (4)
-
eric pouech (＠epo) -
Piotr Caban (＠piotr)
-
Zsolt Vadasz
-
Zsolt Vadász