[PATCH 0/1] MR6140: ntdll: Exclude biggest envars to fit 32767 chars limit in get_initial_environment.
Windows has a limit of 32767 characters in the environment block. https://superuser.com/questions/1070272/why-does-windows-have-a-limit-on-env... Wine doesn't attempt to respect that at all. Do some programs actually depend on this? Unfortunately yes. With this patch, during initialization, if the block size would exceed (or be close to) the limit, the biggest environment variables will be excluded. This is useful when a user has very long environment variables in their system for reasons unrelated to Wine. Do note that there's still nothing done to make sure that the limit isn't exceeded after initialization. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56941 Signed-off-by: Martino Fontana <tinozzo123(a)gmail.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140
From: Martino Fontana <tinozzo123(a)gmail.com> Windows has a limit of 32767 characters in the environment block. https://superuser.com/questions/1070272/why-does-windows-have-a-limit-on-env... Wine doesn't attempt to respect that at all. Do some programs actually depend on this? Unfortunately yes. With this patch, during initialization, if the block size would exceed (or be close to) the limit, the biggest environment variables will be excluded. This is useful when a user has very long environment variables in their system for reasons unrelated to Wine. Do note that there's still nothing done to make sure that the limit isn't exceeded after initialization. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56941 Signed-off-by: Martino Fontana <tinozzo123(a)gmail.com> --- dlls/kernelbase/process.c | 2 -- dlls/ntdll/unix/env.c | 47 ++++++++++++++++++++++++++++++++++----- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 2d08481cd35..835cdf654fc 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1610,8 +1610,6 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value NTSTATUS status; DWORD len, ret; - /* limit the size to sane values */ - size = min( size, 32767 ); if (!(valueW = HeapAlloc( GetProcessHeap(), 0, size * sizeof(WCHAR) ))) return 0; RtlCreateUnicodeStringFromAsciiz( &us_name, name ); diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c index 36949b905fb..4c29921ddbc 100644 --- a/dlls/ntdll/unix/env.c +++ b/dlls/ntdll/unix/env.c @@ -919,6 +919,15 @@ static const char overrides_help_message[] = "Example:\n" " WINEDLLOVERRIDES=\"comdlg32=n,b;shell32,shlwapi=b\"\n"; +struct str_len { + char *str; + size_t len; +}; + +static int cmp_str_len(const void *a, const void *b) { + return ((struct str_len*) a)->len - ((struct str_len*) b)->len; +} + /************************************************************************* * get_initial_environment * @@ -926,20 +935,46 @@ static const char overrides_help_message[] = */ static WCHAR *get_initial_environment( SIZE_T *pos, SIZE_T *size ) { - char **e; WCHAR *env, *ptr, *end; + SIZE_T i, env_count = 0; + struct str_len *env_vars; + /* Windows has a limit of 32767 characters for the environment block, and + * some programs malfunction if this is exceeded. As a workaround, don't add + * the biggest environment variables. Leave some space for variables that + * the program itself might add. */ + /* FIXME: Prevent exceeding the limit after initialization. */ + const SIZE_T MAX_ENV_SIZE = 30000; + + while (main_envp[env_count]) env_count++; + + env_vars = calloc(env_count, sizeof(struct str_len)); /* estimate needed size */ *size = 1; - for (e = main_envp; *e; e++) *size += strlen(*e) + 1; + for (i = 0; i < env_count; i++) { + env_vars[i].str = main_envp[i]; + env_vars[i].len = strlen(main_envp[i]) + 1; + *size += env_vars[i].len; + } + if (*size > MAX_ENV_SIZE){ + /* sort environment variables by length in ascending order, to exclude + * only the biggest ones */ + qsort(env_vars, env_count, sizeof(struct str_len), cmp_str_len); + *size = MAX_ENV_SIZE; + } env = malloc( *size * sizeof(WCHAR) ); ptr = env; end = env + *size - 1; - for (e = main_envp; *e && ptr < end; e++) - { - char *str = *e; + for (i = 0; i < env_count; i++) { + char *str = env_vars[i].str; + if (ptr - env + env_vars[i].len >= MAX_ENV_SIZE) + { + WARN("Environment variables block exceeds or is close to the 32767 " + "characters limit. Removing the biggest variables.\n"); + break; + } /* skip Unix special variables and use the Wine variants instead */ if (!strncmp( str, "WINE", 4 )) { @@ -947,6 +982,7 @@ static WCHAR *get_initial_environment( SIZE_T *pos, SIZE_T *size ) else if (!strcmp( str, "WINEDLLOVERRIDES=help" )) { MESSAGE( overrides_help_message ); + free(env_vars); exit(0); } } @@ -956,6 +992,7 @@ static WCHAR *get_initial_environment( SIZE_T *pos, SIZE_T *size ) ptr += ntdll_umbstowcs( str, strlen(str) + 1, ptr, end - ptr ); } *pos = ptr - env; + free(env_vars); return env; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6140
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=147273 Your paranoid android. === debian11b (64 bit WoW report) === ddraw: ddraw4.c:3969: Test failed: Expected message 0x5, but didn't receive it.
1) please start by adding tests, also exhibiting which component enforces the limitation (you can start by kernel32/kernelbase, msvcrt/ucrtbase could also play some role at some point) 2) I'm not sure that not inheriting some env variables (whatever the criteria) (in the back of the user) is a good idea IMO, it's safer (in cases like yours) to wrap wine inside a shell script that will reset the unneeded env variables (and decide here what are the variables that can be dropped). 3) enforcing the 32k limit on process creation and on variable creation/modification shall be desirable in Wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76777
About 2., there are problems in asking the user to wrap Wine inside a shell script: - If an app is broken because of this issue, a user might not have the slightest idea that it's related to environment variables. - Wrapping Wine inside a shell script can cause even more issues, since it removes the variables _before_ Wine is launched. For instance, Steam uses `XDG_DATA_DIRS` in order to launch its runtime, so by unsetting it in its launch options (since, for instance, it's more than 20000 characters on my end), the application won't start at all. You can work around this be cutting them partially, but [it's ugly](https://github.com/ValveSoftware/Proton/issues/6766#issuecomment-1819822798). IMO, I doubt that env variables that are this big are something that the user wanted to have inside Wine. We are talking about 5 digits length. About 1. and 3., I don't guarantee that I will be able to do this (this is my first PR here), but I will try. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76780
As Eric suggested in p. 2., this will likely break things. E. g., SDL_GAMECONTROLLER_IGNORE_DEVICES env var set by Steam is about 10k in size but it is probably needed for proper controllers operation. You are bringing in XDG_DATA_DIRS yourselves as an example, so if this will happen to be the largest variable removing it will break things even if it is removed after first Wine process is started: it won't propagate to the child processes and that will break things, the same Steam if it is not started directly as the first process or, just e.g., opening URLs in external browser. I am not sure if there is a good and universal way to safely shrink environment, so far addressing that on system setup looks like a saner choice. Maybe a test that Windows indeed doesn't accept >32k env size would be great (as it is not apparent that it is actually such a limit or it is just very uncommon to have big env on Windows). FWIW we have a hack in Proton made for some game with the similar issue (removing some named env vars) but it is in effect for specific game only, doing that for every app would break things (and such hacks and app-specific workarounds are not accepted to upstream Wine). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76786
it won't propagate to the child processes and that will break things [...] e.g., opening URLs in external browser
Only the Windows apps won't see these variables, but on the Unix side, all the envars are still there.
SDL_GAMECONTROLLER_IGNORE_DEVICES env var set by Steam is about 10k in size but it is probably needed for proper controllers operation
AFAIK, it's Proton/Wine itself that uses this envar, not the Windows app itself, so nothing should be broken even if it's "removed". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76895
Could you check whether the attached patch would fix the problem as well ? [0001-clean-all-XDG_envvar.patch](/uploads/f79c042a444ffccf22266f0d90389a6f/0001-clean-all-XDG_envvar.patch) If that would work, one might switch from blacklisting envvars to a whitelisting of envvars. (as suggested in https://bugs.winehq.org/show_bug.cgi?id=56941#c3) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76926
Whitelisting would be a bad idea. Think of software that runs under Windows but is meant to be used with Wine, like DXVK. These can be configured with (short) environment variables that need to be present in the Windows environment. Now, it obviously wouldn't be hard to whitelist DXVK specifically, but think about less popular software that does the same thing. Having to be whitelisted by Wine is unfair gatekeeping. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76933
The attribute "unfair" does not seem suitable here, because when some XDG_* envvars breaks wine and it application, all the "fairness" does not help. There seem to be the following options to provide a solution to bug #56941. 1) One alternative is to make the env space in wine as large as on the host system, but this could have implications on the design of wine and its compatibility to windows. If this is not a suitable approach, we need to accept that env space in windows (and wine) is limited and much smaller than on the host system, and obviously under some circumstances (see bug #56941) some arbitration need to be applied or the application will break in wine. This leads to the following options: 2) Blacklisting: certain host envvars are not provided within wine. This seems to be partially implemented (QT_*, XDG_SESSION_TYPE, ..), or some heuristical approach as suggested by this patch (e.g. remove the largest envvars). This comes at the cost to maintain the "blacklist" and its heuristics. 3) Whitelisting: only a well-defined list of host envvars are provided to the apps within wine. It comes at the cost to manage that whitelist. That might not be completely trivial as all envvars for all libraries used by wine need to be considered, including MESA_*, LIBGL_*, GL_DEBUG_*, DXVK_*, WINE*, etc. Therefore, the question does not seem to be about "fairness", but about which approach it the most effective solution. Whitelisting seems to be the most deterministic approach, because it prevents the influence of unrelated host envvars. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_76936
Please check this alternative MR !6166. Would this work for you too ? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_77198
This merge request was closed by Martino Fontana. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140
As I just commented on the bug report, the issue happens on Windows too: it's an application issue, and unfortunately it seems that the developers don't care about fixing it. Still, this workaround doesn't belong here. Closing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6140#note_118313
participants (7)
-
Alois Schloegl (＠alois.schloegl) -
Alois Schloegl (＠schloegl)
-
eric pouech (＠epo)
-
Martino Fontana
-
Martino Fontana (＠tinozzo123)
-
Marvin -
Paul Gofman (＠gofman)