[PATCH v3 0/4] MR6855: server: Fix renaming file to a hardlink of itself.
-- 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 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 | 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); -- 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 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()); } -- 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 | 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; } -- 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> --- 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 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88841
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_89287
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)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_89288
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_89300
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*.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_89301
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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_89500
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) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_91786
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
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_91787
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_91906
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)