[PATCH v5 0/4] MR6855: server: Fix renaming file to a hardlink of itself.
-- v5: server: When renaming, only fstat the source once. server: Handle hardlinks and casefolding when renaming the same file. 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 b651fc1a1ba..2e5dd78a13c 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -1996,9 +1996,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; @@ -2078,14 +2077,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'; @@ -2102,14 +2101,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()); @@ -2123,14 +2122,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); @@ -2145,12 +2144,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 @@ -2160,8 +2158,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); @@ -2895,7 +2893,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 2e5dd78a13c..ec58e5e685a 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2162,10 +2162,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> 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 | 93 +++++++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index ec58e5e685a..c15f4ac97ca 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2184,9 +2184,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); ret = DeleteFileA(dest); ok(ret, "DeleteFileA: error %ld\n", GetLastError()); diff --git a/server/fd.c b/server/fd.c index ce32e7f8397..aeba50be49c 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2531,6 +2531,91 @@ 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 ) +{ + 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; + struct stat st, st2; + int dirfd, res = 0; + unsigned i; + + /* check the directories they reside in */ + dstname = strrchr( dst, '/' ) + 1; + 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) + { + sprintf( tmpname, tmpname_fmt, tmp_value ); + if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */ + break; + } + if (i < 0x8000) + { + if (renameat( dirfd, srcname, dirfd, tmpname )) + { + file_set_error(); + goto ret; + } + + if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST) + { + file_set_error(); + /* Revert the rename */ + renameat( dirfd, tmpname, dirfd, srcname ); + goto ret; + } + + if (unlinkat( dirfd, tmpname, 0 )) + 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 ) @@ -2556,6 +2641,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 (nameptr[len - 1] == '/') + len--; + if (!(name = mem_alloc( len + 1 ))) return; memcpy( name, nameptr, len ); name[len] = 0; @@ -2583,7 +2673,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 ); + else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); free( name ); return; } -- 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> --- server/fd.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/server/fd.c b/server/fd.c index aeba50be49c..e1ff5144530 100644 --- a/server/fd.c +++ b/server/fd.c @@ -319,6 +319,24 @@ static file_pos_t max_unix_offset = OFF_T_MAX; fprintf( stderr, "%lx", (unsigned long)(val) ); \ } while (0) +struct cached_stat +{ + struct stat st; + int init; +}; + +/* Caches fstat (must use on same fd), set 'init' to 0 before use */ +static inline int cached_fstat(int fd, struct cached_stat *cached_stat) +{ + int ret; + + if (cached_stat->init) + return 0; + if (!(ret = fstat( fd, &cached_stat->st ))) + cached_stat->init = 1; + return ret; +} + /****************************************************************/ @@ -2620,11 +2638,14 @@ ret: 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 ) { + struct cached_stat cached_st; struct inode *inode; - struct stat st, st2; + struct stat st; char *name; const unsigned int replace = flags & FILE_RENAME_REPLACE_IF_EXISTS; + cached_st.init = 0; + if (!fd->inode || !fd->unix_name) { set_error( STATUS_OBJECT_TYPE_MISMATCH ); @@ -2663,7 +2684,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da } /* when creating a hard link, source cannot be a dir */ - if (create_link && !fstat( fd->unix_fd, &st ) && S_ISDIR( st.st_mode )) + if (create_link && !cached_fstat( fd->unix_fd, &cached_st ) && S_ISDIR( cached_st.st.st_mode )) { set_error( STATUS_FILE_IS_A_DIRECTORY ); goto failed; @@ -2671,7 +2692,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da if (!stat( name, &st )) { - if (!fstat( fd->unix_fd, &st2 ) && st.st_ino == st2.st_ino && st.st_dev == st2.st_dev) + if (!cached_fstat( fd->unix_fd, &cached_st ) && st.st_ino == cached_st.st.st_ino && st.st_dev == cached_st.st.st_dev) { if (!create_link) rename_same_file( fd->unix_name, name ); else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); @@ -2714,7 +2735,7 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da /* link() expects that the target doesn't exist */ /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (create_link || S_ISDIR( cached_st.st.st_mode )) { if (unlink( name )) { @@ -2738,14 +2759,14 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da goto failed; } - if (is_file_executable( fd->unix_name ) != is_file_executable( name ) && !fstat( fd->unix_fd, &st )) + if (is_file_executable( fd->unix_name ) != is_file_executable( name ) && !cached_fstat( fd->unix_fd, &cached_st )) { if (is_file_executable( name )) /* set executable bit where read bit is set */ - st.st_mode |= (st.st_mode & 0444) >> 2; + cached_st.st.st_mode |= (cached_st.st.st_mode & 0444) >> 2; else - st.st_mode &= ~0111; - fchmod( fd->unix_fd, st.st_mode ); + cached_st.st.st_mode &= ~0111; + fchmod( fd->unix_fd, cached_st.st.st_mode ); } free( fd->nt_name ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
On Fri Feb 7 00:00:01 2025 +0000, Jinoh Kang wrote:
I think `lookup_unix_name` is supposed to reject trailing slashes with `OBJECT_NAME_INVALID`, because it's just a special case of an empty path component. This is just a suggestion, so it's not necessary to take my code as-is. I still think that it's better to (1) adopt an unambiguous function name, and (2) try not to modify the argument at all but instead allocate a separate buffer for that (modifying and reverting is error-prone in general). For completeness, `fd->unix_name` is returned by realpath(), so not a concern. Correct me if I'm wrong, though. Sorry it took a bit, I only had some time to look through it again today. I made a combination of your method and mine. I basically put everything into the new function instead of just checking the dentry, because we need to act based on what was found anyway.
I also used relative paths (*at functions) to make it simpler and reduce the race conditions, keeping the dir fd open. Obviously it's impossible to be atomic, but I tried to make it have as few syscalls as possible. There's still one problem: hardlinks don't work on directories, so I think for dirs we need something else. I mean the whole "rename temporary then hardlink into dest" thing. For files I tested and it works fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_95689
On Sun Feb 23 19:54:25 2025 +0000, Gabriel Ivăncescu wrote:
Sorry it took a bit, I only had some time to look through it again today. I made a combination of your method and mine. I basically put everything into the new function instead of just checking the dentry, because we need to act based on what was found anyway. I also used relative paths (*at functions) to make it simpler and reduce the race conditions, keeping the dir fd open. Obviously it's impossible to be atomic, but I tried to make it have as few syscalls as possible. There's still one problem: hardlinks don't work on directories, so I think for dirs we need something else. I mean the whole "rename temporary then hardlink into dest" thing. For files I tested and it works fine. Oh yeah I also strip trailing slashes from the destination now, since they're pointless anyway (no need to do it on source).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_95690
On Sun Feb 23 19:55:25 2025 +0000, Gabriel Ivăncescu wrote:
Oh yeah I also strip trailing slashes from the destination now, since they're pointless anyway (no need to do it on source). In case of directories, I think it's safe to do the rename-to-temp-and-rename-to-dest dance.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_95745
On second thought, directories can't have hardlinks, so we needn't check anything other than the basename matching and then do the dance as you said.
On Mon Feb 24 15:28:37 2025 +0000, Jinoh Kang wrote:
In case of directories, I think it's safe to do the rename-to-temp-and-rename-to-dest dance. True, and on second thought, can even skip the directory checks since directories can't be hardlinked anyway. If it goes past the strcmp that checks if same dentry, it's guaranteed to be casefold filesystem anyway, so in all other cases it would exit early (which is good because the rename-to-temporary isn't atomic, so in other cases it shouldn't do anything).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_95924
participants (3)
-
Gabriel Ivăncescu -
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)