[PATCH v13 0/4] MR6855: server: Fix renaming file to a hardlink of itself.
-- v13: 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 38c68297097..126c28ca440 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 126c28ca440..e81fa0a4e20 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()); + *strrchr(dest, 'l') = '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 e81fa0a4e20..378c3eb12a0 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 bb0e90d99d0..4a0f7a1ad02 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2568,6 +2568,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 | 141 ++++++++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 378c3eb12a0..621293f8a69 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 4a0f7a1ad02..80517278929 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2543,6 +2543,144 @@ 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 */]; + int dirfd, undo_tmp_rename = 0, res = 0; + const char *srcname, *dstname; + struct stat st, st2; + unsigned i; + + if (!strcmp( src, "/" ) || !strcmp( dst, "/" )) + { + set_error( STATUS_ACCESS_DENIED ); + return; + } + + /* first, 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) + { + snprintf( tmpname, sizeof(tmpname), tmpname_fmt, tmp_value ); + + /* create an empty file or directory to avoid TOCTTOU */ + if (!is_dir) + { + int fd = openat( dirfd, tmpname, O_CREAT | O_EXCL | O_WRONLY, 0666 ); + if (fd == -1) + { + if (errno == EEXIST) continue; + file_set_error(); + goto ret; + } + close( fd ); + } + else if (mkdirat( dirfd, tmpname, 0777 )) + { + if (errno == EEXIST) continue; + file_set_error(); + goto ret; + } + + if (renameat( dirfd, srcname, dirfd, tmpname )) + { + file_set_error(); + unlinkat( dirfd, tmpname, is_dir ? AT_REMOVEDIR : 0 ); + goto ret; + } + + break; + } + if (i >= 0x8000) + { + set_error( STATUS_UNSUCCESSFUL ); + goto ret; + } + + undo_tmp_rename = 1; + + /* directories can't have hardlinks */ + 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) + { + undo_tmp_rename = 0; + + if (unlinkat( dirfd, tmpname, 0 )) + file_set_error(); + goto ret; + } + + if (errno != EPERM && errno != EOPNOTSUPP) + { + file_set_error(); + goto ret; + } + + /* filesystem doesn't support hardlinks, fall back to renaming (same as directories) */ + } + + if (renameat( dirfd, tmpname, dirfd, dstname )) + file_set_error(); + else + undo_tmp_rename = 0; + +ret: + if (undo_tmp_rename) + { + /* revert the temporary rename */ + renameat( dirfd, tmpname, dirfd, srcname ); + } + 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 ) @@ -2600,7 +2738,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
On Sun Mar 30 17:08:01 2025 +0000, Gabriel Ivăncescu wrote:
changed this line in [version 13 of the diff](/wine/wine/-/merge_requests/6855/diffs?diff_id=167479&start_sha=dfe9be50de60d388e01ef3647f9a759b1c6f0ef5#21721cceaf2ea82f93539de2c6e01cc016bbac65_2584_2592) Done, thanks with all the help and reviewing it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_99352
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
Actually there's something I've missed: do you know any application that relies on renaming hardlink to itself? Otherwise, since this MR stemmed from !246 (which I assume is from Proton), could we proceed with the original MR without the rename-hardlink fix? Sorry for not noticing this earlier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_101543
On Mon Apr 21 11:02:42 2025 +0000, Jinoh Kang wrote:
Actually there's something I've missed: do you know any application that relies on renaming hardlink to itself? Otherwise, since this MR stemmed from !246 (which I assume is from Proton), could we proceed with the original MR without the rename-hardlink fix? Sorry for not noticing this earlier. I think the intention of this MR is that, rather than fixing an app directly, it simplifies the work for !246 when dealing with casefold filesystems.
The next step would be to explore how to simplify this MR so that only the ncessary groundwork for !246 renamins. The extra complexity could only be justified if there was an app with a possibility of renaming hardlink to the link itself. But maybe, fixing !246 the proper way might end up needing this MR, in which case that alone could be a strong argument for this patch. Again, my sincere apologies for backtracking from my original position. As someone who took up the reviewing work, I take responsibility for not thinking through what the Wine project's ultimate goal is when judging neccessity of a contribution. I nonetheless appreciate your effort so far, including the multiple iterations that led to the latest revision of !246. It has been a great time working with you! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_101545
On Mon Apr 21 11:07:25 2025 +0000, Jinoh Kang wrote:
I think the intention of this MR is that, rather than fixing an app directly, it simplifies the work for !246 when dealing with casefold filesystems. The next step would be to explore how to simplify this MR so that only the ncessary groundwork for !246 renamins. The extra complexity could only be justified if there was an app with a possibility of renaming hardlink to the link itself. But maybe, fixing !246 the proper way might end up needing this MR, in which case that alone could be a strong argument for this patch. Again, my sincere apologies for backtracking from my original position. As someone who took up the reviewing work, I take responsibility for not thinking through what the Wine project's ultimate goal is when judging neccessity of a contribution (and thanks @julliard for pointing this out on IRC). I nonetheless appreciate your effort so far, including the multiple iterations that led to the latest revision of !246. It has been a great time working with you! Understandable, and yes even if it's just to simplify !246 first, it's also a step in the right direction in terms of correctness (which IMO is the most important), and while no app "directly" requires this to not "break", it is indeed used by e.g. file managers (it was my original issue with !246 too, and still is, though I carry my own patches for it), and I've seen people interested in it so I'm not the only one that uses file managers in Wine...
So personally I'd rather have it fixed as-is, but if it would help to reduce its scope to get it in (as long as !246 can get in, since I need that one) I wouldn't be too hesitant to change. That said, I'd want to know for sure before I proceed since IMO we put quite some work into it so far to have it correct. @julliard any thoughts please? Or would someone else review it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_101566
On Mon Apr 21 17:49:07 2025 +0000, Gabriel Ivăncescu wrote:
Understandable, and yes even if it's just to simplify !246 first, it's also a step in the right direction in terms of correctness (which IMO is the most important), and while no app "directly" requires this to not "break", it is indeed used by e.g. file managers (it was my original issue with !246 too, and still is, though I carry my own patches for it), and I've seen people interested in it so I'm not the only one that uses file managers in Wine... So personally I'd rather have it fixed as-is, but if it would help to reduce its scope to get it in (as long as !246 can get in, since I need that one) I wouldn't be too hesitant to change. That said, I'd want to know for sure before I proceed since IMO we put quite some work into it so far to have it correct. @julliard any thoughts please? Or would someone else review it? It looks like a lot of complexity for something that it's very unlikely that an app would require...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_101576
On Mon Apr 21 20:49:21 2025 +0000, Alexandre Julliard wrote:
It looks like a lot of complexity for something that it's very unlikely that an app would require... Yeah, but apps might make use of it "indirectly". For example, if you use a file manager in Wine (e.g. Total Commander), you can't rename a file to itself in a different case.
Even with some form of !246 in, you still can't do it on case-insensitive directories (those with casefold +F attr on them). Hardlinks are just a specific case needed for casefold dirs. So it's actually a practical fix for people using file managers. It does impact their usefulness. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_101577
@julliard Would removing the casefold part of the code make this more likely to be reviewed/accepted? (i.e. the part after the "This is more complicated now" comment) It wouldn't have proper behavior on casefolded dirs/filesystems only, and it's the main source of complication. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_123839
participants (3)
-
Alexandre Julliard (＠julliard) -
Gabriel Ivăncescu
-
Jinoh Kang (＠iamahuman)