-- v8: 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
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..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()); }
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 | 20 ++++++- server/fd.c | 109 ++++++++++++++++++++++++++++++++++++- 2 files changed, 126 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 4573326a6b5..5d5119d1405 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2182,9 +2182,25 @@ 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); + lstrcpyA(strrchr(dest, '\') + 1, "dir\"); + ret = MoveFileA(source, dest); + ok(ret, "MoveFileA: error %ld\n", GetLastError()); + dest[strrchr(dest, '\') - 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..5fc994cf2dd 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2535,6 +2535,107 @@ 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, if not a dir, check the directories they reside in */ + if (!is_dir) + { + 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 (!is_dir) + { + 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 ) @@ -2560,6 +2661,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; @@ -2587,7 +2693,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 Thu Mar 13 18:05:50 2025 +0000, Jinoh Kang wrote:
Maybe test that `MoveFileA("xyz", "abc\");` works even if xyz is a regular file? I don't mind personally, but just watching out for bikesheds :-)
No worries, it's a good test because now nobody will question why it's there… :P
On Thu Mar 13 22:09:05 2025 +0000, Gabriel Ivăncescu wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/6855/diffs?diff_id=163785&start_sha=0ef2f1d163a859cb9e7697addea01c01e441c3d1#21721cceaf2ea82f93539de2c6e01cc016bbac65_2760_2761)
Yeah, you're right, but I decided to remove those from this MR, hopefully @julliard will look at it more now since it only does 1 thing. I will send those in a separate MR after this. I will incorporate your suggestion into them though, thanks.
Jinoh Kang (@iamahuman) commented about dlls/kernel32/tests/file.c:
- if (hfind != INVALID_HANDLE_VALUE)
- {
todo_wineok(!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);
- ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError());
- /* 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());
- dest[strrchr(dest, '\') - dest] = '\0';
```suggestion:-0+0 *strrchr(dest, '\') = '\0'; ```
Jinoh Kang (@iamahuman) commented about server/fd.c:
set_error( STATUS_OBJECT_PATH_SYNTAX_BAD ); return; }
- /* strip trailing slashes */
- while (len > 1 && nameptr[len - 1] == '/')
Since this fixes a bug in Wine (as it turns out), I suggest splitting this out into its own commit, along with the test. Feel free to also split out the test from the fix.
Jinoh Kang (@iamahuman) commented about server/fd.c:
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 (!is_dir)
Even if `is_dir == 1`, we still need to check the parent directories since, even if directories cannot be hardlinked, they can still be bind-mounted.
For example, if `/var/lib/foo/dir1` is bind-mounted to `/home/user/dir1`, renaming the former to the latter would succeed without doing anything even if `/var/lib/foo` and `/home/user` are completely different directories.
Such renaming should fail, but we're skipping the stat check entirely so the code will simply assume the same dentry (when it's not) and let it pass.
On Fri Mar 14 12:45:04 2025 +0000, Jinoh Kang wrote:
Even if `is_dir == 1`, we still need to check the parent directories since, even if directories cannot be hardlinked, they can still be bind-mounted. For example, if `/var/lib/foo/dir1` is bind-mounted to `/home/user/dir1`, renaming the former to the latter would succeed without doing anything even if `/var/lib/foo` and `/home/user` are completely different directories. Such renaming should fail, but we're skipping the stat check entirely so the code will simply assume the same dentry (when it's not) and let it pass.
On second thought, actually such rename would still be no-op even if we got to the the codepath below.
On Fri Mar 14 13:09:19 2025 +0000, Jinoh Kang wrote:
On second thought, actually such rename would still be no-op even if we got to the the codepath below.
Sorry, I was mistaken, it wouldn't be a no-op.
To be honest I'm not sure how frequent such scenario would be in practice. I also wonder what the semantics for bind mounts should be in practice--perhaps like directory junctions?
In any case it is not immediately clear `if (!is_dir)` check is correct, or serves as a performance advantage. Making the directory comparison unconditional should simplify the logic too.
On Fri Mar 14 13:15:08 2025 +0000, Jinoh Kang wrote:
Sorry, I was mistaken, it wouldn't be a no-op. To be honest I'm not sure how frequent such scenario would be in practice. I also wonder what the semantics for bind mounts should be in practice--perhaps like directory junctions? In any case it is not immediately clear `if (!is_dir)` check is correct, or serves as a performance advantage. Making the directory comparison unconditional should simplify the logic too.
Thanks for spotting it. You're right, I actually wanted to have less impact on directories but it's better this way for sure.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)