[PATCH v4 0/4] MR6855: server: Fix renaming file to a hardlink of itself.
-- v4: 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 | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index c57edebb316..50c3595a08e 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 ); -- 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 50c3595a08e..1ef43b269ac 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 1ef43b269ac..b1f6e1c98d4 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..aae39d7a721 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2531,6 +2531,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 ) @@ -2583,7 +2633,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 aae39d7a721..68c45cc3b0d 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; +} + /****************************************************************/ @@ -2585,11 +2603,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 ); @@ -2623,7 +2644,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; @@ -2631,7 +2652,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) { @@ -2679,7 +2700,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 )) { @@ -2703,14 +2724,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
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 ))
This probably suffers from the same casefold problem that !246 has run into. On a casefold filesystem, the `fd->unix_name` is the realpath, but `name` uses whatever casing is provided by the application. This is treated as a "hardlink," resulting in erroneous removal. (untested) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93385
On Sat Feb 1 14:12:16 2025 +0000, Jinoh Kang wrote:
This probably suffers from the same casefold problem that !246 has run into. On a casefold filesystem, the `fd->unix_name` is the realpath, but `name` uses whatever casing is provided by the application. This is treated as a "hardlink," resulting in erroneous removal. (untested) I'm specifically mentioning casefold because Steam OS uses the ext4 casefold feature, and I'm assuming you're interested in keeping Steam OS support. If not, I guess we can explore how to handle only case-sensitive filesystems w/o causing any significant regression for casefold ext4.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93387
On Sat Feb 1 14:14:44 2025 +0000, Jinoh Kang wrote:
I'm specifically mentioning casefold because Steam OS uses the ext4 casefold feature, and I'm assuming you're interested in keeping Steam OS support. If not, I guess we can explore how to handle only case-sensitive filesystems w/o causing any significant regression for casefold ext4. Yeah, I want it to be a proper fix, thanks for pointing these corner cases.
I suppose a theoretical fix here would be to do a case insensitive comparison. But I'm not sure about the details. Does wineserver's tolower match the kernel's (whatever it uses for case folding)? `rename` doesn't even work properly in this case, because it's a no-op on case-folding (it can't change case like wine), unlike Windows. I'm open to better ideas. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93467
I suppose a theoretical fix here would be to do a case insensitive comparison. But I'm not sure about the details. Does wineserver's tolower match the kernel's (whatever it uses for case folding)?
NTFS has a `$UpCase` table (in filesystem data) for UCS-2 codepoints, presumably initialized from some NLS table at filesystem creation (partition format). Wineserver `to_lower` uses a preloaded `casemap` lookup table, in turn loaded from `l_intl.nls`. So I'm guessing they match most of the time, although theoretical deviation is possible from very old NTFS filesystems. Meanwhile, it seems like ext4 uses UTF8\_NFDICF (NFD normalization plus removal of ignorable code points and case folding). Since NTFS uses legacy UTF-16 with surrogates permitted, I think it's safe to assume it has no support for case-folding in Supplementary Multilingual Plane codepoints (U+10000 and above).
`rename` doesn't even work properly in this case, because it's a no-op on case-folding (it can't change case like wine), unlike Windows.
On a pure case-change operation on ext4 +F directory (e.g., `FOObar` -> `fooBAR`), `stat()` will basically report that the destination is a hard link to the source. Instead of unlinking the source immediately, we should: 1. Rename the source to some other temporary name. - In case of a casefold (+F) directory, the destination will disappear as well (since it's just the source). 2. Create a hard link from the destination to the temporary name via `link(2)`. - If this succeeds, then the destination (`fooBAR` in the example above) was removed in step (1), which means it was a casefold (+F) directory and we have successfully changed the case. Proceed to (3). - If this fails with `EEXIST`, the destination still exists. Proceed to (3). - If this fails with any other reason, revert (1) and exit early. 3. Unlink the temporary name. In case of non-casefold filesystems, this is still correct: since (2) does nothing, (1) and (3) together simply unlinks the source without touching the destination. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93892
On Mon Dec 2 22:17:00 2024 +0000, Gabriel Ivăncescu wrote:
Yes that's what I meant. Is there a better way to detect that? There is probably no fundamental improvements in I/O aspect, although I believe the name `is_same_dentry` is much clearer. I like this version better:
```c /* Determine if two paths have the same parent directories and basenames. */ static int is_same_dentry( const char *pathname1, const char *pathname2 ) { const char *lastslash1 = strrchr(pathname1, '/'); const char *lastslash2 = strrchr(pathname2, '/'); char *dirname1 = NULL, dirname2 = NULL; int res; /* Last slash must be preceded by a non-slash character */ assert( lastslash1 && lastslash1 != pathname1 && lastslash1[-1] != '/' ); assert( lastslash2 && lastslash2 != pathname2 && lastslash2[-1] != '/' ); /* different basenames imply different dentries */ if (strcmp( lastslash1 + 1, lastslash2 + 1 ) != 0) return 0; res = -1; if ((dirname1 = memdup( pathname1, lastslash1 + 1 - pathname1 )) && (dirname2 = memdup( pathname2, lastslash2 + 1 - pathname2 ))) { struct stat st1, st2; dirname1[lastslash1 - pathname1] = '\0'; dirname2[lastslash2 - pathname2] = '\0'; if (stat(dirname1, &st1) || stat(dirname2, &st2)) { file_set_error(); } else { res = st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino; } } free(dirname2); free(dirname1); return res; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93894
On Thu Feb 6 16:35:42 2025 +0000, Jinoh Kang wrote: > > I suppose a theoretical fix here would be to do a case insensitive > comparison. But I'm not sure about the details. Does wineserver's > tolower match the kernel's (whatever it uses for case folding)? > NTFS has a `$UpCase` table (in filesystem data) for UCS-2 codepoints, > presumably initialized from some NLS table at filesystem creation > (partition format). Wineserver `to_lower` uses a preloaded `casemap` > lookup table, in turn loaded from `l_intl.nls`. So I'm guessing they > match most of the time, although theoretical deviation is possible from > very old NTFS filesystems. > Meanwhile, it seems like ext4 uses UTF8\_NFDICF (NFD normalization plus > removal of ignorable code points before case folding). Since NTFS uses > legacy UTF-16 with surrogates permitted, I think it's safe to assume it > (NTFS) has no support for case-folding in Supplementary Multilingual > Plane codepoints (U+10000 and above). > > `rename` doesn't even work properly in this case, because it's a no-op > on case-folding (it can't change case like wine), unlike Windows. > On a pure case-change operation on ext4 +F directory (e.g., `FOObar` -> > `fooBAR`), `stat()` will basically report that the destination is a hard > link to the source. Instead of unlinking the source immediately, we should: > 1. Rename the source to some other temporary name. > - In case of a casefold (+F) directory, the destination will > disappear as well (since it's just the source). > 2. Create a hard link from the destination to the temporary name via `link(2)`. > - If this succeeds, then the destination (`fooBAR` in the example > above) was removed in step (1), which means it was a casefold (+F) > directory and we have successfully changed the case. Proceed to (3). > - If this fails with `EEXIST`, the destination still exists. Proceed > to (3). > - If this fails with any other reason, revert (1) and exit early. > 3. Unlink the temporary name. > In case of casefold filesystems, the source briefly disappears in (1) > but comes back with the destination name in (2). > In case of non-casefold filesystems, this is still correct: since (2) > does nothing, (1) and (3) together simply unlinks the source without > touching the destination. That actually looks cleaner than what I had in mind (rename to temporary, then rename to destination), but there is still the same problem that we need temporary filename. Not sure how to do that in a good/clean way? Also to alleviate possible worries, even though this complicates the code a bit, it's a path that won't be taken except when renaming files to themselves, but in different casing, so it won't slow down the most general path. And even in this case, it was currently broken, so IMO correctness is more important than performance when the latter means it isn't even working (treated as no-op). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93956
On Thu Feb 6 13:12:41 2025 +0000, Jinoh Kang wrote:
There is probably no fundamental improvements in I/O aspect, although I believe the name `is_same_dentry` is much clearer. I like this version better: ```c /* Determine if two paths have the same parent directories and basenames. */ static int is_same_dentry( const char *pathname1, const char *pathname2 ) { const char *lastslash1 = strrchr(pathname1, '/'); const char *lastslash2 = strrchr(pathname2, '/'); char *dirname1 = NULL, dirname2 = NULL; int res; /* Last slash must be preceded by a non-slash character */ assert( lastslash1 && lastslash1 != pathname1 && lastslash1[-1] != '/' ); assert( lastslash2 && lastslash2 != pathname2 && lastslash2[-1] != '/' ); /* different basenames imply different dentries */ if (strcmp( lastslash1 + 1, lastslash2 + 1 ) != 0) return 0; res = -1; if ((dirname1 = memdup( pathname1, lastslash1 + 1 - pathname1 )) && (dirname2 = memdup( pathname2, lastslash2 + 1 - pathname2 ))) { struct stat st1, st2; dirname1[lastslash1 - pathname1] = '\0'; dirname2[lastslash2 - pathname2] = '\0'; if (stat(dirname1, &st1) || stat(dirname2, &st2)) { file_set_error(); } else { res = st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino; } } free(dirname2); free(dirname1); return res; } ``` Are you sure that "name" (I mean the one passed to the function) can't have trailing slashes or multiple of them? I somehow remember it was a possible concern at some point long ago when the other case-insens patchset got reviewed by Rémi, but not sure anymore.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93957
On Thu Feb 6 22:10:35 2025 +0000, Gabriel Ivăncescu wrote:
That actually looks cleaner than what I had in mind (rename to temporary, then rename to destination), but there is still the same problem that we need temporary filename. Not sure how to do that in a good/clean way? Also to alleviate possible worries, even though this complicates the code a bit, it's a path that won't be taken except when renaming files to themselves, but in different casing, so it won't slow down the most general path. And even in this case, it was currently broken, so IMO correctness is more important than performance when the latter means it isn't even working (treated as no-op). Temporary file name can be allocated in a similar manner to mkstemp(3) or make\_temp\_file() in server/mapping.c. I don't think we have "the clean way" to do this, just compromising between various alternatives with different tradeoffs. Note that, as you said, the code was already incorrect.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93980
On Thu Feb 6 22:14:36 2025 +0000, Gabriel Ivăncescu wrote:
Are you sure that "name" (I mean the one passed to the function) can't have trailing slashes or multiple of them? I somehow remember it was a possible concern at some point long ago when the other case-insens patchset got reviewed by Rémi, but not sure anymore. I think `lookup_unix_name` is supposed to reject trailing slashes with `OBJECT_NAME_INVALID`, because it's just a special case of an empty path component.
This is just a suggestion, so it's not necessary to take my code as-is. I still think that it's better to (1) adopt an unambiguous name, and (2) try not to modify the argument at all but instead allocate a separate buffer for that (modifying and reverting is error-prone in general). For completeness, `fd->unix_name` is returned by realpath(), so not a concern. Correct me if I'm wrong, though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6855#note_93981
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)