-- v3: 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 | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index c57edebb316..60ae9e98a8c 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);
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 60ae9e98a8c..4cc1f11f394 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 4cc1f11f394..3a0fa9c3f64 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 dc2475b2d28..a1df0531786 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2521,6 +2521,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 ) @@ -2573,7 +2623,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 a1df0531786..5a3138c2897 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; +} +
/****************************************************************/ @@ -2575,11 +2593,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 ); @@ -2613,7 +2634,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; @@ -2621,7 +2642,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) { @@ -2669,7 +2690,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 )) { @@ -2693,14 +2714,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 );
On Sat Nov 23 14:56:27 2024 +0000, Jinoh Kang wrote:
I suspect this (both old and new code) is broken since it doesn't go through the rest of checks below. This is merely a hack to work around rename(2) being a no-op if src and dest are hard-links to the same file, so the special casing should be done at the rename(2) call below, instead of returning early IMHO.
I haven't changed this, mostly because I don't know exactly what you meant. I updated with changes and the cached idea, let me know what you think when you're back.
On Mon Nov 25 19:19:31 2024 +0000, Gabriel Ivăncescu wrote:
I haven't changed this, mostly because I don't know exactly what you meant. I updated with changes and the cached idea, let me know what you think when you're back.
Like, wine (regardless of this patch) is doing `free( name );` and then early return. That doesn't *look* right to me. I feel like we're still supposed to do the following checks (e.g., permissions check, get_inode check, etc) even if src and dest are the same file. But the current tests are too lacking to verify if what I'm claiming is true.
On Mon Nov 25 19:18:56 2024 +0000, Gabriel Ivăncescu wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/6855/diffs?diff_id=145279&start_sha=2c2eff3907bf5060456f2cb29709095cfb7128ce#21721cceaf2ea82f93539de2c6e01cc016bbac65_2619_2637)
Encapsulating init flag looks better, but we can do better still by just reusing dev/ino information from `fd->inode`. This way we can reduce fstat() calls to just two (the latter of which is arguably more rare in practice)
On Sat Nov 30 11:57:23 2024 +0000, Jinoh Kang wrote:
Like, wine (regardless of this patch) is doing `free( name );` and then early return. That doesn't *look* right to me. I feel like we're still supposed to do the following checks (e.g., permissions check, get_inode check, etc) even if src and dest are the same file. But the current tests are too lacking to verify if what I'm claiming is true.
Actually I can fix this in a separate MR. Let's mark this resolved for now.
On Fri Nov 22 21:06:34 2024 +0000, Gabriel Ivăncescu wrote:
@iamahuman Thanks for the review, I believe I've done it right now, let me know if you have further comments/corner cases.
Thanks. By "is_same_file_by_path", do you mean "is_same_dentry"? A file can have multiple paths (due to symlinks) as well as realpaths (due to mountpoints). I think you're talking about dentry, not path. For example, if there are two hardlinks with different names that refer to the same file, they are two different *dentries (directory entries)* referencing the same *inode*.
On Sat Nov 30 12:04:55 2024 +0000, Jinoh Kang wrote:
Thanks. By "is_same_file_by_path", do you mean "is_same_dentry"? A file can have multiple paths (due to symlinks) as well as realpaths (due to mountpoints). I think you're talking about dentry, not path. For example, if there are two hardlinks with different names that refer to the same file, they are two different *dentries (directory entries)* referencing the same *inode*.
Yes that's what I meant. Is there a better way to detect that?
You missed s/CloseHandle/FindClose/ at line 2889.
Although, arguably, global fixes like thisshould be part of a separate patchset for easier review. (My previous review was about *new* usages of FindClose/CloseHandle, not existing onces. I don't think existing incorrect code should block this MR or any others)
On Sat Jan 11 12:18:41 2025 +0000, Jinoh Kang wrote:
I'm probably unavailable this entire week, so take your time. Meanwhile I'll try to come up with my own version with a (hopefully) more correct fix.
I've managed to get back to this, but will be only able to finish around end of code freeze
On Sat Jan 11 12:19:31 2025 +0000, Jinoh Kang wrote:
You missed s/CloseHandle/FindClose/ at line 2889. Although, arguably, global fixes like thisshould be part of a separate patchset for easier review. (My previous review was about *new* usages of FindClose/CloseHandle, not existing onces. I don't think existing incorrect code should block this MR or any others)
Ah oops. Will fix. But I personally think it's too trivial to split in a separate MR, especially since it's for tests only.
BTW it's fine to take your time with your variant, we're in code freeze anyway.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)