[PATCH v9 0/4] MR6855: server: Fix renaming file to a hardlink of itself.
-- v9: server: Handle hardlinks and casefolding when renaming the same file. server: Handle renames to destinations containing trailing slashes. kernel32/tests: Test renaming a file into a hardlink of itself. kernel32/tests: Use FindClose instead of CloseHandle when closing https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 1a1505060cd..3f3646b6979 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -1994,9 +1994,8 @@ static void test_MoveFileA(void) char tempdir[MAX_PATH]; char source[MAX_PATH], dest[MAX_PATH]; static const char prefix[] = "pfx"; + HANDLE hfile, hfind, hmapfile; WIN32_FIND_DATAA find_data; - HANDLE hfile; - HANDLE hmapfile; DWORD ret; BOOL retok; @@ -2076,14 +2075,14 @@ static void test_MoveFileA(void) ret = MoveFileA(source, tempdir); ok(ret, "MoveFileA: failed, error %ld\n", GetLastError()); - hfile = FindFirstFileA(tempdir, &find_data); - ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); - if (hfile != INVALID_HANDLE_VALUE) + hfind = FindFirstFileA(tempdir, &find_data); + ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfind != INVALID_HANDLE_VALUE) { todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), "MoveFile failed to change casing on same file: got %s\n", find_data.cFileName); } - CloseHandle(hfile); + FindClose(hfind); /* test renaming another file "Remove Be" to "Remove Me", which replaces the existing "Remove me" */ tempdir[lstrlenA(tempdir) - 2] = 'B'; @@ -2100,14 +2099,14 @@ static void test_MoveFileA(void) tempdir[lstrlenA(tempdir) - 2] = 'm'; - hfile = FindFirstFileA(tempdir, &find_data); - ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); - if (hfile != INVALID_HANDLE_VALUE) + hfind = FindFirstFileA(tempdir, &find_data); + ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfind != INVALID_HANDLE_VALUE) { ok(!lstrcmpA(strrchr(source, '\\') + 1, find_data.cFileName), "MoveFile failed to change casing on existing target file: got %s\n", find_data.cFileName); } - CloseHandle(hfile); + FindClose(hfind); ret = DeleteFileA(tempdir); ok(ret, "DeleteFileA: error %ld\n", GetLastError()); @@ -2121,14 +2120,14 @@ static void test_MoveFileA(void) ret = MoveFileA(source, tempdir); ok(ret, "MoveFileA: failed, error %ld\n", GetLastError()); - hfile = FindFirstFileA(tempdir, &find_data); - ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); - if (hfile != INVALID_HANDLE_VALUE) + hfind = FindFirstFileA(tempdir, &find_data); + ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfind != INVALID_HANDLE_VALUE) { todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), "MoveFile failed to change casing on same directory: got %s\n", find_data.cFileName); } - CloseHandle(hfile); + FindClose(hfind); lstrcpyA(source, dest); lstrcpyA(dest, tempdir); @@ -2143,12 +2142,11 @@ static void test_MoveFileA(void) { WIN32_FIND_DATAA fd; char temppath[MAX_PATH]; - HANDLE hFind; lstrcpyA(temppath, tempdir); lstrcatA(temppath, "\\*.*"); - hFind = FindFirstFileA(temppath, &fd); - if (INVALID_HANDLE_VALUE != hFind) + hfind = FindFirstFileA(temppath, &fd); + if (INVALID_HANDLE_VALUE != hfind) { LPSTR lpName; do @@ -2158,8 +2156,8 @@ static void test_MoveFileA(void) lpName = fd.cFileName; ok(IsDotDir(lpName), "MoveFileA: wildcards file created!\n"); } - while (FindNextFileA(hFind, &fd)); - FindClose(hFind); + while (FindNextFileA(hfind, &fd)); + FindClose(hfind); } } ret = DeleteFileA(source); @@ -2893,7 +2891,7 @@ static void test_FindFirstFileA(void) if (handle == INVALID_HANDLE_VALUE) ok( err == ERROR_PATH_NOT_FOUND, "Bad Error number %d\n", err ); else - CloseHandle( handle ); + FindClose( handle ); /* try FindFirstFileA on "c:\foo\nul\bar" */ SetLastError( 0xdeadbeaf ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 3f3646b6979..4573326a6b5 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2160,10 +2160,34 @@ static void test_MoveFileA(void) FindClose(hfind); } } - ret = DeleteFileA(source); - ok(ret, "DeleteFileA: error %ld\n", GetLastError()); ret = DeleteFileA(dest); ok(!ret, "DeleteFileA: error %ld\n", GetLastError()); + + /* test renaming file to hardlink of itself with different case */ + lstrcpyA(dest, tempdir); + lstrcatA(dest, "\\hardlink"); + ret = CreateHardLinkA(dest, source, NULL); + ok(ret, "CreateHardLinkA: error %ld\n", GetLastError()); + dest[lstrlenA(dest) - 4] = 'L'; + ret = MoveFileA(source, dest); + ok(ret, "MoveFileA: error %ld\n", GetLastError()); + + hfind = FindFirstFileA(dest, &find_data); + ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfind != INVALID_HANDLE_VALUE) + { + todo_wine + ok(!lstrcmpA(strrchr(dest, '\\') + 1, find_data.cFileName), + "MoveFile failed to change casing on hardlink of itself: got %s\n", find_data.cFileName); + } + FindClose(hfind); + ret = GetFileAttributesA(source); + todo_wine + ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError()); + if(ret != INVALID_FILE_ATTRIBUTES) DeleteFileA(source); + ret = DeleteFileA(dest); + ok(ret, "DeleteFileA: error %ld\n", GetLastError()); + ret = RemoveDirectoryA(tempdir); ok(ret, "DeleteDirectoryA: error %ld\n", GetLastError()); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> They don't affect the operation even for regular files. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Also simplifies next patch. --- dlls/kernel32/tests/file.c | 18 ++++++++++++++++++ server/fd.c | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 4573326a6b5..b6c143d1408 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2185,6 +2185,24 @@ static void test_MoveFileA(void) todo_wine ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError()); if(ret != INVALID_FILE_ATTRIBUTES) DeleteFileA(source); + + /* test renaming a regular file to a name with trailing slash */ + lstrcpyA(source, dest); + lstrcpyA(strrchr(dest, '\\') + 1, "dir\\"); + ret = MoveFileA(source, dest); + ok(ret, "MoveFileA: error %ld\n", GetLastError()); + *strrchr(dest, '\\') = '\0'; + + hfind = FindFirstFileA(dest, &find_data); + ok(hfind != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfind != INVALID_HANDLE_VALUE) + { + ok(!lstrcmpA(find_data.cFileName, "dir"), + "MoveFile failed to rename regular file to name with trailing slash: got %s\n", find_data.cFileName); + } + FindClose(hfind); + ret = GetFileAttributesA(source); + ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError()); ret = DeleteFileA(dest); ok(ret, "DeleteFileA: error %ld\n", GetLastError()); diff --git a/server/fd.c b/server/fd.c index e1b969a0439..911fa4931c2 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2560,6 +2560,11 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da set_error( STATUS_OBJECT_PATH_SYNTAX_BAD ); return; } + + /* strip trailing slashes */ + while (len > 1 && nameptr[len - 1] == '/') + len--; + if (!(name = mem_alloc( len + 1 ))) return; memcpy( name, nameptr, len ); name[len] = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Based on code and suggestions by Jinoh Kang. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 2 - server/fd.c | 98 +++++++++++++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index b6c143d1408..ea905717b10 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2182,9 +2182,7 @@ static void test_MoveFileA(void) } FindClose(hfind); ret = GetFileAttributesA(source); - todo_wine ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError()); - if(ret != INVALID_FILE_ATTRIBUTES) DeleteFileA(source); /* test renaming a regular file to a name with trailing slash */ lstrcpyA(source, dest); diff --git a/server/fd.c b/server/fd.c index 911fa4931c2..c01c601237f 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2535,6 +2535,101 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags ) ((fd->options & FILE_DELETE_ON_CLOSE) ? FILE_DISPOSITION_DELETE : 0); } +/* rename the same file, dealing with casefolding and possibly different hardlinks to it */ +static void rename_same_file( const char *src, const char *dst, int is_dir ) +{ + static const char tmpname_fmt[] = ".wine-rename-tmp-%08x"; + static unsigned tmp_value; + + char *dirname, tmpname[sizeof(tmpname_fmt) + 4 /* remaining of %08x */]; + const char *srcname, *dstname = strrchr( dst, '/' ) + 1; + struct stat st, st2; + int dirfd, res = 0; + unsigned i; + + /* first, check the directories they reside in */ + if (!(dirname = memdup( dst, dstname - dst ))) + return; + dirname[dstname - dst - 1] = '\0'; + if ((res = stat( dirname, &st ))) + file_set_error(); + free( dirname ); + if (res) + return; + + srcname = strrchr( src, '/' ) + 1; + if (!(dirname = memdup( src, srcname - src ))) + return; + dirname[srcname - src - 1] = '\0'; + if ((dirfd = open( dirname, O_RDONLY | O_NONBLOCK )) == -1) + file_set_error(); + free( dirname ); + if (dirfd == -1) + return; + + if (fstat( dirfd, &st2 )) + { + file_set_error(); + goto ret; + } + + /* if different directories, it must be a hardlink, so simply remove the source */ + if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino) + { + if (unlinkat( dirfd, srcname, 0 )) + file_set_error(); + goto ret; + } + + /* same dentry means a no-op */ + if (!strcmp( srcname, dstname )) goto ret; + + /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry, + * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead, + * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the + * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and + * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */ + tmp_value += (current_time >> 16) + current_time; + for (i = 0; i < 0x8000; i++, tmp_value += 7777) + { + snprintf( tmpname, sizeof(tmpname), tmpname_fmt, tmp_value ); + if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */ + break; + } + if (i >= 0x8000) + { + set_error( STATUS_UNSUCCESSFUL ); + goto ret; + } + + if (renameat( dirfd, srcname, dirfd, tmpname )) + { + file_set_error(); + goto ret; + } + + if (!is_dir) + { + /* we can't just do rename(2) here; renaming a file to its hardlink is no-op */ + if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST) + { + file_set_error(); + /* revert the temporary rename */ + renameat( dirfd, tmpname, dirfd, srcname ); + goto ret; + } + + if (unlinkat( dirfd, tmpname, 0 )) + file_set_error(); + } + /* directories can't have hardlinks, so just rename it to the destination */ + else if (renameat( dirfd, tmpname, dirfd, dstname )) + file_set_error(); + +ret: + close( dirfd ); +} + /* set new name for the fd */ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, data_size_t len, struct unicode_str nt_name, int create_link, unsigned int flags ) @@ -2592,7 +2687,8 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da { if (!fstat( fd->unix_fd, &st2 ) && st.st_ino == st2.st_ino && st.st_dev == st2.st_dev) { - if (create_link && !replace) set_error( STATUS_OBJECT_NAME_COLLISION ); + if (!create_link) rename_same_file( fd->unix_name, name, S_ISDIR( st.st_mode ) ); + else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); free( name ); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
Jinoh Kang (@iamahuman) commented about dlls/kernel32/tests/file.c:
- FindClose(hFind); + while (FindNextFileA(hfind, &fd)); + FindClose(hfind); } } - ret = DeleteFileA(source); - ok(ret, "DeleteFileA: error %ld\n", GetLastError()); ret = DeleteFileA(dest); ok(!ret, "DeleteFileA: error %ld\n", GetLastError()); + + /* test renaming file to hardlink of itself with different case */ + lstrcpyA(dest, tempdir); + lstrcatA(dest, "\\hardlink"); + ret = CreateHardLinkA(dest, source, NULL); + ok(ret, "CreateHardLinkA: error %ld\n", GetLastError()); + dest[lstrlenA(dest) - 4] = 'L'; Nit, but consistent with the strrchr backslash and for readability
```suggestion:-0+0 *strrchr(dest, 'l') = 'L'; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97917
Jinoh Kang (@iamahuman) commented about server/fd.c:
+ } + + /* same dentry means a no-op */ + if (!strcmp( srcname, dstname )) goto ret; + + /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry, + * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead, + * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the + * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and + * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */ + tmp_value += (current_time >> 16) + current_time; + for (i = 0; i < 0x8000; i++, tmp_value += 7777) + { + snprintf( tmpname, sizeof(tmpname), tmpname_fmt, tmp_value ); + if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */ + break; If multiple wineservers were to attempt to rename the same hardlink, they may end up allocating the *same* tempname and race against each other.
This fixes the race without increasing the number of syscalls (untested): ```suggestion:-1+0 if (!is_dir) { /* to avoid TOCTTOU, probe existence of tmpname and link at the same time */ if (linkat( dirfd, srcname, dirfd, tmpname )) continue; if (unlinkat( dirfd, srcname )) { file_set_error(); unlinkat( dirfd, tmpname ); goto ret; } break; } /* directories can't have hardlinks, so just rename it to tmpname */ else if (!renameat( dirfd, srcname, dirfd, tmpname )) break; ``` (And remove the renameat srcname,tmpname below) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97918
Jinoh Kang (@iamahuman) commented about server/fd.c:
((fd->options & FILE_DELETE_ON_CLOSE) ? FILE_DISPOSITION_DELETE : 0); }
+/* rename the same file, dealing with casefolding and possibly different hardlinks to it */ +static void rename_same_file( const char *src, const char *dst, int is_dir ) +{ + static const char tmpname_fmt[] = ".wine-rename-tmp-%08x"; + static unsigned tmp_value; + + char *dirname, tmpname[sizeof(tmpname_fmt) + 4 /* remaining of %08x */]; + const char *srcname, *dstname = strrchr( dst, '/' ) + 1; + struct stat st, st2; + int dirfd, res = 0; + unsigned i; +
You should check for `"/"`; otherwise dstname will be empty and `MoveFile( "\\??\\unix\\", "\\??\\unix\\" )` fails with a cryptic error `ERROR_FILE_NOT_FOUND`. Also, move `dstname` assignment below the / check where we have guaranteed that dst is not `"/"`: ```suggestion:-0+0 if (!strcmp( src, "/" ) || !strcmp( dst, "/" )) { set_error( STATUS_ACCESS_DENIED ); return; } dstname = strrchr( dst, '/' ) + 1; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_97916
On Sat Mar 15 12:38:32 2025 +0000, Jinoh Kang wrote:
If multiple wineservers (and thus wineprefixes) were to attempt to rename the same hardlink, they may end up allocating the *same*[^1] tempname and race against each other. This fixes the race without increasing the number of syscalls (untested): ```suggestion:-1+0 if (!is_dir) { /* to avoid TOCTTOU, probe existence of tmpname and link at the same time */ if (linkat( dirfd, srcname, dirfd, tmpname )) continue; if (unlinkat( dirfd, srcname )) { file_set_error(); unlinkat( dirfd, tmpname ); goto ret; } break; } /* directories can't have hardlinks, so just rename it to tmpname */ else if (fstatat( dirfd, tmpname, &st, 0 ) && /* tmpname doesn't exist */ !renameat( dirfd, srcname, dirfd, tmpname )) break; ``` (And remove the renameat srcname,tmpname below) [^1]: Each wineserver starts with `tmp_value = 0`. Each wineserver has the same `current_time` at the same time if clock sampling aligns. Good point. I've added it along with checking for error codes properly (we only want to keep going if it actually exists; if it fails for other reasons, there's no point retrying since it will likely keep failing).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_98020
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)