-- v11: 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
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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 );
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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..19ef06e254f 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()); }
From: Gabriel Ivăncescu gabrielopcode@gmail.com
They don't affect the operation even for regular files.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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 19ef06e254f..7e89c462f66 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;
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Based on code and suggestions by Jinoh Kang.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/kernel32/tests/file.c | 2 - server/fd.c | 153 ++++++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 7e89c462f66..3a910114711 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..2994859b44e 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2535,6 +2535,156 @@ 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; + struct stat st, st2; + int dirfd, res = 0; + 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_ino != st2.st_ino || st.st_dev != st2.st_dev) + { + 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 (!is_dir) + { + /* to avoid TOCTTOU, probe existence of tmpname and link at the same time */ + if (linkat( dirfd, srcname, dirfd, tmpname, 0 )) + { + if (errno == EEXIST) continue; + if (errno != EPERM && errno != EOPNOTSUPP) + { + file_set_error(); + goto ret; + } + + /* FIXME: filesystem doesn't support hardlinks, so this can race with other wineservers */ + if (renameat( dirfd, srcname, dirfd, tmpname )) + { + file_set_error(); + goto ret; + } + + break; + } + + if (unlinkat( dirfd, srcname, 0 )) + { + file_set_error(); + unlinkat( dirfd, tmpname, 0 ); + goto ret; + } + } + else + { + /* create an empty directory to avoid races */ + 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, AT_REMOVEDIR ); + goto ret; + } + } + + break; + } + if (i >= 0x8000) + { + set_error( STATUS_UNSUCCESSFUL ); + 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) + { + if (errno == EPERM || errno == EOPNOTSUPP) + { + /* FIXME: filesystem doesn't support hardlinks, entire operation might be a no-op on casefold dirs */ + if (renameat( dirfd, tmpname, dirfd, dstname )) + file_set_error(); + goto ret; + } + + 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 +2742,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; }
On Mon Mar 17 12:08:08 2025 +0000, Jinoh Kang wrote:
I notice you dropped the fstatat(). It was there to reduce chance for race and/or conflict with accidental leftover tmpnames. If you don't like the fstatat(), we can use `mkdirat( dirfd, tmpname, 0777 )` to atomically probe for existence and create an empty directory that will be replaced by `renameat()`. (Yes, rename(2) replaces empty directories.)
I mean yeah, I didn't really like it, if we're going to do something against TOCTTOU, that fstatat seemed like needless complication since it doesn't *solve* it anyway. But your mkdir idea is great, I didn't even know it works on empty dirs to replace them.
The new version should address all the issues, but if I understand correctly, on filesystems not supporting hardlinks (like FAT32), casefold rename to same file won't work correctly, right?
I mean, it's not that I messed something up, or is it?
(the latest version fails to rename casefold files to same name but different case in FAT32, even *if* in this case it's passed the correct case, so it's not affected by my other MR, but works fine for ext4)
On Mon Mar 17 21:26:00 2025 +0000, Gabriel Ivăncescu wrote:
I mean yeah, I didn't really like it, if we're going to do something against TOCTTOU, that fstatat seemed like needless complication since it doesn't *solve* it anyway. But your mkdir idea is great, I didn't even know it works on empty dirs to replace them. The new version should address all the issues, but if I understand correctly, on filesystems not supporting hardlinks (like FAT32), casefold rename to same file won't work correctly, right? I mean, it's not that I messed something up, or is it? (the latest version fails to rename casefold files to same name but different case in FAT32, even *if* in this case it's passed the correct case, so it's not affected by my other MR, but works fine for ext4)
I'm not sure what you meant, but your MR seems to work exactly what I'd expect on both vfat and ext4 (+F and -F).
Note that vfat and ext4-casefold have the same semantics when it comes to rename(2) into a different case i.e., rename("foo", "FOO") is a no-op.
On Mon Mar 17 23:15:44 2025 +0000, Jinoh Kang wrote:
I'm not sure what you meant, but your MR seems to work exactly what I'd expect on both vfat and ext4 (+F and -F). Note that vfat and ext4-casefold have the same semantics when it comes to rename(2) into a different case i.e., rename("foo", "FOO") is a no-op.
For reference, this is the expected result for ext4 casefold:
``` file.c:2083: Test succeeded inside todo block: MoveFile failed to change casing on same file: got Remove me file.c:2107: Test failed: MoveFile failed to change casing on existing target file: got Remove me file.c:2128: Test succeeded inside todo block: MoveFile failed to change casing on same directory: got Remove Me ```
For vfat (which does not support hardlinks):
``` file.c:2083: Test succeeded inside todo block: MoveFile failed to change casing on same file: got Remove me file.c:2107: Test failed: MoveFile failed to change casing on existing target file: got Remove me file.c:2128: Test succeeded inside todo block: MoveFile failed to change casing on same directory: got Remove Me file.c:2171: Test failed: CreateHardLinkA: error 5 file.c:2181: Test succeeded inside todo block: MoveFile failed to change casing on hardlink of itself: got hardLink ```
Jinoh Kang (@iamahuman) commented about server/fd.c:
- for (i = 0; i < 0x8000; i++, tmp_value += 7777)
- {
snprintf( tmpname, sizeof(tmpname), tmpname_fmt, tmp_value );if (!is_dir){/* to avoid TOCTTOU, probe existence of tmpname and link at the same time */if (linkat( dirfd, srcname, dirfd, tmpname, 0 )){if (errno == EEXIST) continue;if (errno != EPERM && errno != EOPNOTSUPP){file_set_error();goto ret;}/* FIXME: filesystem doesn't support hardlinks, so this can race with other wineservers */
FYI, this fixme can be resolved by creating a temporary file on tmpname and then renaming to it (just like the is_dir case). I didn't originally mention it because it involves one extra sysca (linkat+unlinkat vs. openat+close+renameat).
I'll leave this up to you, because I'm not sure how common this scenario would be in practice. But I think it could make the logic simpler since now we can do one renameat(2) call for both file and dir, and the EPERM check can go away—at the expense of one extra syscall.
Jinoh Kang (@iamahuman) commented about server/fd.c:
break;- }
- if (i >= 0x8000)
- {
set_error( STATUS_UNSUCCESSFUL );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){if (errno == EPERM || errno == EOPNOTSUPP){/* FIXME: filesystem doesn't support hardlinks, entire operation might be a no-op on casefold dirs */
If a filesystem does not support hardlinks, different dentry means different file so it shouldn't be a silent no-op (for bind mounts it would return EBUSY, which is not silent)
Jinoh Kang (@iamahuman) commented about server/fd.c:
- 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){if (errno == EPERM || errno == EOPNOTSUPP){/* FIXME: filesystem doesn't support hardlinks, entire operation might be a no-op on casefold dirs */if (renameat( dirfd, tmpname, dirfd, dstname ))file_set_error();goto ret;}file_set_error();/* revert the temporary rename */renameat( dirfd, tmpname, dirfd, srcname );
Let's make this a common logic for all cases (hardlink, no hardlink, & dir).
Something like this (untested):
```c int undo_tmp_rename = 0;
(...)
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) { if (errno != EPERM && errno != EOPNOTSUPP) { file_set_error(); goto ret; }
/* Filesystem doesn't support hardlinks, fall back to renaming */ } else { undo_tmp_rename = 0;
if (unlinkat( dirfd, tmpname, 0 )) file_set_error();
goto ret; /* We're done */ } }
/* Renaming a directory, or filesystem doesn't support hardlinks */ 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 ); } ```
On Mon Mar 17 23:43:40 2025 +0000, Jinoh Kang wrote:
If a filesystem does not support hardlinks, different dentry means different file so it shouldn't be a silent no-op (for bind mounts it would return EBUSY, which is not silent)
Yeah not sure what I was thinking. I was still thinking it can have hardlinks despite clearly not. Sorry.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)