-- v4: server: When renaming, only fstat the source once. server: Remove source when renaming to a hardlink of itself. 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 c57edebb316..50c3595a08e 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 );
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 50c3595a08e..1ef43b269ac 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()); }
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/kernel32/tests/file.c | 2 -- server/fd.c | 58 +++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 1ef43b269ac..b1f6e1c98d4 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..aae39d7a721 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2531,6 +2531,56 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags ) ((fd->options & FILE_DELETE_ON_CLOSE) ? FILE_DISPOSITION_DELETE : 0); }
+/* check if it's same file by path, and not a hardlink (inodes are assumed to be the same already) */ +static int is_same_file_by_path( char *name1, char *name2 ) +{ + /* we check if (1) the directories they're in are the same and (2) they have the same name */ + size_t pathlen1, pathlen2; + struct stat st, st2; + int stat_ret; + char *p; + + /* get path len up to last component, ignoring trailing slashes */ + for (pathlen1 = 0, p = name1; *p;) + if (*p++ == '/' && *p) + pathlen1 = p - name1; + if (pathlen1) { + name1[pathlen1 - 1] = '\0'; + stat_ret = stat( name1, &st ); + name1[pathlen1 - 1] = '/'; + if (stat_ret || !S_ISDIR( st.st_mode )) + return 0; + } + + for (pathlen2 = 0, p = name2; *p;) + if (*p++ == '/' && *p) + pathlen2 = p - name2; + if (pathlen2) { + name2[pathlen2 - 1] = '\0'; + stat_ret = stat( name2, &st2 ); + name2[pathlen2 - 1] = '/'; + if (stat_ret || !S_ISDIR( st2.st_mode )) + return 0; + } + + if (!pathlen1 || !pathlen2) + { + if (pathlen1 != pathlen2) + return 0; + } + else + { + if (st.st_ino != st2.st_ino || st.st_dev != st2.st_dev) + return 0; + name1 += pathlen1; + name2 += pathlen2; + } + + /* compare ignoring trailing slashes */ + while (*name1 == *name2 && *name1) name1++, name2++; + return *name1 == *name2 || (!*name1 && *name2 == '/') || (*name1 == '/' && !*name2); +} + /* 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 ) @@ -2583,7 +2633,13 @@ 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) + { + /* if it's not the "same" file (by path), it means it's a hardlink, so we need to remove the source */ + if (!is_same_file_by_path( fd->unix_name, name ) && unlink( fd->unix_name )) + file_set_error(); + } + else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); free( name ); return; }
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- server/fd.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/server/fd.c b/server/fd.c index aae39d7a721..68c45cc3b0d 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; +} +
/****************************************************************/ @@ -2585,11 +2603,14 @@ static int is_same_file_by_path( char *name1, char *name2 ) 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 ); @@ -2623,7 +2644,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; @@ -2631,7 +2652,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) { @@ -2679,7 +2700,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 )) { @@ -2703,14 +2724,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 );
Jinoh Kang (@iamahuman) commented about server/fd.c:
{ 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){/* if it's not the "same" file (by path), it means it's a hardlink, so we need to remove the source */if (!is_same_file_by_path( fd->unix_name, name ) && unlink( fd->unix_name ))
This probably suffers from the same casefold problem that !246 has run into. On a casefold filesystem, the `fd->unix_name` is the realpath, but `name` uses whatever casing is provided by the application. This is treated as a "hardlink," resulting in erroneous removal.
(untested)
On Sat Feb 1 14:12:16 2025 +0000, Jinoh Kang wrote:
This probably suffers from the same casefold problem that !246 has run into. On a casefold filesystem, the `fd->unix_name` is the realpath, but `name` uses whatever casing is provided by the application. This is treated as a "hardlink," resulting in erroneous removal. (untested)
I'm specifically mentioning casefold because Steam OS uses the ext4 casefold feature, and I'm assuming you're interested in keeping Steam OS support. If not, I guess we can explore how to handle only case-sensitive filesystems w/o causing any significant regression for casefold ext4.
On Sat Feb 1 14:14:44 2025 +0000, Jinoh Kang wrote:
I'm specifically mentioning casefold because Steam OS uses the ext4 casefold feature, and I'm assuming you're interested in keeping Steam OS support. If not, I guess we can explore how to handle only case-sensitive filesystems w/o causing any significant regression for casefold ext4.
Yeah, I want it to be a proper fix, thanks for pointing these corner cases.
I suppose a theoretical fix here would be to do a case insensitive comparison. But I'm not sure about the details. Does wineserver's tolower match the kernel's (whatever it uses for case folding)?
`rename` doesn't even work properly in this case, because it's a no-op on case-folding (it can't change case like wine), unlike Windows.
I'm open to better ideas.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)