[PATCH resend 1/2] ntdll: Allow renaming a file/directory to a different capitalization of itself.

[Gabriel Ivăncescu]

On 26/10/2020 17:01, Zebediah Figura wrote:
> 
> 
> On 10/26/20 8:30 AM, Gabriel Ivăncescu wrote:
>> Hi Zeb,
>>
>> Thanks for the review!
>>
>> On 25/10/2020 00:36, Zebediah Figura wrote:
>>> On 10/8/20 9:55 AM, Gabriel Ivăncescu wrote:
>>>> Renaming a file or directory from e.g. foobar to FooBar (or any other
>>>> caps-only change) should work and capitalize it, like on Windows, instead
>>>> of being a no-op.
>>>>
>>>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46203
>>>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>>>> ---
>>>>    dlls/ntdll/unix/file.c | 48 ++++++++++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 46 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c
>>>> index afb552b..639ea6c 100644
>>>> --- a/dlls/ntdll/unix/file.c
>>>> +++ b/dlls/ntdll/unix/file.c
>>>> @@ -4326,9 +4326,10 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io,
>>>>            if (len >= sizeof(FILE_RENAME_INFORMATION))
>>>>            {
>>>>                FILE_RENAME_INFORMATION *info = ptr;
>>>> +            char *unix_name, *src_unix;
>>>>                UNICODE_STRING name_str;
>>>>                OBJECT_ATTRIBUTES attr;
>>>> -            char *unix_name;
>>>> +            size_t unix_name_len;
>>>>    
>>>>                name_str.Buffer = info->FileName;
>>>>                name_str.Length = info->FileNameLength;
>>>> @@ -4343,13 +4344,56 @@ NTSTATUS WINAPI NtSetInformationFile( HANDLE handle, IO_STATUS_BLOCK *io,
>>>>                if (io->u.Status != STATUS_SUCCESS && io->u.Status != STATUS_NO_SUCH_FILE)
>>>>                    break;
>>>>    
>>>> +            unix_name_len = strlen(unix_name);
>>>> +
>>>> +            /* When it's renaming to the same file, preserve the case sensitivity of the last
>>>> +               component, so that renaming a\b\c\foobar to a\b\c\Foobar works correctly. */
>>>> +            if (io->u.Status != STATUS_NO_SUCH_FILE && !server_get_unix_name(handle, &src_unix))
>>>> +            {
>>>> +                char *name = realpath(unix_name, NULL);
>>>> +
>>>> +                if (name && !strcmp(name, src_unix))
>>>> +                {
>>>
>>> Do we even need to make either of these checks?
>>>
>>> (What happens if you try to clobber an existing file with different casing?)
>>>
>>
>> Interesting. It looks like Windows keeps the casing of the rename
>> operation even when clobbering an existing file.
>>
>> e.g.: `Abc` is an existing file. Rename `def` into `ABC` and overwrite,
>> we get `ABC` with the contents of `def`, and `Abc` is no more.
>>
>> This means the patch is incomplete.
>>
>> However, this also means I'd have to pass both the existing unix
>> filename (as we do now) and, on top of that, the new casing filename
>> (without path) to the wineserver.
>>
>> What would be the best way to do that? Currently we just send the unix
>> filename path without a NUL terminator.
>>
>> Would something like the following be acceptable, or do you have a
>> better idea? Perhaps using length-prefix so it's faster to decode?
>>
>> unix filename path, NUL, actual casing filename
>>
>> So with the above example, renaming `def` would look like:
>>
>> C:\Abc<NUL>ABC
>>
>> Obviously it will require a bit more work to decode this.
> 
> I don't really know, but honestly, I'm inclined to think that without an
> application known to depend on this, the extra complexity just isn't
> worth having. That likely goes for the patch in general as well.
> 

Well there are applications that "depend" on this, not by crashing or 
giving a failure error, but simply unable to rename such files.

Most notably, any file managers under Wine won't be able to rename a 
file to some other casing of itself. So it's more like a missing 
feature, but IMO it's still a bug that should be solved somehow.

>>
>>>> +                    size_t nt_filename_len, pathlen;
>>>> +                    const WCHAR *nt_filename;
>>>> +                    char *tmp;
>>>> +                    INT maxsz;
>>>> +
>>>> +                    for (pathlen = name_str.Length / sizeof(WCHAR); pathlen; pathlen--)
>>>> +                        if (name_str.Buffer[pathlen - 1] == '\\')
>>>> +                            break;
>>>
>>> wcsrchr()?
>>>
>>
>> For some reason, it looks like the string is not NUL terminated. I
>> assumed it was intended and not a bug. I can add a comment, though.
> 
> Never mind, you're right; we can't assume the string is NULL terminated.
> 
>>
>>>> +
>>>> +                    nt_filename     = name_str.Buffer + pathlen;
>>>> +                    nt_filename_len = name_str.Length / sizeof(WCHAR) - pathlen;
>>>> +
>>>> +                    /* Build it from path + filename (the latter converted from nt_filename) */
>>>> +                    for (pathlen = unix_name_len; pathlen; pathlen--)
>>>> +                        if (unix_name[pathlen - 1] == '/')
>>>> +                            break;
>>>> +
>>>> +                    tmp = unix_name;
>>>> +                    maxsz = pathlen + nt_filename_len * 3 + 1;
>>>> +                    if (unix_name_len + 1 < maxsz) tmp = realloc(tmp, maxsz);
>>>
>>> This reallocation strategy is very weird.
>>>
>>
>> Basically I'm trying to make room for the "converted" nt_filename into
>> unix. It might take more room when converted to UTF-8 for example,
>> depending on the character.
>>
>> I noticed in other parts of the code that deals with such conversions,
>> such as nt_to_unix_file_name_attr, that it uses something like:
>>
>>       name_len * 3 + MAX_DIR_ENTRY_LEN + 3;
>>
>> so I assumed the * 3 is due to wide -> UTF-8 conversion, and used
>> something similar.
>>
>> That said, it probably won't be needed to reallocate if I go with the
>> new approach to pass it to the wineserver. I'll probably still need the
>> `name_len * 3` thing though, before I copy it, just to have enough room.
>>
>> Thanks,
>> Gabriel
>>
>