On 21/04/2021 00:02, Rémi Bernon wrote:
On 4/20/21 5:04 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.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203 Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
We just prepend the unix casing filename (without path) to the wineserver data before the actual unix filename, and a field `casinglen` which is the length of this data.
dlls/ntdll/unix/file.c | 59 ++++++++++++++++++++------ server/fd.c | 95 +++++++++++++++++++++++++++++------------- server/protocol.def | 2 + 3 files changed, 114 insertions(+), 42 deletions(-)
diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 488f748..0cf1a7f 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -2800,6 +2800,31 @@ void init_files(void) } +/***********************************************************************
- * get_nt_file_name_unix_casing
- Get the Unix file name with the same casing as the NT file name
(without path).
- */
+static unsigned int get_nt_file_name_unix_casing(const OBJECT_ATTRIBUTES *attr, char **casing) +{ + const WCHAR *filename = attr->ObjectName->Buffer + attr->ObjectName->Length / sizeof(WCHAR); + int len, casinglen = 0;
+ /* NT name may not be NUL terminated; look for last \ character */ + for (; filename != attr->ObjectName->Buffer; filename--) + if (filename[-1] == '\') + break; + len = attr->ObjectName->Buffer + attr->ObjectName->Length / sizeof(WCHAR) - filename;
+ if ((*casing = malloc( len * 3 ))) + { + casinglen = ntdll_wcstoumbs( filename, len, *casing, len * 3, TRUE ); + if (casinglen < 0) casinglen = 0; + } + return casinglen; +}
/******************************************************************************
* get_dos_device * @@ -4521,8 +4546,8 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_RENAME_INFORMATION *info = ptr; UNICODE_STRING name_str, redir; + char *unix_name, *casing; OBJECT_ATTRIBUTES attr; - char *unix_name; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4533,20 +4558,25 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, io->u.Status = nt_to_unix_file_name( &attr, &unix_name, FILE_OPEN_IF ); if (io->u.Status == STATUS_SUCCESS || io->u.Status == STATUS_NO_SUCH_FILE) { + unsigned int casinglen = get_nt_file_name_unix_casing( &attr, &casing );
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->link = FALSE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = attr.ObjectName->Length; + req->casinglen = casinglen; + req->link = FALSE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, attr.ObjectName->Buffer, attr.ObjectName->Length ); + wine_server_add_data( req, casing, casinglen ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; free( unix_name ); + free( casing ); } free( redir.Buffer ); }
I think it'd be better to handle the casing allocation even before nt_to_unix_file_name, and set Status in case of failure.
Then I'm not sure how likely ntdll_wcstoumbs is expected to fail, but in that case it should probably just use strrchr( unix_name, '/' ) or unix_name, to have a nicer fallback.
@@ -4558,8 +4588,8 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, { FILE_LINK_INFORMATION *info = ptr; UNICODE_STRING name_str, redir; + char *unix_name, *casing; OBJECT_ATTRIBUTES attr; - char *unix_name; name_str.Buffer = info->FileName; name_str.Length = info->FileNameLength; @@ -4570,20 +4600,25 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, io->u.Status = nt_to_unix_file_name( &attr, &unix_name, FILE_OPEN_IF ); if (io->u.Status == STATUS_SUCCESS || io->u.Status == STATUS_NO_SUCH_FILE) { + unsigned int casinglen = get_nt_file_name_unix_casing( &attr, &casing );
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->link = TRUE; - req->replace = info->ReplaceIfExists; + req->handle = wine_server_obj_handle( handle ); + req->rootdir = wine_server_obj_handle( attr.RootDirectory ); + req->namelen = attr.ObjectName->Length; + req->casinglen = casinglen; + req->link = TRUE; + req->replace = info->ReplaceIfExists; wine_server_add_data( req, attr.ObjectName->Buffer, attr.ObjectName->Length ); + wine_server_add_data( req, casing, casinglen ); wine_server_add_data( req, unix_name, strlen(unix_name) ); io->u.Status = wine_server_call( req ); } SERVER_END_REQ; free( unix_name ); + free( casing ); } free( redir.Buffer ); }
Here too you could set casing to be strrchr( unix_name, '/' ) + 1 or unix_name. So it wouldn't need an allocation. It's also probably not worth keeping the separate helper.
(My OCD also tells me that using "caselen" instead of "casinglen" in the request would keep things aligned and save you the re-identation, but that probably doesn't matter much ;))
Alright, sounds good to me, and with such fallback I guess the helper won't really save much code.
diff --git a/server/fd.c b/server/fd.c index 481e9a8..0b4c0a1 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2488,11 +2488,14 @@ 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 casinglen, struct unicode_str nt_name, + int create_link, int replace ) { + char *name, *filename; + int different_casing; struct inode *inode; struct stat st, st2; - char *name; + size_t filenamelen; if (!fd->inode || !fd->unix_name) { @@ -2533,50 +2536,79 @@ static void set_fd_name( struct fd *fd, struct fd *root, const char *nameptr, da goto failed; } + filename = strrchr( name, '/' ); + filename = filename ? filename + 1 : name; + filenamelen = strlen( filename ); + different_casing = casinglen && (filenamelen != casinglen || memcmp( filename, casing, casinglen ));
I think with the fallback above the casinglen == 0 check is probably not very useful here?
Also I would do the name realloc here, or even earlier, right after combined_name, simply checking if casinglen > filenamelen, to make sure we can simply copy casing to filename later.
Good idea. Should I treat the casinglen == 0 as an input error then? (now I'm using it as fallback)
if (!stat( name, &st )) { 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_casing) + { + 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 */ + if (create_link || S_ISDIR( st2.st_mode ) || different_casing) + { + if (unlink( name )) + { + file_set_error(); + 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 (different_casing) + { + size_t pathlen = filename - name; + if (filenamelen < casinglen) { - if (unlink( name )) + char *new_name = mem_alloc( pathlen + casinglen + 1 ); + if (!new_name) { - file_set_error(); + set_error( STATUS_NO_MEMORY ); goto failed; } + memcpy( new_name, name, pathlen ); + free( name ); + name = new_name; } + memcpy(name + pathlen, casing, casinglen); + name[pathlen + casinglen] = 0; }
And so this whole if could be something like that instead:
memcpy( filename, casing, caselen ); filename[caselen] = 0;
Should I not check for different_casing here? It should be the most common case after all and avoid a copy.
if (create_link) @@ -2879,16 +2911,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->casinglen) { 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->casinglen; if (req->rootdir) { @@ -2902,8 +2937,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->casinglen, 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 9ea6967..8560469 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3531,9 +3531,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 casinglen; /* 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,casinglen); /* new file name's actual casing (without path) */ VARARG(filename,string); /* new file name */ @END