[PATCH 0/1] MR4471: msvcrt: Hold _ENV_LOCK when accessing environment variables.

[Piotr Caban (＠piotr)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4471

[PATCH 1/1] msvcrt: Hold _ENV_LOCK when accessing environment variables.

[Piotr Caban]

From: Piotr Caban <piotr(a)codeweavers.com>

---
 dlls/msvcrt/environ.c | 59 +++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/dlls/msvcrt/environ.c b/dlls/msvcrt/environ.c
index 3c90ec4bf21..67f8472e82f 100644
--- a/dlls/msvcrt/environ.c
+++ b/dlls/msvcrt/environ.c
@@ -21,6 +21,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
  */
 #include "msvcrt.h"
+#include "mtdll.h"
 #include <winnls.h>
 #include "wine/debug.h"
 
@@ -220,9 +221,14 @@ static char * getenv_helper(const char *name)
  */
 char * CDECL getenv(const char *name)
 {
+    char *ret;
+
     if (!MSVCRT_CHECK_PMT(name != NULL)) return NULL;
 
-    return getenv_helper(name);
+    _lock(_ENV_LOCK);
+    ret = getenv_helper(name);
+    _unlock(_ENV_LOCK);
+    return ret;
 }
 
 static wchar_t * wgetenv_helper(const wchar_t *name)
@@ -242,9 +248,14 @@ static wchar_t * wgetenv_helper(const wchar_t *name)
  */
 wchar_t * CDECL _wgetenv(const wchar_t *name)
 {
+    wchar_t *ret;
+
     if (!MSVCRT_CHECK_PMT(name != NULL)) return NULL;
 
-    return wgetenv_helper(name);
+    _lock(_ENV_LOCK);
+    ret = wgetenv_helper(name);
+    _unlock(_ENV_LOCK);
+    return ret;
 }
 
 static int putenv_helper(const char *name, const char *val, const char *eq)
@@ -253,7 +264,10 @@ static int putenv_helper(const char *name, const char *val, const char *eq)
     char *env;
     int r;
 
-    if (env_init(FALSE, TRUE)) return -1;
+    _lock(_ENV_LOCK);
+    r = env_init(FALSE, TRUE);
+    _unlock(_ENV_LOCK);
+    if (r) return -1;
 
     if (eq)
     {
@@ -279,7 +293,9 @@ static int putenv_helper(const char *name, const char *val, const char *eq)
         return -1;
     }
 
+    _lock(_ENV_LOCK);
     r = env_set(&env, &wenv);
+    _unlock(_ENV_LOCK);
     free(env);
     free(wenv);
     return r;
@@ -303,7 +319,10 @@ static int wputenv_helper(const wchar_t *name, const wchar_t *val, const wchar_t
     char *env;
     int r;
 
-    if (env_init(TRUE, TRUE)) return -1;
+    _lock(_ENV_LOCK);
+    r = env_init(TRUE, TRUE);
+    _unlock(_ENV_LOCK);
+    if (r) return -1;
 
     if (eq)
     {
@@ -329,7 +348,9 @@ static int wputenv_helper(const wchar_t *name, const wchar_t *val, const wchar_t
         return -1;
     }
 
+    _lock(_ENV_LOCK);
     r = env_set(&env, &wenv);
+    _unlock(_ENV_LOCK);
     free(env);
     free(wenv);
     return r;
@@ -482,12 +503,17 @@ int CDECL getenv_s(size_t *ret_len, char* buffer, size_t len, const char *varnam
     if (!MSVCRT_CHECK_PMT((buffer && len > 0) || (!buffer && !len))) return EINVAL;
     if (buffer) buffer[0] = 0;
 
-    if (!(e = getenv_helper(varname))) return 0;
-    *ret_len = strlen(e) + 1;
-    if (!len) return 0;
-    if (len < *ret_len) return ERANGE;
+    _lock(_ENV_LOCK);
+    e = getenv_helper(varname);
+    if (e)
+    {
+        *ret_len = strlen(e) + 1;
+        if (len >= *ret_len) strcpy(buffer, e);
+    }
+    _unlock(_ENV_LOCK);
 
-    strcpy(buffer, e);
+    if (!e || !len) return 0;
+    if (len < *ret_len) return ERANGE;
     return 0;
 }
 
@@ -504,12 +530,17 @@ int CDECL _wgetenv_s(size_t *ret_len, wchar_t *buffer, size_t len,
     if (!MSVCRT_CHECK_PMT((buffer && len > 0) || (!buffer && !len))) return EINVAL;
     if (buffer) buffer[0] = 0;
 
-    if (!(e = wgetenv_helper(varname))) return 0;
-    *ret_len = wcslen(e) + 1;
-    if (!len) return 0;
-    if (len < *ret_len) return ERANGE;
+    _lock(_ENV_LOCK);
+    e = wgetenv_helper(varname);
+    if (e)
+    {
+        *ret_len = wcslen(e) + 1;
+        if (len >= *ret_len) wcscpy(buffer, e);
+    }
+    _unlock(_ENV_LOCK);
 
-    wcscpy(buffer, e);
+    if (!e || !len) return 0;
+    if (len < *ret_len) return ERANGE;
     return 0;
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/4471

[eric pouech (＠epo)]

Out of curiosity, MSDN states that these functions are not thread safe (but without further details on ucrt vs msvcrt and various versions). Do you have evidence it's needed?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4471#note_53339

[Piotr Caban (＠piotr)]

On Wed Nov 22 14:34:50 2023 +0000, eric pouech wrote:
> Out of curiosity, MSDN states that these functions are not thread safe
> (but without further details on ucrt vs msvcrt and various versions). Do
> you have evidence it's needed?
> MSDN is wrong (except of getenv that is not thread-safe even with the locking). I have tested it with something similar to the attached patch. Note that the lock number doesn't match, it's internal so it should be ok. Probably our mtdll.h is outdated.

I have tested it in msvcrt and msvcr90. I have also tried modifying the environment from multiple threads and it didn't get corrupted (no entries were missing).

[test.diff](/uploads/a1bdd78cb1e652113c4c9208a1212cd1/test.diff)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4471#note_53343

[eric pouech (＠epo)]

thanks for the detail. Another wrong entry in MSDN.

You may also want add locking in \\\_(w)dupenv\\\_s  because of the returned values from (w)getenv

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4471#note_53365