[PATCH v2 0/3] MR6855: server: Fix renaming file to a hardlink of itself.
-- v2: 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. 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 c57edebb316..f5c8bd8125a 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2164,10 +2164,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()); + + hfile = FindFirstFileA(dest, &find_data); + ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfile != 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(hfile); + 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 f5c8bd8125a..7d1f2ec0220 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2186,9 +2186,7 @@ static void test_MoveFileA(void) } FindClose(hfile); 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> This avoids unnecessary fstat() calls on the source fd, and also fixes a bug if fstat ever fails where checking if source is a directory, although that's unlikely. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- server/fd.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/server/fd.c b/server/fd.c index a1df0531786..272f7299c3a 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2575,8 +2575,9 @@ 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 stat st, src_st; + int src_st_filled = 0; struct inode *inode; - struct stat st, st2; char *name; const unsigned int replace = flags & FILE_RENAME_REPLACE_IF_EXISTS; @@ -2613,15 +2614,21 @@ 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 && !fstat( fd->unix_fd, &src_st )) { - set_error( STATUS_FILE_IS_A_DIRECTORY ); - goto failed; + src_st_filled = 1; + if (S_ISDIR( src_st.st_mode )) + { + set_error( STATUS_FILE_IS_A_DIRECTORY ); + goto failed; + } } if (!stat( name, &st )) { - if (!fstat( fd->unix_fd, &st2 ) && st.st_ino == st2.st_ino && st.st_dev == st2.st_dev) + if (!src_st_filled && !fstat( fd->unix_fd, &src_st )) + src_st_filled = 1; + if (src_st_filled && st.st_ino == src_st.st_ino && st.st_dev == src_st.st_dev) { if (!create_link) { @@ -2669,7 +2676,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 || (src_st_filled && S_ISDIR( src_st.st_mode ))) { if (unlink( name )) { @@ -2693,14 +2700,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 ) && (src_st_filled || !fstat( fd->unix_fd, &src_st ))) { if (is_file_executable( name )) /* set executable bit where read bit is set */ - st.st_mode |= (st.st_mode & 0444) >> 2; + src_st.st_mode |= (src_st.st_mode & 0444) >> 2; else - st.st_mode &= ~0111; - fchmod( fd->unix_fd, st.st_mode ); + src_st.st_mode &= ~0111; + fchmod( fd->unix_fd, src_st.st_mode ); } free( fd->nt_name ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6855
On Fri Nov 22 21:05:23 2024 +0000, Gabriel Ivăncescu wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/6855/diffs?diff_id=144923&start_sha=59ac8c4c781bbe90963492308102a83e4346554e#21721cceaf2ea82f93539de2c6e01cc016bbac65_2588_2635) @iamahuman Thanks for the review, I believe I've done it right now, let me know if you have further comments/corner cases.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88600
Jinoh Kang (@iamahuman) commented about server/fd.c:
}
/* 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 && !fstat( fd->unix_fd, &src_st )) + { + src_st_filled = 1;
(Interim comment) How about we just unconditionally call `fstat` here? `src_st_filled` looks error-prone anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88711
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 )) + file_set_error(); + } + else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); free( name ); return; }
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88729
Jinoh Kang (@iamahuman) commented about dlls/kernel32/tests/file.c:
+ 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()); + + hfile = FindFirstFileA(dest, &find_data); + ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); + if (hfile != 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(hfile); (nit) I think `hFind` is better than `hfile` (which implies a file when it's not). Consistent with the above code, let's go with:
```suggestion:-8+0 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); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88728
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88730
On Sat Nov 23 08:35:12 2024 +0000, Jinoh Kang wrote:
(Interim comment) How about we just unconditionally call `fstat` here? `src_st_filled` looks error-prone anyway. I don't think that's an improvement if we consider this patch. Right now it's called only in "rarer-than-typical" cases, making it unconditionally might be worse because, even though it's called only once, it will be called on every rename (even typical cases).
How about a helper that functions the same way but caches the result so it only fstats once? That would be almost same as now in complexity while avoiding extra calls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88836
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)