-- v6: server: When renaming, only fstat the source once. server: Handle hardlinks and casefolding when renaming the same file. 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 | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index b651fc1a1ba..2e5dd78a13c 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); @@ -2895,7 +2893,7 @@ static void test_FindFirstFileA(void) if (handle == INVALID_HANDLE_VALUE) ok( err == ERROR_PATH_NOT_FOUND, "Bad Error number %d\n", err ); else - CloseHandle( handle ); + FindClose( handle );
/* try FindFirstFileA on "c:\foo\nul\bar" */ SetLastError( 0xdeadbeaf );
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 2e5dd78a13c..ec58e5e685a 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
Based on code and suggestions by Jinoh Kang.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/kernel32/tests/file.c | 2 - server/fd.c | 111 ++++++++++++++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 3 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index ec58e5e685a..c15f4ac97ca 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 ce32e7f8397..a6e0fff9290 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2531,6 +2531,109 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags ) ((fd->options & FILE_DELETE_ON_CLOSE) ? FILE_DISPOSITION_DELETE : 0); }
+/* rename the same file, dealing with casefolding and possibly different hardlinks to it */ +static void rename_same_file( const char *src, const char *dst, int is_dir ) +{ + static const char tmpname_fmt[] = ".wine-rename-tmp-%08x"; + static unsigned tmp_value; + + char *dirname, tmpname[sizeof(tmpname_fmt) + 4 /* remaining of %08x */]; + const char *srcname, *dstname = strrchr( dst, '/' ) + 1; + struct stat st, st2; + int dirfd, res = 0; + unsigned i; + + /* first, if not a dir, check the directories they reside in */ + if (!is_dir) + { + if (!(dirname = memdup( dst, dstname - dst ))) + return; + dirname[dstname - dst - 1] = '\0'; + if ((res = stat( dirname, &st ))) + file_set_error(); + free( dirname ); + if (res) + return; + } + + srcname = strrchr( src, '/' ) + 1; + if (!(dirname = memdup( src, srcname - src ))) + return; + dirname[srcname - src - 1] = '\0'; + if ((dirfd = open( dirname, O_RDONLY | O_NONBLOCK )) == -1) + file_set_error(); + free( dirname ); + if (dirfd == -1) + return; + + if (!is_dir) + { + if (fstat( dirfd, &st2 )) + { + file_set_error(); + goto ret; + } + + /* if different directories, it must be a hardlink, so simply remove the source */ + if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino) + { + if (unlinkat( dirfd, srcname, 0 )) + file_set_error(); + goto ret; + } + } + + /* same dentry means a no-op */ + if (!strcmp( srcname, dstname )) goto ret; + + /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry, + * even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead, + * we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the + * destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and + * finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */ + tmp_value += (current_time >> 16) + current_time; + for (i = 0; i < 0x8000; i++, tmp_value += 7777) + { + sprintf( tmpname, tmpname_fmt, tmp_value ); + if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */ + break; + } + if (i < 0x8000) + { + if (renameat( dirfd, srcname, dirfd, tmpname )) + { + file_set_error(); + goto ret; + } + + if (!is_dir) + { + if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST) + { + file_set_error(); + /* Revert the temporary rename */ + renameat( dirfd, tmpname, dirfd, srcname ); + goto ret; + } + + if (unlinkat( dirfd, tmpname, 0 )) + file_set_error(); + } + else + { + /* directories can't have hardlinks, so just rename it to the destination */ + if (renameat( dirfd, tmpname, dirfd, dstname )) + { + file_set_error(); + goto ret; + } + } + } + +ret: + close( dirfd ); +} + /* 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 ) @@ -2556,6 +2659,11 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da set_error( STATUS_OBJECT_PATH_SYNTAX_BAD ); return; } + + /* strip trailing slashes */ + while (nameptr[len - 1] == '/') + len--; + if (!(name = mem_alloc( len + 1 ))) return; memcpy( name, nameptr, len ); name[len] = 0; @@ -2583,7 +2691,8 @@ 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) rename_same_file( fd->unix_name, name, S_ISDIR( st.st_mode ) ); + 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 a6e0fff9290..2cc5cd2cc54 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; +} +
/****************************************************************/ @@ -2638,11 +2656,14 @@ ret: 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 ); @@ -2681,7 +2702,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; @@ -2689,7 +2710,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) rename_same_file( fd->unix_name, name, S_ISDIR( st.st_mode ) ); else if (!replace) set_error( STATUS_OBJECT_NAME_COLLISION ); @@ -2732,7 +2753,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 )) { @@ -2756,14 +2777,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 );
Jinoh Kang (@iamahuman) commented about server/fd.c:
goto ret;}- }
- /* same dentry means a no-op */
- if (!strcmp( srcname, dstname )) goto ret;
- /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry,
* even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead,* we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the* destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and* finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */- tmp_value += (current_time >> 16) + current_time;
- for (i = 0; i < 0x8000; i++, tmp_value += 7777)
- {
sprintf( tmpname, tmpname_fmt, tmp_value );
You shouldn't use sprintf on macOS:
``` ../server/fd.c:2615:9: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations] sprintf( tmpname, tmpname_fmt, tmp_value ); ^ /Applications/Xcode15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/stdio.h:180:1: note: 'sprintf' has been explicitly marked deprecated here __deprecated_msg("This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.") ^ /Applications/Xcode15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/cdefs.h:218:48: note: expanded from macro '__deprecated_msg' #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg))) ^ 1 warning generated. ```
(That one `sprintf` call in server/ptrace.c isn't compiled in macOS. All other uses of `sprintf` have been removed.)
Jinoh Kang (@iamahuman) commented about server/fd.c:
set_error( STATUS_OBJECT_PATH_SYNTAX_BAD ); return; }
- /* strip trailing slashes */
- while (nameptr[len - 1] == '/')
To avoid `MoveFile("Z:\", "Z:\")` from crashing:
```suggestion:-0+0 while (len > 1 && nameptr[len - 1] == '/') ```
Jinoh Kang (@iamahuman) commented about server/fd.c:
- /* same dentry means a no-op */
- if (!strcmp( srcname, dstname )) goto ret;
- /* This is more complicated now, because in case of a casefold (+F) directory, the destination may very well be the same dentry,
* even if the name doesn't match (if it differs just by case), in which case unlinking it is wrong and can be dangerous. Instead,* we first rename the source to a temporary filename in the same directory. If this is a casefold dir, this will also remove the* destination, otherwise the destination still exists. We then create a hardlink from the destination to our temporary name, and* finally, unlink the temporary. This still works if the directory is case sensitive, so it's not a problem in either case. */- tmp_value += (current_time >> 16) + current_time;
- for (i = 0; i < 0x8000; i++, tmp_value += 7777)
- {
sprintf( tmpname, tmpname_fmt, tmp_value );if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */break;- }
- if (i < 0x8000)
Better flip the condition for readability. Also needs to set error.
```suggestion:-0+0 if (i >= 0x8000) { set_error( STATUS_UNSUCCESSFUL ); goto ret; } ```
...then dedent the following code.
Jinoh Kang (@iamahuman) commented about server/fd.c:
- {
sprintf( tmpname, tmpname_fmt, tmp_value );if (fstatat( dirfd, tmpname, &st, 0 )) /* tmpname doesn't exist */break;- }
- if (i < 0x8000)
- {
if (renameat( dirfd, srcname, dirfd, tmpname )){file_set_error();goto ret;}if (!is_dir){if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST)
To avoid bikesheds, maybe add a comment before this line:
```suggestion:-0+0 /* we can't just do rename(2) here; renaming a file to its hardlink is no-op */ if (linkat( dirfd, tmpname, dirfd, dstname, 0 ) && errno != EEXIST) ```
Jinoh Kang (@iamahuman) commented about server/fd.c:
- {
if (!(dirname = memdup( dst, dstname - dst )))return;dirname[dstname - dst - 1] = '\0';if ((res = stat( dirname, &st )))file_set_error();free( dirname );if (res)return;- }
- srcname = strrchr( src, '/' ) + 1;
- if (!(dirname = memdup( src, srcname - src )))
return;- dirname[srcname - src - 1] = '\0';
- if ((dirfd = open( dirname, O_RDONLY | O_NONBLOCK )) == -1)
Better open the destination file's directory instead of the source file's directory.
1. We may only have search access to src directory but not read access (--x). 2. `O_PATH` is only available on Linux-like OSes. `O_SEARCH` is also not cross platform. Notably, macOS lacks `O_PATH`.
To achieve this, we can switch the order: do stat() on src dirname, and open/fstat on dest dirname.
If different directories, it must be a hardlink, so we simply remove the source and bail out. If the same, switching order did nothing, and we can proceed as normal.
Sorry it took this long.
On Sat Nov 30 03:20:12 2024 +0000, Jinoh Kang wrote:
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)
Any progress on this front?
On Sat Mar 8 14:00:06 2025 +0000, Jinoh Kang wrote:
Any progress on this front?
Sorry completely forgot about it.
I'm not sure how to do it here. I can use the source's fd->inode and device but:
1. How do I check if it's a directory without `stat`ing it? (needed for the first fstat() but also below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)
2. `unlink_closed_fd` appears to think it can mismatch. I mean it makes sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here?
On Sat Mar 8 13:58:45 2025 +0000, Jinoh Kang wrote:
Better open the destination file's directory instead of the source file's directory.
- We may only have search access to src directory but not read access (--x).
- `O_PATH` is only available on Linux-like OSes. `O_SEARCH` is also not
cross platform. Notably, macOS lacks `O_PATH`. To achieve this, we can switch the order: do stat() on src dirname, and open/fstat on dest dirname. If different directories, it must be a hardlink, so we simply remove the source and bail out. If the same, switching order did nothing, and we can proceed as normal.
Yeah, I hadn't realized that possibility, although probably rare in practice. I probably used this order so I can use `unlinkat` when removing the source, but it doesn't really matter, I'll swap.
On Mon Mar 10 20:58:40 2025 +0000, Gabriel Ivăncescu wrote:
Sorry completely forgot about it.
I'm not sure how to do it here. I can use the source's fd->inode and device but:
1. How do I check if it's a directory without `stat`ing it? (needed for the first fstat() but also below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)
2. `unlink_closed_fd` appears to think it can mismatch. I mean it makes sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here?
On Mon Mar 10 20:59:30 2025 +0000, Gabriel Ivăncescu wrote:
Yeah, I hadn't realized that possibility, although probably rare in practice. I probably used this order so I can use `unlinkat` when removing the source, but it doesn't really matter, I'll swap.
On second thought, the current order *is* fine. You should keep opening the source directory since that's what's being modified (file unlinked). Sorry about that.
below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)
We can probably eliminate the check and just handle the EISDIR/EEXISTS error from link()/rename() themselves. Would make sense for your goal too, which is perf optimization.
The last stat (for the executable bit) is probably not called very frequently anyway, so I think it's fine to skip the cache there too.
`unlink_closed_fd` appears to think it can mismatch. I mean it makes sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here?
`unlink_closed_fd` is calling `stat( fd->unix_name, ... )`, not `fstat( fd->unix_fd, ... )`. Note there is a difference.
Unix names can be basically moved around and renamed, but fd's dev/ino will stay the same.
On Mon Mar 10 23:32:18 2025 +0000, Jinoh Kang wrote:
below at the `if (create_link || S_ISDIR( st2.st_mode ))` line)
We can probably eliminate the check and just handle the EISDIR/EEXISTS error from link()/rename() themselves. Would make sense for your goal too, which is perf optimization. The last stat (for the executable bit) is probably not called very frequently anyway, so I think it's fine to skip the cache there too.
`unlink_closed_fd` appears to think it can mismatch. I mean it makes
sense (since in Linux we can remove open fds), didn't know we even guard against it though. Should it be a concern here? `unlink_closed_fd` is calling `stat( fd->unix_name, ... )`, not `fstat( fd->unix_fd, ... )`. Note there is a difference. Unix names can be basically moved around and renamed, but fd's dev/ino will stay the same.
Yeah, my issue was inaccurate. The ISDIR check only happens if create_link is false, but the first fstat happens only when it's true, so they can't both happen, which is fine.
I changed these to better fit your suggestions and split into 2 patches. But I have no problem dropping them from this MR, if needed. The important part is to fix the hardlink issue anyway.
-
Gabriel Ivăncescu -
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jinoh Kang (＠iamahuman)