[PATCH v5 0/1] MR246: Allow renaming a file/directory to a different case of itself
-- v5: ntdll: Allow renaming a file/directory to a different case of itself. https://gitlab.winehq.org/wine/wine/-/merge_requests/246
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Renaming a file or directory from e.g. foobar to FooBar (or any other case change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new case. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 4 +- dlls/ntdll/tests/file.c | 4 +- dlls/ntdll/unix/file.c | 58 +++++++++++++++++- server/fd.c | 122 +++++++++++++++++++++++++------------ server/protocol.def | 2 + 5 files changed, 145 insertions(+), 45 deletions(-) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 0493f90e30d..ed89c9fcb4c 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -2032,7 +2032,7 @@ static void test_MoveFileA(void) ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); if (hfile != INVALID_HANDLE_VALUE) { - todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), + ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), "MoveFile failed to change casing on same file: got %s\n", find_data.cFileName); } CloseHandle(hfile); @@ -2077,7 +2077,7 @@ static void test_MoveFileA(void) ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %ld\n", GetLastError()); if (hfile != INVALID_HANDLE_VALUE) { - todo_wine ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), + ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), "MoveFile failed to change casing on same directory: got %s\n", find_data.cFileName); } CloseHandle(hfile); diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index 4e1750a4751..b80a9ae9e8a 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -2428,7 +2428,7 @@ static void test_file_link_information(FILE_INFORMATION_CLASS class) ok(handle != INVALID_HANDLE_VALUE, "FindFirstFileW: failed, error %ld\n", GetLastError()); if (handle != INVALID_HANDLE_VALUE) { - todo_wine ok(!lstrcmpW(wcsrchr(newpath, '\\') + 1, find_data.cFileName), + ok(!lstrcmpW(wcsrchr(newpath, '\\') + 1, find_data.cFileName), "Link did not change casing on existing target file: got %s\n", wine_dbgstr_w(find_data.cFileName)); } @@ -3022,7 +3022,7 @@ static void test_file_link_information(FILE_INFORMATION_CLASS class) ok(handle != INVALID_HANDLE_VALUE, "FindFirstFileW: failed, error %ld\n", GetLastError()); if (handle != INVALID_HANDLE_VALUE) { - todo_wine ok(!lstrcmpW(wcsrchr(oldpath, '\\') + 1, find_data.cFileName), + ok(!lstrcmpW(wcsrchr(oldpath, '\\') + 1, find_data.cFileName), "Link did not change casing on same file: got %s\n", wine_dbgstr_w(find_data.cFileName)); } diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index c6533710365..d76b0e1cdb1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -3823,6 +3823,52 @@ NTSTATUS WINAPI wine_unix_to_nt_file_name( const char *name, WCHAR *buffer, ULON } +/*********************************************************************** + * get_filename_case + * + * Get the unix filename, with the case from NT name's last component. + */ +static char *get_filename_case( const OBJECT_ATTRIBUTES *attr ) +{ + const WCHAR *p, *nt_filename = attr->ObjectName->Buffer; + int len = attr->ObjectName->Length / sizeof(WCHAR); + char *file_case; + + /* skip the device and prefix (allow slashes for unix namespace) */ + if (!attr->RootDirectory) + { + int pos = get_dos_prefix_len( attr->ObjectName ); + while (pos < len) + { + WCHAR c = nt_filename[pos++]; + if (c == '\\' || c == '/') break; + } + nt_filename += pos; + len -= pos; + } + + /* strip off trailing backslashes */ + for (; len; len--) + if (nt_filename[len - 1] != '\\' && nt_filename[len - 1] != '/') + break; + + /* get the last component */ + for (p = nt_filename + len; p != nt_filename; p--) + if (p[-1] == '\\' || p[-1] == '/') + break; + len -= p - nt_filename; + nt_filename = p; + + if ((file_case = malloc( len * 3 + 1 ))) + { + len = ntdll_wcstoumbs( nt_filename, len, file_case, len * 3, TRUE ); + if (len < 0 || len > MAX_DIR_ENTRY_LEN) len = 0; + file_case[len] = 0; + } + return file_case; +} + + /*********************************************************************** * get_full_path * @@ -4789,8 +4835,8 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, FILE_RENAME_INFORMATION *info = ptr; unsigned int flags; UNICODE_STRING name_str, redir; + char *unix_name, *file_case; OBJECT_ATTRIBUTES attr; - char *unix_name; if (class == FileRenameInformation) flags = info->ReplaceIfExists ? FILE_RENAME_REPLACE_IF_EXISTS : 0; @@ -4809,20 +4855,24 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, status = nt_to_unix_file_name( &attr, &unix_name, FILE_OPEN_IF ); if (status == STATUS_SUCCESS || status == STATUS_NO_SUCH_FILE) { + file_case = get_filename_case( &attr ); SERVER_START_REQ( set_fd_name_info ) { req->handle = wine_server_obj_handle( handle ); req->rootdir = wine_server_obj_handle( attr.RootDirectory ); req->namelen = attr.ObjectName->Length; + req->caselen = file_case ? strlen( file_case ) : 0; req->link = FALSE; req->flags = flags; wine_server_add_data( req, attr.ObjectName->Buffer, attr.ObjectName->Length ); + wine_server_add_data( req, file_case, req->caselen ); wine_server_add_data( req, unix_name, strlen(unix_name) ); status = wine_server_call( req ); } SERVER_END_REQ; free( unix_name ); + free( file_case ); } free( redir.Buffer ); } @@ -4836,8 +4886,8 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, FILE_LINK_INFORMATION *info = ptr; unsigned int flags; UNICODE_STRING name_str, redir; + char *unix_name, *file_case; OBJECT_ATTRIBUTES attr; - char *unix_name; if (class == FileLinkInformation) flags = info->ReplaceIfExists ? FILE_LINK_REPLACE_IF_EXISTS : 0; @@ -4856,20 +4906,24 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, status = nt_to_unix_file_name( &attr, &unix_name, FILE_OPEN_IF ); if (status == STATUS_SUCCESS || status == STATUS_NO_SUCH_FILE) { + file_case = get_filename_case( &attr ); SERVER_START_REQ( set_fd_name_info ) { req->handle = wine_server_obj_handle( handle ); req->rootdir = wine_server_obj_handle( attr.RootDirectory ); req->namelen = attr.ObjectName->Length; + req->caselen = file_case ? strlen( file_case ) : 0; req->link = TRUE; req->flags = flags; wine_server_add_data( req, attr.ObjectName->Buffer, attr.ObjectName->Length ); + wine_server_add_data( req, file_case, req->caselen ); wine_server_add_data( req, unix_name, strlen(unix_name) ); status = wine_server_call( req ); } SERVER_END_REQ; free( unix_name ); + free( file_case ); } free( redir.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 8576882aaa9..174dace783e 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2532,11 +2532,14 @@ static void set_fd_disposition( struct fd *fd, unsigned int flags ) /* 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 ) + const char *file_case, data_size_t caselen, struct unicode_str nt_name, + int create_link, unsigned int flags ) { + size_t pathlen = 0, filenamelen; struct inode *inode; struct stat st, st2; - char *name; + int different_case; + char *name, *p; const unsigned int replace = flags & FILE_RENAME_REPLACE_IF_EXISTS; if (!fd->inode || !fd->unix_name) @@ -2571,6 +2574,30 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da name = combined_name; } + /* get path len up to last component, ignoring trailing slashes */ + for (p = name; *p;) + if (*p++ == '/' && *p) + pathlen = p - name; + + /* get filename ptr and len, without trailing slashes */ + filenamelen = p - name - pathlen; + p = name + pathlen; + while (filenamelen && p[filenamelen - 1] == '/') + filenamelen--; + + different_case = caselen && (filenamelen != caselen || memcmp( p, file_case, caselen )); + + if (filenamelen < caselen) + { + p = realloc( name, pathlen + caselen + 1 ); + if (!p) + { + set_error( STATUS_NO_MEMORY ); + goto failed; + } + name = p; + } + /* when creating a hard link, source cannot be a dir */ if (create_link && !fstat( fd->unix_fd, &st ) && S_ISDIR( st.st_mode )) { @@ -2583,55 +2610,69 @@ 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 ); - free( name ); - return; - } - - if (!replace) - { - set_error( STATUS_OBJECT_NAME_COLLISION ); - goto failed; - } + if (!different_case) + { + free( name ); + return; + } - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) - { - set_error( STATUS_ACCESS_DENIED ); - goto failed; + /* creating a link with a different case on itself renames the file */ + create_link = 0; } - - /* read-only files cannot be replaced */ - if (!(st.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) && - !(flags & FILE_RENAME_IGNORE_READONLY_ATTRIBUTE)) + else { - set_error( STATUS_ACCESS_DENIED ); - goto failed; - } + if (!replace) + { + set_error( STATUS_OBJECT_NAME_COLLISION ); + goto failed; + } - /* can't replace an opened file */ - if ((inode = get_inode( st.st_dev, st.st_ino, -1 ))) - { - int is_empty = list_empty( &inode->open ); - release_object( inode ); - if (!is_empty) + /* can't replace directories or special files */ + if (!S_ISREG( st.st_mode )) { set_error( STATUS_ACCESS_DENIED ); goto failed; } - } - /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) - { - if (unlink( name )) + /* read-only files cannot be replaced */ + if (!(st.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) && + !(flags & FILE_RENAME_IGNORE_READONLY_ATTRIBUTE)) { - file_set_error(); + set_error( STATUS_ACCESS_DENIED ); goto failed; } + + /* can't replace an opened file */ + if ((inode = get_inode( st.st_dev, st.st_ino, -1 ))) + { + int is_empty = list_empty( &inode->open ); + release_object( inode ); + if (!is_empty) + { + set_error( STATUS_ACCESS_DENIED ); + goto failed; + } + } + + /* link() expects that the target doesn't exist */ + /* rename() cannot replace files with directories */ + if (create_link || S_ISDIR( st2.st_mode ) || different_case) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } } + if (different_case) + { + memcpy( name + pathlen, file_case, caselen ); + name[pathlen + caselen] = 0; + } + if (create_link) { if (link( fd->unix_name, name )) @@ -2973,16 +3014,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *fullname, *file_case; struct fd *fd, *root_fd = NULL; struct unicode_str nt_name; - if (req->namelen > get_req_data_size()) + if (req->namelen > get_req_data_size() || get_req_data_size() - req->namelen < req->caselen) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + file_case = (const char *)get_req_data() + req->namelen; + fullname = file_case + req->caselen; if (req->rootdir) { @@ -2996,8 +3040,8 @@ DECL_HANDLER(set_fd_name_info) if ((fd = get_handle_fd_obj( current->process, req->handle, 0 ))) { - set_fd_name( fd, root_fd, (const char *)get_req_data() + req->namelen, - get_req_data_size() - req->namelen, nt_name, req->link, req->flags ); + set_fd_name( fd, root_fd, fullname, (const char *)get_req_data() + get_req_data_size() - fullname, + file_case, req->caselen, nt_name, req->link, req->flags ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index e9195df6b65..1e40d61f418 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3700,9 +3700,11 @@ struct handle_info obj_handle_t handle; /* handle to a file or directory */ obj_handle_t rootdir; /* root directory */ data_size_t namelen; /* length of NT name in bytes */ + data_size_t caselen; /* length of the actual case filename */ int link; /* link instead of renaming */ unsigned int flags; /* FILE_RENAME_* flags */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(actual_case,string,caselen); /* new file name's actual case (without path) */ VARARG(filename,string); /* new file name */ @END -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/246
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 (!different_case) + { free( name ); return; }
+ /* creating a link with a different case on itself renames the file */ + create_link = 0;
What if the destination file is a hard link to the same file as the source, but with a completely different name (not just different case)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_87708
On Sat Nov 16 10:36:27 2024 +0000, Jinoh Kang wrote:
What if the destination file is a hard link to the same file as the source, but with a completely different name (not just different case)? Suppose `foo.txt` and `bar.txt` are hard links to the same file. What happens if we link `foo.txt` to `bar.txt` with `FILE_LINK_REPLACE_IF_EXISTS`?
`different_case = 1` for completely different names too (foo ≠ bar), but I'm not sure this is renaming the file on Windows as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_87709
On Sat Nov 16 10:41:24 2024 +0000, Jinoh Kang wrote:
Suppose `foo.txt` and `bar.txt` are hard links to the same file. What happens if we link `foo.txt` to `bar.txt` with `FILE_LINK_REPLACE_IF_EXISTS`? In your code, `different_case = 1` for completely different names too (foo ≠ bar), but I'm not sure this is renaming the file (deleting `foo.txt`) on Windows as well. Thanks for the review, I had forgotten about this (well, with year end I'd probably look through my stuff again).
Good point. Note that `different_case` should be 1 only when the case is different on the name, but the "name" here is always the destination. The "source" is just a fd. Maybe this is not clear, I should add a comment. So for your question, the behavior is the same as before, because you have a fd to `foo.txt` and then you want to rename it to `bar.txt` (which is already a hardlink to the same file), but in this case the "name" and "case" are the same, both are `bar.txt` so `different_case = 0`. However, if we were to rename it to `Bar.txt` (while `bar.txt` already exists, lowercase), then `different_case = 1`. In this case we'd try to rename `foo.txt` to `Bar.txt`. This is… bad I guess, because now we'd have both `Bar.txt` and `bar.txt` in the same directory (but both are hardlinks to the same file). Any idea how to **detect** this? And apply appropriate fix (depending what happens on Windows). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_87890
@iamahuman I created !6855 to fix an existing issue with renaming into a hardlink. That's a requirement to fix this issue you mentioned anyway. I'd appreciate if you could review and maybe find a better solution than checking paths with realpath? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_88074
On Fri Nov 22 14:39:52 2024 +0000, Gabriel Ivăncescu wrote:
Thanks for the review, I had forgotten about this (well, with year end I'd probably look through my stuff again). Good point. Note that `different_case` should be 1 only when the case is different on the name, but the "name" here is always the destination. The "source" is just a fd. Maybe this is not clear, I should add a comment. So for your question, the behavior is the same as before, because you have a fd to `foo.txt` and then you want to rename it to `bar.txt` (which is already a hardlink to the same file), but in this case the "name" and "case" are the same, both are `bar.txt` so `different_case = 0`. However, if we were to rename it to `Bar.txt` (while `bar.txt` already exists, lowercase), then `different_case = 1`. In this case we'd try to rename `foo.txt` to `Bar.txt`. This is… bad I guess, because now we'd have both `Bar.txt` and `bar.txt` in the same directory (but both are hardlinks to the same file). Any idea how to **detect** this? And apply appropriate fix (depending what happens on Windows). Thanks for the explanation.
However, if we were to rename it to `Bar.txt` (while `bar.txt` already exists, lowercase), then `different_case = 1`. In this case we'd try to rename `foo.txt` to `Bar.txt`.
This is… bad I guess, because now we'd have both `Bar.txt` and `bar.txt` in the same directory (but both are hardlinks to the same file).
Any idea how to **detect** this? And apply appropriate fix (depending what happens on Windows).
Hopefully we can continue this discussion on !6855, when I find time. Nevertheless, I don't consider this blocking if the existing behavior is already broken. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_88509
I've noticed that this MR fails on an ext4 casefold filesystem: ``` file.c:2085: Test failed: MoveFile failed to change casing on same file: got Remove Me file.c:2130: Test failed: MoveFile failed to change casing on same directory: got Remove me ``` I'd place blame on rename(2) itself ignoring case renames if casefold is enabled. Does SteamOS use case-insensitive filesystem? In that case, does Proton take any different approach? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_93314
On Fri Jan 31 18:17:27 2025 +0000, Jinoh Kang wrote:
I've noticed that this MR fails on an ext4 casefold filesystem: ``` file.c:2085: Test failed: MoveFile failed to change casing on same file: got Remove Me file.c:2130: Test failed: MoveFile failed to change casing on same directory: got Remove me ``` I'd place blame on rename(2) itself ignoring case renames if casefold is enabled. Does SteamOS use case-insensitive filesystem? In that case, does Proton take any different approach? Interesting. That seems like a different problem, it won't even reach the `rename`. In fact, if you have say a file `ABC` and attempt to rename to `abc`, `nt_to_unix_file_name` will return /path/to/abc (yes, lowercase) because it won't look it up manually component-by-component, as the direct lookup is valid (due to case folding).
This causes `different_case` in the patch to be 0, since it's the same case (both are `abc`), and the file has the same inode because it's the same file, so it thinks it's a no-op, hence this returns early here: ```c if (!different_case) { free( name ); return; } ``` This probably needs something akin to !6855, I'm not sure. I think at this point, we should try to get that one in first, so we can go from there and see how we can fix this case, otherwise they'd keep ending up conflicting each other. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/246#note_93335
participants (2)
-
Gabriel Ivăncescu -
Jinoh Kang (＠iamahuman)