[PATCH v4 1/2] ntdll: Allow renaming a file/directory to a different casing of itself.
Rémi Bernon
rbernon at codeweavers.com
Wed Apr 21 06:55:12 CDT 2021
On 4/21/21 1:40 PM, Gabriel Ivăncescu wrote:
> 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 at 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)
>
I don't think it should ever happen?
I mean, even with the strrchr fallback above you would have caselen == 0
only if the last path element is empty, which means an empty filename in
unix_name?
Handling caselen == 0 in the common case should work, it would just do
0-size comparisons and copies.
If it's not valid input then I expect the code to be handling the case
already somehow, possibly because rename / link would fail, and they
would fail here all the same?
>>> 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.
>
I don't think it's useful and it makes the logic more complicated than
it need to be.
We have already read the whole filename to compare the casing, so I
don't think the copy would take any noticeable time, even with long file
names.
>>> 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
>>>
>>
>>
>
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list