[PATCH v3 1/2] ntdll: Allow renaming a file/directory to a different casing of itself.
Renaming a file or directory from e.g. foobar to FooBar (or any other casing change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new casing. Note that we only send the different casing (and thus casing_len != 0) if it's actually different. This is important to preserve atomicity of rename() in wineserver in all other cases that don't require this special casing handling. The only extra time rename() is not atomic is when an existing file is clobbered with a different casing, since we have to unlink the old file first, then rename the new file into the new casing, but there's no way around that... Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and add an extra (optional) field `casing_len` which is the length of this data. If there's no casing, this is zero and the behavior is identical to before, which is the default. dlls/ntdll/unix/file.c | 46 ++++++++++++++++++--- server/fd.c | 94 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 107 insertions(+), 35 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 9ee5e84..fa604a1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4508,8 +4508,9 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, nt_name = { 0 }; + char *unix_name, *casing = NULL; OBJECT_ATTRIBUTES attr; - char *unix_name; + int casing_len = 0; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4524,19 +4525,52 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE) break; + /* get the casing of the last component if target exists */ + if (io->u.Status != STATUS_NO_SUCH_FILE) + { + const WCHAR *nt_filename = name_str.Buffer + name_str.Length / sizeof(WCHAR); + size_t nt_filename_len; + + /* name_str is not NUL terminated; look for last \ character */ + for (; nt_filename != name_str.Buffer; nt_filename--) + if (nt_filename[-1] == '\\') + break; + nt_filename_len = name_str.Buffer + name_str.Length / sizeof(WCHAR) - nt_filename; + + /* see if only casing differs from nt_name and update it */ + if (nt_name.Length / sizeof(WCHAR) >= nt_filename_len) + { + WCHAR *p = nt_name.Buffer + nt_name.Length / sizeof(WCHAR) - nt_filename_len; + + if ((p == nt_name.Buffer || p[-1] == '\\') && + memcmp( p, nt_filename, nt_filename_len * sizeof(WCHAR) ) && + !wcsnicmp( p, nt_filename, nt_filename_len ) && + (casing = malloc( nt_filename_len * 3 ))) + { + casing_len = ntdll_wcstoumbs( nt_filename, nt_filename_len, casing, + nt_filename_len * 3, TRUE ); + if (casing_len < 0) casing_len = 0; + if (casing_len) memcpy( p, nt_filename, nt_filename_len * sizeof(WCHAR) ); + } + } + } + SERVER_START_REQ( set_fd_name_info ) { - req->handle = wine_server_obj_handle( handle ); - req->rootdir = wine_server_obj_handle( attr.RootDirectory ); - req->namelen = nt_name.Length; - req->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = nt_name.Length; + req->casing_len = casing_len; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, nt_name.Buffer, nt_name.Length ); + wine_server_add_data( req, casing, casing_len ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; + free( casing ); free( unix_name ); free( nt_name.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 481e9a8..502d958 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,7 +2488,8 @@ static void set_fd_disposition( struct fd *fd, int unlink ) /* 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, int replace ) + const char *casing, data_size_t casing_len, struct unicode_str nt_name, + int create_link, int replace ) { struct inode *inode; struct stat st, st2; @@ -2538,45 +2539,77 @@ 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 (!casing_len) + { + free( name ); + return; + } } - - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) + 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; } + + /* 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 */ + /* we also have to unlink it if target has different casing */ + if (create_link || S_ISDIR( st2.st_mode ) || casing_len) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } + } + + /* replace the last component with its actual casing */ + if (casing_len) + { + char *p = strrchr( name, '/' ); + size_t orig_len, path_len; + + p = p ? p + 1 : name; + path_len = p - name; + orig_len = strlen(p); - /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (orig_len < casing_len) { - if (unlink( name )) + char *new_name = mem_alloc( path_len + casing_len + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, path_len ); + free( name ); + name = new_name; } + memcpy(name + path_len, casing, casing_len); + name[path_len + casing_len] = 0; } if (create_link) @@ -2879,16 +2912,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *filename, *casing; 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->casing_len) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + casing = (const char *)get_req_data() + req->namelen; + filename = casing + req->casing_len; if (req->rootdir) { @@ -2902,8 +2938,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->replace ); + set_fd_name( fd, root_fd, filename, (const char *)get_req_data() + get_req_data_size() - filename, + casing, req->casing_len, nt_name, req->link, req->replace ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index 617818f..d97cd1a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3532,9 +3532,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 casing_len; /* length of the casing filename */ int link; /* link instead of renaming */ int replace; /* replace an existing file? */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(casing,string,casing_len); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END -- 2.30.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/kernel32/tests/file.c | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 5deed96..dfa69db 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -1954,6 +1954,7 @@ static void test_MoveFileA(void) char tempdir[MAX_PATH]; char source[MAX_PATH], dest[MAX_PATH]; static const char prefix[] = "pfx"; + WIN32_FIND_DATAA find_data; HANDLE hfile; HANDLE hmapfile; DWORD ret; @@ -2023,9 +2024,72 @@ static void test_MoveFileA(void) ok(ret, "MoveFileA: failed, error %d\n", GetLastError()); lstrcatA(tempdir, "Remove Me"); + + /* test renaming a file "Remove Me" to itself but in lowercase "me" */ + lstrcpyA(source, tempdir); + tempdir[lstrlenA(tempdir) - 2] = 'm'; + + hfile = CreateFileA(source, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS, 0, 0); + ok(hfile != INVALID_HANDLE_VALUE, "failed to create %s\n", source); + CloseHandle(hfile); + + ret = MoveFileA(source, tempdir); + ok(ret, "MoveFileA: failed, error %d\n", GetLastError()); + + hfile = FindFirstFileA(tempdir, &find_data); + ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %d\n", GetLastError()); + if (hfile != INVALID_HANDLE_VALUE) + { + ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), + "MoveFile failed to change caps on same file: got %s\n", find_data.cFileName); + } + CloseHandle(hfile); + + /* test renaming another file "Remove Be" to "Remove Me", which replaces the existing "Remove me" */ + tempdir[lstrlenA(tempdir) - 2] = 'B'; + + hfile = CreateFileA(tempdir, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS, 0, 0); + ok(hfile != INVALID_HANDLE_VALUE, "failed to create %s\n", tempdir); + CloseHandle(hfile); + + ret = MoveFileA(tempdir, source); + ok(!ret, "MoveFileA: expected failure\n"); + ok(GetLastError() == ERROR_ALREADY_EXISTS, "MoveFileA: expected ERROR_ALREADY_EXISTS, got %d\n", GetLastError()); + ret = MoveFileExA(tempdir, source, MOVEFILE_REPLACE_EXISTING); + ok(ret, "MoveFileExA: failed, error %d\n", GetLastError()); + + tempdir[lstrlenA(tempdir) - 2] = 'm'; + + hfile = FindFirstFileA(tempdir, &find_data); + ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %d\n", GetLastError()); + if (hfile != INVALID_HANDLE_VALUE) + { + ok(!lstrcmpA(strrchr(source, '\\') + 1, find_data.cFileName), + "MoveFile failed to change caps on existing target file: got %s\n", find_data.cFileName); + } + CloseHandle(hfile); + + ret = DeleteFileA(tempdir); + ok(ret, "DeleteFileA: error %d\n", GetLastError()); + + /* now test a directory from "Remove me" to uppercase "Me" */ ret = CreateDirectoryA(tempdir, NULL); ok(ret == TRUE, "CreateDirectoryA failed\n"); + lstrcpyA(source, tempdir); + tempdir[lstrlenA(tempdir) - 2] = 'M'; + ret = MoveFileA(source, tempdir); + ok(ret, "MoveFileA: failed, error %d\n", GetLastError()); + + hfile = FindFirstFileA(tempdir, &find_data); + ok(hfile != INVALID_HANDLE_VALUE, "FindFirstFileA: failed, error %d\n", GetLastError()); + if (hfile != INVALID_HANDLE_VALUE) + { + ok(!lstrcmpA(strrchr(tempdir, '\\') + 1, find_data.cFileName), + "MoveFile failed to change caps on same directory: got %s\n", find_data.cFileName); + } + CloseHandle(hfile); + lstrcpyA(source, dest); lstrcpyA(dest, tempdir); lstrcatA(dest, "\\wild?.*"); -- 2.30.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=88517 Your paranoid android. === debiant2 (32 bit Chinese:China report) === ntdll: threadpool.c:1960: Test failed: WaitForSingleObject returned 258 === debiant2 (64 bit WoW report) === ntdll: om.c:2290: Test failed: got 89
Hi, Can someone review this please? I kept resending it to no avail—I'm of course open to *any* ideas how to fix this, it's not of concern and I have no strong opinions, just want it to work. It does seem that prepending more data (such as the NtName now) is ok at the very least, so it can't be obviously too wrong or unacceptable. BTW I also made sure it can't crash the wineserver no matter what data it sends; I have checks for overflow. We have to prepend the casing data due to the fact that: 1) We need the exact actual filename (without true casing) to test on the wineserver to see if it's the same file. This is important. 2) We need the casing of it, to know what to rename it into. Note that this is only needed in case it's different than the filename. For files that don't exist, no casing is prepended, and it's identical to before. For already existing files, we still need the old (existing) filename, even if we somehow check some other way for "same file", because we need to remove the file first (unlink), then rename the new one into the actual casing (tests demonstrate this). For example: existing file 'foobar' and 'bazfoo'. Rename file 'bazfoo' into 'FooBar' and replace the original 'foobar'. In this case, 'bazfoo' will now be called 'FooBar' (new casing) and the original 'foobar' is gone. This patch also updates the NtName to use the actual casing of course, so it matches the new file name. Thanks, Gabriel
Hi Gabriel, I don't know much about the code but I had a look nonetheless, hopefully it'll help. On 4/9/21 5:47 PM, Gabriel Ivăncescu wrote:
Renaming a file or directory from e.g. foobar to FooBar (or any other casing change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new casing.
Note that we only send the different casing (and thus casing_len != 0) if it's actually different. This is important to preserve atomicity of rename() in wineserver in all other cases that don't require this special casing handling. The only extra time rename() is not atomic is when an existing file is clobbered with a different casing, since we have to unlink the old file first, then rename the new file into the new casing, but there's no way around that...
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and add an extra (optional) field `casing_len` which is the length of this data. If there's no casing, this is zero and the behavior is identical to before, which is the default.
dlls/ntdll/unix/file.c | 46 ++++++++++++++++++--- server/fd.c | 94 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 107 insertions(+), 35 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 9ee5e84..fa604a1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4508,8 +4508,9 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, nt_name = { 0 }; + char *unix_name, *casing = NULL; OBJECT_ATTRIBUTES attr; - char *unix_name; + int casing_len = 0;
name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4524,19 +4525,52 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE) break;
+ /* get the casing of the last component if target exists */ + if (io->u.Status != STATUS_NO_SUCH_FILE) + { + const WCHAR *nt_filename = name_str.Buffer + name_str.Length / sizeof(WCHAR); + size_t nt_filename_len; + + /* name_str is not NUL terminated; look for last \ character */ + for (; nt_filename != name_str.Buffer; nt_filename--) + if (nt_filename[-1] == '\\') + break; + nt_filename_len = name_str.Buffer + name_str.Length / sizeof(WCHAR) - nt_filename; + + /* see if only casing differs from nt_name and update it */ + if (nt_name.Length / sizeof(WCHAR) >= nt_filename_len) + { + WCHAR *p = nt_name.Buffer + nt_name.Length / sizeof(WCHAR) - nt_filename_len; + + if ((p == nt_name.Buffer || p[-1] == '\\') && + memcmp( p, nt_filename, nt_filename_len * sizeof(WCHAR) ) && + !wcsnicmp( p, nt_filename, nt_filename_len ) && + (casing = malloc( nt_filename_len * 3 ))) + { + casing_len = ntdll_wcstoumbs( nt_filename, nt_filename_len, casing, + nt_filename_len * 3, TRUE ); + if (casing_len < 0) casing_len = 0; + if (casing_len) memcpy( p, nt_filename, nt_filename_len * sizeof(WCHAR) ); + } + } + }
This code recently changed upstream so it'll have to be rebased (the nt_name and name_str buffers are now the same so there's no need to update it anymore). Anyway, this feels a bit overcomplicated, and imho we could just always send the correct case version of the file name in addition to the matched unix file path. It would also need to be sent with the other set_fd_name_info below.
+ SERVER_START_REQ( set_fd_name_info ) { - req->handle = wine_server_obj_handle( handle ); - req->rootdir = wine_server_obj_handle( attr.RootDirectory ); - req->namelen = nt_name.Length; - req->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = nt_name.Length; + req->casing_len = casing_len; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, nt_name.Buffer, nt_name.Length ); + wine_server_add_data( req, casing, casing_len ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ;
+ free( casing ); free( unix_name ); free( nt_name.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 481e9a8..502d958 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,7 +2488,8 @@ static void set_fd_disposition( struct fd *fd, int unlink )
/* 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, int replace ) + const char *casing, data_size_t casing_len, struct unicode_str nt_name, + int create_link, int replace ) { struct inode *inode; struct stat st, st2; @@ -2538,45 +2539,77 @@ 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 (!casing_len) + { + free( name ); + return; + } } - - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) + 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; } + + /* 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 */ + /* we also have to unlink it if target has different casing */ + if (create_link || S_ISDIR( st2.st_mode ) || casing_len) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } + }
Then I think this would also become simpler, with this first part being done on "name" (which is the unix path of a matching file), with a few more conditions to not return an error if "name" was the old name of the file and it's renamed to a different casing.
+ + /* replace the last component with its actual casing */ + if (casing_len) + { + char *p = strrchr( name, '/' ); + size_t orig_len, path_len; + + p = p ? p + 1 : name; + path_len = p - name; + orig_len = strlen(p);
- /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (orig_len < casing_len) { - if (unlink( name )) + char *new_name = mem_alloc( path_len + casing_len + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, path_len ); + free( name ); + name = new_name; } + memcpy(name + path_len, casing, casing_len); + name[path_len + casing_len] = 0; }
Then, this instead could be simplified by replacing, unconditionally, the last element in the name path by the correctly cased version (possibly identical), before doing the link and rename calls.
if (create_link) @@ -2879,16 +2912,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *filename, *casing; 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->casing_len) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + casing = (const char *)get_req_data() + req->namelen; + filename = casing + req->casing_len;
if (req->rootdir) { @@ -2902,8 +2938,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->replace ); + set_fd_name( fd, root_fd, filename, (const char *)get_req_data() + get_req_data_size() - filename, + casing, req->casing_len, nt_name, req->link, req->replace ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index 617818f..d97cd1a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3532,9 +3532,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 casing_len; /* length of the casing filename */ int link; /* link instead of renaming */ int replace; /* replace an existing file? */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(casing,string,casing_len); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END
Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
Hi Rémi, Thanks for the review! On 20/04/2021 10:27, Rémi Bernon wrote:
Hi Gabriel,
I don't know much about the code but I had a look nonetheless, hopefully it'll help.
On 4/9/21 5:47 PM, Gabriel Ivăncescu wrote:
Renaming a file or directory from e.g. foobar to FooBar (or any other casing change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new casing.
Note that we only send the different casing (and thus casing_len != 0) if it's actually different. This is important to preserve atomicity of rename() in wineserver in all other cases that don't require this special casing handling. The only extra time rename() is not atomic is when an existing file is clobbered with a different casing, since we have to unlink the old file first, then rename the new file into the new casing, but there's no way around that...
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and add an extra (optional) field `casing_len` which is the length of this data. If there's no casing, this is zero and the behavior is identical to before, which is the default.
dlls/ntdll/unix/file.c | 46 ++++++++++++++++++--- server/fd.c | 94 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 107 insertions(+), 35 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 9ee5e84..fa604a1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4508,8 +4508,9 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, nt_name = { 0 }; + char *unix_name, *casing = NULL; OBJECT_ATTRIBUTES attr; - char *unix_name; + int casing_len = 0; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4524,19 +4525,52 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE) break; + /* get the casing of the last component if target exists */ + if (io->u.Status != STATUS_NO_SUCH_FILE) + { + const WCHAR *nt_filename = name_str.Buffer + name_str.Length / sizeof(WCHAR); + size_t nt_filename_len; + + /* name_str is not NUL terminated; look for last \ character */ + for (; nt_filename != name_str.Buffer; nt_filename--) + if (nt_filename[-1] == '\\') + break; + nt_filename_len = name_str.Buffer + name_str.Length / sizeof(WCHAR) - nt_filename; + + /* see if only casing differs from nt_name and update it */ + if (nt_name.Length / sizeof(WCHAR) >= nt_filename_len) + { + WCHAR *p = nt_name.Buffer + nt_name.Length / sizeof(WCHAR) - nt_filename_len; + + if ((p == nt_name.Buffer || p[-1] == '\\') && + memcmp( p, nt_filename, nt_filename_len * sizeof(WCHAR) ) && + !wcsnicmp( p, nt_filename, nt_filename_len ) && + (casing = malloc( nt_filename_len * 3 ))) + { + casing_len = ntdll_wcstoumbs( nt_filename, nt_filename_len, casing, + nt_filename_len * 3, TRUE ); + if (casing_len < 0) casing_len = 0; + if (casing_len) memcpy( p, nt_filename, nt_filename_len * sizeof(WCHAR) ); + } + } + }
This code recently changed upstream so it'll have to be rebased (the nt_name and name_str buffers are now the same so there's no need to update it anymore).
Anyway, this feels a bit overcomplicated, and imho we could just always send the correct case version of the file name in addition to the matched unix file path.
It would also need to be sent with the other set_fd_name_info below.
Ah ok, when I wrote this, I was worried that would make it less upstreamable because it changed all the operation behavior, not just when the casing is needed, which had more possible side-effects etc.
+ SERVER_START_REQ( set_fd_name_info ) { - req->handle = wine_server_obj_handle( handle ); - req->rootdir = wine_server_obj_handle( attr.RootDirectory ); - req->namelen = nt_name.Length; - req->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = nt_name.Length; + req->casing_len = casing_len; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, nt_name.Buffer, nt_name.Length ); + wine_server_add_data( req, casing, casing_len ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; + free( casing ); free( unix_name ); free( nt_name.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 481e9a8..502d958 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,7 +2488,8 @@ static void set_fd_disposition( struct fd *fd, int unlink ) /* 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, int replace ) + const char *casing, data_size_t casing_len, struct unicode_str nt_name, + int create_link, int replace ) { struct inode *inode; struct stat st, st2; @@ -2538,45 +2539,77 @@ 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 (!casing_len) + { + free( name ); + return; + } } - - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) + 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; } + + /* 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 */ + /* we also have to unlink it if target has different casing */ + if (create_link || S_ISDIR( st2.st_mode ) || casing_len) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } + }
Then I think this would also become simpler, with this first part being done on "name" (which is the unix path of a matching file), with a few more conditions to not return an error if "name" was the old name of the file and it's renamed to a different casing.
The problem here is that I'll have to check if the casing is the same or not, so it would move that check from ntdll to the wineserver. We have to check it for two reasons: 1) renaming a file into a different casing of itself 2) unlinking a target file first, if the casing is different For (2), we currently use an atomic rename() so I'd like to keep it that way unless absolutely needed (only in the case we clobber existing file with different casing).
+ + /* replace the last component with its actual casing */ + if (casing_len) + { + char *p = strrchr( name, '/' ); + size_t orig_len, path_len; + + p = p ? p + 1 : name; + path_len = p - name; + orig_len = strlen(p); - /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (orig_len < casing_len) { - if (unlink( name )) + char *new_name = mem_alloc( path_len + casing_len + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, path_len ); + free( name ); + name = new_name; } + memcpy(name + path_len, casing, casing_len); + name[path_len + casing_len] = 0; }
Then, this instead could be simplified by replacing, unconditionally, the last element in the name path by the correctly cased version (possibly identical), before doing the link and rename calls.
I think, since I'm going to have to check if the casing is the same anyway, that I should only copy the new casing if it's actually different, to avoid the common case where it's not. Note that this code won't really be simplified by doing it unconditionally (that's just an if). What might simplify it, though, is to allocate enough space to hold the new casing (if it's larger) at the beginning so we don't have to reallocate. I'll see if I can do that without introducing too much complexity, since I have to scan the filename already now to see if the casing is different.
if (create_link) @@ -2879,16 +2912,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *filename, *casing; 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->casing_len) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + casing = (const char *)get_req_data() + req->namelen; + filename = casing + req->casing_len; if (req->rootdir) { @@ -2902,8 +2938,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->replace ); + set_fd_name( fd, root_fd, filename, (const char *)get_req_data() + get_req_data_size() - filename, + casing, req->casing_len, nt_name, req->link, req->replace ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index 617818f..d97cd1a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3532,9 +3532,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 casing_len; /* length of the casing filename */ int link; /* link instead of renaming */ int replace; /* replace an existing file? */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(casing,string,casing_len); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END
Cheers,
On 4/20/21 2:33 PM, Gabriel Ivăncescu wrote:
Hi Rémi,
Thanks for the review!
On 20/04/2021 10:27, Rémi Bernon wrote:
Hi Gabriel,
I don't know much about the code but I had a look nonetheless, hopefully it'll help.
On 4/9/21 5:47 PM, Gabriel Ivăncescu wrote:
Renaming a file or directory from e.g. foobar to FooBar (or any other casing change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new casing.
Note that we only send the different casing (and thus casing_len != 0) if it's actually different. This is important to preserve atomicity of rename() in wineserver in all other cases that don't require this special casing handling. The only extra time rename() is not atomic is when an existing file is clobbered with a different casing, since we have to unlink the old file first, then rename the new file into the new casing, but there's no way around that...
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and add an extra (optional) field `casing_len` which is the length of this data. If there's no casing, this is zero and the behavior is identical to before, which is the default.
dlls/ntdll/unix/file.c | 46 ++++++++++++++++++--- server/fd.c | 94 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 107 insertions(+), 35 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 9ee5e84..fa604a1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4508,8 +4508,9 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, nt_name = { 0 }; + char *unix_name, *casing = NULL; OBJECT_ATTRIBUTES attr; - char *unix_name; + int casing_len = 0; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4524,19 +4525,52 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE) break; + /* get the casing of the last component if target exists */ + if (io->u.Status != STATUS_NO_SUCH_FILE) + { + const WCHAR *nt_filename = name_str.Buffer + name_str.Length / sizeof(WCHAR); + size_t nt_filename_len; + + /* name_str is not NUL terminated; look for last \ character */ + for (; nt_filename != name_str.Buffer; nt_filename--) + if (nt_filename[-1] == '\\') + break; + nt_filename_len = name_str.Buffer + name_str.Length / sizeof(WCHAR) - nt_filename; + + /* see if only casing differs from nt_name and update it */ + if (nt_name.Length / sizeof(WCHAR) >= nt_filename_len) + { + WCHAR *p = nt_name.Buffer + nt_name.Length / sizeof(WCHAR) - nt_filename_len; + + if ((p == nt_name.Buffer || p[-1] == '\\') && + memcmp( p, nt_filename, nt_filename_len * sizeof(WCHAR) ) && + !wcsnicmp( p, nt_filename, nt_filename_len ) && + (casing = malloc( nt_filename_len * 3 ))) + { + casing_len = ntdll_wcstoumbs( nt_filename, nt_filename_len, casing, + nt_filename_len * 3, TRUE ); + if (casing_len < 0) casing_len = 0; + if (casing_len) memcpy( p, nt_filename, nt_filename_len * sizeof(WCHAR) ); + } + } + }
This code recently changed upstream so it'll have to be rebased (the nt_name and name_str buffers are now the same so there's no need to update it anymore).
Anyway, this feels a bit overcomplicated, and imho we could just always send the correct case version of the file name in addition to the matched unix file path.
It would also need to be sent with the other set_fd_name_info below.
Ah ok, when I wrote this, I was worried that would make it less upstreamable because it changed all the operation behavior, not just when the casing is needed, which had more possible side-effects etc.
+ SERVER_START_REQ( set_fd_name_info ) { - req->handle = wine_server_obj_handle( handle ); - req->rootdir = wine_server_obj_handle( attr.RootDirectory ); - req->namelen = nt_name.Length; - req->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = nt_name.Length; + req->casing_len = casing_len; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, nt_name.Buffer, nt_name.Length ); + wine_server_add_data( req, casing, casing_len ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; + free( casing ); free( unix_name ); free( nt_name.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 481e9a8..502d958 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,7 +2488,8 @@ static void set_fd_disposition( struct fd *fd, int unlink ) /* 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, int replace ) + const char *casing, data_size_t casing_len, struct unicode_str nt_name, + int create_link, int replace ) { struct inode *inode; struct stat st, st2; @@ -2538,45 +2539,77 @@ 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 (!casing_len) + { + free( name ); + return; + } } - - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) + 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; } + + /* 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 */ + /* we also have to unlink it if target has different casing */ + if (create_link || S_ISDIR( st2.st_mode ) || casing_len) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } + }
Then I think this would also become simpler, with this first part being done on "name" (which is the unix path of a matching file), with a few more conditions to not return an error if "name" was the old name of the file and it's renamed to a different casing.
The problem here is that I'll have to check if the casing is the same or not, so it would move that check from ntdll to the wineserver.
We have to check it for two reasons:
1) renaming a file into a different casing of itself 2) unlinking a target file first, if the casing is different
For (2), we currently use an atomic rename() so I'd like to keep it that way unless absolutely needed (only in the case we clobber existing file with different casing).
Yes, you would have to add a check when stat and fstat succeed and the inode match, meaning "name" is the same as the current fd name. If casing is different in that case, then it means we are trying to rename the file (and it shouldn't fail). I think in that case it can just skip all the other checks, as we will simply link / rename the current fd to a different casing (and if there're more files with conflicting cases, then it'll fail anyway).
+ + /* replace the last component with its actual casing */ + if (casing_len) + { + char *p = strrchr( name, '/' ); + size_t orig_len, path_len; + + p = p ? p + 1 : name; + path_len = p - name; + orig_len = strlen(p); - /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (orig_len < casing_len) { - if (unlink( name )) + char *new_name = mem_alloc( path_len + casing_len + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, path_len ); + free( name ); + name = new_name; } + memcpy(name + path_len, casing, casing_len); + name[path_len + casing_len] = 0; }
Then, this instead could be simplified by replacing, unconditionally, the last element in the name path by the correctly cased version (possibly identical), before doing the link and rename calls.
I think, since I'm going to have to check if the casing is the same anyway, that I should only copy the new casing if it's actually different, to avoid the common case where it's not.
Note that this code won't really be simplified by doing it unconditionally (that's just an if). What might simplify it, though, is to allocate enough space to hold the new casing (if it's larger) at the beginning so we don't have to reallocate.
I'll see if I can do that without introducing too much complexity, since I have to scan the filename already now to see if the casing is different.
I think here all you need is to make sure to unlink "name" when casing is different, and "name" isn't the fd old name, the same way as when "name" is a directory. Then updating "name" with the new casing right before the link / rename calls should handle all remaining cases?
if (create_link) @@ -2879,16 +2912,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *filename, *casing; 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->casing_len) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + casing = (const char *)get_req_data() + req->namelen; + filename = casing + req->casing_len; if (req->rootdir) { @@ -2902,8 +2938,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->replace ); + set_fd_name( fd, root_fd, filename, (const char *)get_req_data() + get_req_data_size() - filename, + casing, req->casing_len, nt_name, req->link, req->replace ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index 617818f..d97cd1a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3532,9 +3532,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 casing_len; /* length of the casing filename */ int link; /* link instead of renaming */ int replace; /* replace an existing file? */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(casing,string,casing_len); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END
Cheers,
-- Rémi Bernon <rbernon(a)codeweavers.com>
On 20/04/2021 16:00, Rémi Bernon wrote:
On 4/20/21 2:33 PM, Gabriel Ivăncescu wrote:
Hi Rémi,
Thanks for the review!
On 20/04/2021 10:27, Rémi Bernon wrote:
Hi Gabriel,
I don't know much about the code but I had a look nonetheless, hopefully it'll help.
On 4/9/21 5:47 PM, Gabriel Ivăncescu wrote:
Renaming a file or directory from e.g. foobar to FooBar (or any other casing change) should work, like on Windows, instead of being a no-op. Clobbering an existing file must also respect the new casing.
Note that we only send the different casing (and thus casing_len != 0) if it's actually different. This is important to preserve atomicity of rename() in wineserver in all other cases that don't require this special casing handling. The only extra time rename() is not atomic is when an existing file is clobbered with a different casing, since we have to unlink the old file first, then rename the new file into the new casing, but there's no way around that...
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and add an extra (optional) field `casing_len` which is the length of this data. If there's no casing, this is zero and the behavior is identical to before, which is the default.
dlls/ntdll/unix/file.c | 46 ++++++++++++++++++--- server/fd.c | 94 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 107 insertions(+), 35 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 9ee5e84..fa604a1 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4508,8 +4508,9 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, nt_name = { 0 }; + char *unix_name, *casing = NULL; OBJECT_ATTRIBUTES attr; - char *unix_name; + int casing_len = 0; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4524,19 +4525,52 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE) break; + /* get the casing of the last component if target exists */ + if (io->u.Status != STATUS_NO_SUCH_FILE) + { + const WCHAR *nt_filename = name_str.Buffer + name_str.Length / sizeof(WCHAR); + size_t nt_filename_len; + + /* name_str is not NUL terminated; look for last \ character */ + for (; nt_filename != name_str.Buffer; nt_filename--) + if (nt_filename[-1] == '\\') + break; + nt_filename_len = name_str.Buffer + name_str.Length / sizeof(WCHAR) - nt_filename; + + /* see if only casing differs from nt_name and update it */ + if (nt_name.Length / sizeof(WCHAR) >= nt_filename_len) + { + WCHAR *p = nt_name.Buffer + nt_name.Length / sizeof(WCHAR) - nt_filename_len; + + if ((p == nt_name.Buffer || p[-1] == '\\') && + memcmp( p, nt_filename, nt_filename_len * sizeof(WCHAR) ) && + !wcsnicmp( p, nt_filename, nt_filename_len ) && + (casing = malloc( nt_filename_len * 3 ))) + { + casing_len = ntdll_wcstoumbs( nt_filename, nt_filename_len, casing, + nt_filename_len * 3, TRUE ); + if (casing_len < 0) casing_len = 0; + if (casing_len) memcpy( p, nt_filename, nt_filename_len * sizeof(WCHAR) ); + } + } + }
This code recently changed upstream so it'll have to be rebased (the nt_name and name_str buffers are now the same so there's no need to update it anymore).
Anyway, this feels a bit overcomplicated, and imho we could just always send the correct case version of the file name in addition to the matched unix file path.
It would also need to be sent with the other set_fd_name_info below.
Ah ok, when I wrote this, I was worried that would make it less upstreamable because it changed all the operation behavior, not just when the casing is needed, which had more possible side-effects etc.
+ SERVER_START_REQ( set_fd_name_info ) { - req->handle = wine_server_obj_handle( handle ); - req->rootdir = wine_server_obj_handle( attr.RootDirectory ); - req->namelen = nt_name.Length; - req->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = nt_name.Length; + req->casing_len = casing_len; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, nt_name.Buffer, nt_name.Length ); + wine_server_add_data( req, casing, casing_len ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; + free( casing ); free( unix_name ); free( nt_name.Buffer ); } diff --git a/server/fd.c b/server/fd.c index 481e9a8..502d958 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,7 +2488,8 @@ static void set_fd_disposition( struct fd *fd, int unlink ) /* 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, int replace ) + const char *casing, data_size_t casing_len, struct unicode_str nt_name, + int create_link, int replace ) { struct inode *inode; struct stat st, st2; @@ -2538,45 +2539,77 @@ 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 (!casing_len) + { + free( name ); + return; + } } - - /* can't replace directories or special files */ - if (!S_ISREG( st.st_mode )) + 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; } + + /* 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 */ + /* we also have to unlink it if target has different casing */ + if (create_link || S_ISDIR( st2.st_mode ) || casing_len) + { + if (unlink( name )) + { + file_set_error(); + goto failed; + } + } } + }
Then I think this would also become simpler, with this first part being done on "name" (which is the unix path of a matching file), with a few more conditions to not return an error if "name" was the old name of the file and it's renamed to a different casing.
The problem here is that I'll have to check if the casing is the same or not, so it would move that check from ntdll to the wineserver.
We have to check it for two reasons:
1) renaming a file into a different casing of itself 2) unlinking a target file first, if the casing is different
For (2), we currently use an atomic rename() so I'd like to keep it that way unless absolutely needed (only in the case we clobber existing file with different casing).
Yes, you would have to add a check when stat and fstat succeed and the inode match, meaning "name" is the same as the current fd name. If casing is different in that case, then it means we are trying to rename the file (and it shouldn't fail).
I think in that case it can just skip all the other checks, as we will simply link / rename the current fd to a different casing (and if there're more files with conflicting cases, then it'll fail anyway).
+ + /* replace the last component with its actual casing */ + if (casing_len) + { + char *p = strrchr( name, '/' ); + size_t orig_len, path_len; + + p = p ? p + 1 : name; + path_len = p - name; + orig_len = strlen(p); - /* link() expects that the target doesn't exist */ - /* rename() cannot replace files with directories */ - if (create_link || S_ISDIR( st2.st_mode )) + if (orig_len < casing_len) { - if (unlink( name )) + char *new_name = mem_alloc( path_len + casing_len + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, path_len ); + free( name ); + name = new_name; } + memcpy(name + path_len, casing, casing_len); + name[path_len + casing_len] = 0; }
Then, this instead could be simplified by replacing, unconditionally, the last element in the name path by the correctly cased version (possibly identical), before doing the link and rename calls.
I think, since I'm going to have to check if the casing is the same anyway, that I should only copy the new casing if it's actually different, to avoid the common case where it's not.
Note that this code won't really be simplified by doing it unconditionally (that's just an if). What might simplify it, though, is to allocate enough space to hold the new casing (if it's larger) at the beginning so we don't have to reallocate.
I'll see if I can do that without introducing too much complexity, since I have to scan the filename already now to see if the casing is different.
I think here all you need is to make sure to unlink "name" when casing is different, and "name" isn't the fd old name, the same way as when "name" is a directory.
Then updating "name" with the new casing right before the link / rename calls should handle all remaining cases?
Yeah, that's what I had in mind. Unfortunately, I can't get rid of the reallocation because of the combined_name and dup_fd_name at the top, so can't allocate extra space if the casing takes more chars (in some specific language). I'll send new version shortly so you can check why, maybe I'm missing something.
if (create_link) @@ -2879,16 +2912,19 @@ DECL_HANDLER(set_fd_disp_info) /* set fd name information */ DECL_HANDLER(set_fd_name_info) { + const char *filename, *casing; 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->casing_len) { set_error( STATUS_INVALID_PARAMETER ); return; } nt_name.str = get_req_data(); nt_name.len = (req->namelen / sizeof(WCHAR)) * sizeof(WCHAR); + casing = (const char *)get_req_data() + req->namelen; + filename = casing + req->casing_len; if (req->rootdir) { @@ -2902,8 +2938,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->replace ); + set_fd_name( fd, root_fd, filename, (const char *)get_req_data() + get_req_data_size() - filename, + casing, req->casing_len, nt_name, req->link, req->replace ); release_object( fd ); } if (root_fd) release_object( root_fd ); diff --git a/server/protocol.def b/server/protocol.def index 617818f..d97cd1a 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3532,9 +3532,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 casing_len; /* length of the casing filename */ int link; /* link instead of renaming */ int replace; /* replace an existing file? */ VARARG(name,unicode_str,namelen); /* NT name */ + VARARG(casing,string,casing_len); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END
Cheers,
participants (3)
-
Gabriel Ivăncescu -
Marvin -
Rémi Bernon