From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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()); }
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 | 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; }
From: Gabriel Ivăncescu gabrielopcode@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@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 );
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_wineok(!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.
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 */ ```
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_wineok(!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.
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.
On Fri Nov 22 16:18:58 2024 +0000, Jinoh Kang wrote:
The error check is missing. Copying from above:
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?
On Fri Nov 22 16:19:13 2024 +0000, Jinoh Kang wrote:
Instead of resolving the full realpath, we can check if:
- 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.
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.
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, ...);`.
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.
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.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)