[PATCH 0/1] MR2446: mscoree/tests: Avoid temporary directory name collisions.
test_loadpaths_execute() was sometimes getting ERROR_ALREADY_EXISTS when creating its temporary directoryi. That was probably because GetTempFileNameW() only uses the low 16-bits of the 'random' LUID which goes over a wider range, potentially leading to collisions in the lower 16-bits. Avoid AllocateLocallyUniqueId() entirely and add a helper function to create a guaranteed new directory. Also try to use the current directory first as it is assumed to have been configured to ward off anti-virus programs (specifically Microsoft Defender) which is especially important when writing executables to that location. Only switch to the system's temporary directory if the current directory is not a writable location. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54560 --- There are a few other tests where I suspect occasional anti-virus interference. It may make sense to reuse this function in those. That said create_new_dir() uses entirely predictable sequenitial numbers which would be bad if attempting to create a secure directory. But the new directory does not even have special permissions and this type of attack shouldn't be an issue for the tests. Still, don't reuse this code in contexts where security matters. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2446
From: Francois Gouget <fgouget(a)codeweavers.com> test_loadpaths_execute() was sometimes getting ERROR_ALREADY_EXISTS when creating its temporary directoryi. That was probably because GetTempFileNameW() only uses the low 16-bits of the 'random' LUID which goes over a wider range, potentially leading to collisions in the lower 16-bits. Avoid AllocateLocallyUniqueId() entirely and add a helper function to create a guaranteed new directory. Also try to use the current directory first as it is assumed to have been configured to ward off anti-virus programs (specifically Microsoft Defender) which is especially important when writing executables to that location. Only switch to the system's temporary directory if the current directory is not a writable location. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54560 --- There are a few other tests where I suspect occasional anti-virus interference. It may make sense to reuse this function in those. That said create_new_dir() uses entirely predictable sequenitial numbers which would be bad if attempting to create a secure directory. But the new directory does not even have special permissions and this type of attack shouldn't be an issue for the tests. Still, don't reuse this code in contexts where security matters. --- dlls/mscoree/tests/mscoree.c | 55 +++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/dlls/mscoree/tests/mscoree.c b/dlls/mscoree/tests/mscoree.c index e5065a41d88..adac72e5f4c 100644 --- a/dlls/mscoree/tests/mscoree.c +++ b/dlls/mscoree/tests/mscoree.c @@ -592,25 +592,55 @@ static BOOL compile_cs(const WCHAR *source, const WCHAR *target, const WCHAR *ty return ret; } +static BOOL create_new_dir(WCHAR newdir[MAX_PATH], const WCHAR* parent, + const WCHAR* prefix) +{ + WCHAR path[MAX_PATH]; + BOOL try_tmpdir = TRUE; + static unsigned i = 0; + + if (!parent) + { + GetCurrentDirectoryW(ARRAY_SIZE(path), path); + parent = path; + } + while (1) + { + swprintf(newdir, MAX_PATH, L"%s\\%s%04d", path, prefix, i); + if (CreateDirectoryW(newdir, NULL)) + return TRUE; + switch (GetLastError()) + { + case ERROR_ACCESS_DENIED: + if (!try_tmpdir) + return FALSE; + try_tmpdir = FALSE; + GetTempPathW(ARRAY_SIZE(path), path); + path[wcslen(path) - 1] = 0; /* redundant trailing backslash */ + parent = path; + break; + case ERROR_ALREADY_EXISTS: + i++; + break; + default: + return FALSE; + } + } +} + static void test_loadpaths_execute(const WCHAR *exe_name, const WCHAR *dll_name, const WCHAR *cfg_name, const WCHAR *dll_dest, BOOL expect_failure, BOOL todo) { - WCHAR tmp[MAX_PATH], tmpdir[MAX_PATH], tmpexe[MAX_PATH], tmpcfg[MAX_PATH], tmpdll[MAX_PATH]; + WCHAR tmpdir[MAX_PATH], tmpexe[MAX_PATH], tmpcfg[MAX_PATH], tmpdll[MAX_PATH]; PROCESS_INFORMATION pi; STARTUPINFOW si = { 0 }; WCHAR *ptr, *end; DWORD exit_code = 0xdeadbeef; - LUID id; BOOL ret; - GetTempPathW(MAX_PATH, tmp); - ret = AllocateLocallyUniqueId(&id); - ok(ret, "AllocateLocallyUniqueId failed: %lu\n", GetLastError()); - ret = GetTempFileNameW(tmp, L"loadpaths", id.LowPart, tmpdir); - ok(ret, "GetTempFileNameW failed: %lu\n", GetLastError()); - - ret = CreateDirectoryW(tmpdir, NULL); - ok(ret, "CreateDirectoryW(%s) failed: %lu\n", debugstr_w(tmpdir), GetLastError()); + ok(create_new_dir(tmpdir, NULL, L"loadpaths"), + "failed to create a new dir %lu\n", GetLastError()); + end = tmpdir + wcslen(tmpdir); wcscpy(tmpexe, tmpdir); PathAppendW(tmpexe, exe_name); @@ -673,9 +703,8 @@ static void test_loadpaths_execute(const WCHAR *exe_name, const WCHAR *dll_name, ret = DeleteFileW(tmpexe); ok(ret, "DeleteFileW(%s) failed: %lu\n", debugstr_w(tmpexe), GetLastError()); - end = tmpdir + wcslen(tmp); - ptr = tmpdir + wcslen(tmpdir) - 1; - while (ptr > end && (ptr = wcsrchr(tmpdir, '\\'))) + ptr = end; + while (ptr >= end && (ptr = wcsrchr(tmpdir, '\\'))) { ret = RemoveDirectoryW(tmpdir); ok(ret, "RemoveDirectoryW(%s) failed: %lu\n", debugstr_w(tmpdir), GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2446
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2446
participants (3)
-
Esme Povirk (＠madewokherd) -
Francois Gouget
-
Francois Gouget (＠fgouget)