[PATCH 0/3] MR6855: server: Fix renaming file to a hardlink of itself.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index c57edebb316..6b619f9db32 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2164,10 +2164,33 @@ 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 and 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); + } + CloseHandle(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); + DeleteFileA(dest); + 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 | 10 +++++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 6b619f9db32..c1b30a4a7c6 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2186,9 +2186,7 @@ static void test_MoveFileA(void) } CloseHandle(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); DeleteFileA(dest); ret = RemoveDirectoryA(tempdir); diff --git a/server/fd.c b/server/fd.c index 04688c5eb0d..786f5e0bbf9 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2573,7 +2573,15 @@ 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) + { + char src[PATH_MAX], dst[PATH_MAX]; + + /* if it's not the "same" file (by path), it means it's a hardlink, so remove the source */ + if (!realpath( fd->unix_name, src ) || !realpath( name, dst ) || (strcmp( src, dst ) && unlink( src ))) + 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 786f5e0bbf9..9314df19b7b 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2525,8 +2525,9 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags ) 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; @@ -2563,15 +2564,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) { @@ -2621,7 +2628,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 )) { @@ -2645,14 +2652,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
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); + } + CloseHandle(hfile); I know that the existing code does this, but this wrongly closes an unrelated handle instead of cleaning up hfile because hfile is actually a `FIND_FIRST_INFO *`, not a kernel handle. You need `FindClose()` to free it.
```suggestion:-0+0 FindClose(hfile); ``` Yes, the existing test is broken as I realize now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88538
Jinoh Kang (@iamahuman) commented about dlls/kernel32/tests/file.c:
+ 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); + } + CloseHandle(hfile); + ret = GetFileAttributesA(source); + ok(ret == INVALID_FILE_ATTRIBUTES && GetLastError() == ERROR_FILE_NOT_FOUND, "GetFileAttributesA: error %ld\n", GetLastError()); + DeleteFileA(dest); The error check is missing. Copying from above:
```suggestion:-0+0 ret = DeleteFileA(dest); ok(!ret, "DeleteFileA: error %ld\n", GetLastError()); ``` Deletion failures sometimes happen due to e.g., unexpectedly open handles w/o FILE\_SHARE\_DELETE, and leftover files interfere with successive tests that assume clean slate. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88540
Jinoh Kang (@iamahuman) commented about dlls/kernel32/tests/file.c:
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 and different case */
At first glance, this sounded like "test renaming file to hardlink of itself in same case as well as different case" This sounds clear to others reading the code: ```suggestion:-0+0 /* test renaming file to hardlink of itself with different case */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88539
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) + { + char src[PATH_MAX], dst[PATH_MAX]; + + /* if it's not the "same" file (by path), it means it's a hardlink, so remove the source */ + if (!realpath( fd->unix_name, src ) || !realpath( name, dst ) || (strcmp( src, dst ) && unlink( src )))
Instead of resolving the full realpath, we can check if: 1. their dirnames resolve to different dev/ino, which means they belong to a different directory, or 2. they have different basenames (even if they belong to the same directory). Granted, there might be an edge case regarding symlinks and/or mount points, but I suspect the existing code isn't fully correct in this regard either, so I guess it's fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88541
On Fri Nov 22 16:18:58 2024 +0000, Jinoh Kang wrote:
The error check is missing. Copying from above: ```suggestion:-0+0 ret = DeleteFileA(dest); ok(!ret, "DeleteFileA: error %ld\n", GetLastError()); ``` Deletion failures sometimes happen due to e.g., unexpectedly open handles w/o FILE\_SHARE\_DELETE, and leftover files interfere with successive tests that assume clean slate. For some reason this returns non-zero for me on Windows, even though it deletes the file. So I assumed it's flaky in some way? Does it not for you?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88543
On Fri Nov 22 16:19:13 2024 +0000, Jinoh Kang wrote:
Instead of resolving the full realpath, we can check if: 1. their dirnames resolve to different dev/ino, which means they belong to a different directory, or 2. they have different basenames (even if they belong to the same directory). Granted, there might be an edge case regarding symlinks and/or mount points, but I suspect the existing code isn't fully correct in this regard either, so I guess it's fine. Is that worth the complication in the code? We'll still need two syscalls, and the kernel will still need to traverse the same amount of symlinks to reach the dirnames.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88544
On Fri Nov 22 16:27:05 2024 +0000, Gabriel Ivăncescu wrote:
Is that worth the complication in the code? We'll still need two syscalls, and the kernel will still need to traverse the same amount of symlinks to reach the dirnames. realpath() is not a syscall: https://elixir.bootlin.com/glibc/glibc-2.40.9000/source/stdlib/canonicalize....
Besides, realpath() cannot handle bind mounts (e.g., due to some container configuratio ) transparently. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88548
On Fri Nov 22 16:23:40 2024 +0000, Gabriel Ivăncescu wrote:
For some reason this returns non-zero for me on Windows, even though it deletes the file. So I assumed it's flaky in some way? Does it not for you? Sorry, typo, it's `ok(ret, ...);`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88549
On Fri Nov 22 16:56:58 2024 +0000, Jinoh Kang wrote:
realpath() is not a syscall: https://elixir.bootlin.com/glibc/glibc-2.40.9000/source/stdlib/canonicalize.... Besides, realpath() cannot handle bind mounts (e.g., due to some container configuration) transparently. Oh I see that is definitely a convincing argument for me, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88550
On Fri Nov 22 17:03:11 2024 +0000, Gabriel Ivăncescu wrote:
Oh I see that is definitely a convincing argument for me, thanks. To be clear, I haven't checked if e.g., Steam runtime/client uses such bind mounts in containers. I don't have strong opinions either way, except perhaps perf impact concerns.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_88552
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)