[PATCH v2 0/2] MR4774: msvcrt: Compare environmental variable names case insensitively

[Zsolt Vadász (＠zsoltiv)]

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

[PATCH v2 1/2] msvcrt: Compare environmental variable names case insensitively

[Zsolt Vadasz]

From: Zsolt Vadasz

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;

[PATCH v2 2/2] msvcrt/tests: Test case insensitivity of getenv() and _wgetenv()

[Zsolt Vadasz]

From: Zsolt Vadasz

---
 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();
 }

[Zsolt Vadász (＠zsoltiv)]

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.

[eric pouech (＠epo)]

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

[Zsolt Vadász (＠zsoltiv)]

> 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?

[Piotr Caban (＠piotr)]

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.