[PATCH v10] ntdll: Allow renaming a file/directory to a different case of itself.

[Gabriel Ivăncescu]

On 20/05/2021 18:39, Rémi Bernon wrote:
> On 5/20/21 4:57 PM, Gabriel Ivăncescu wrote:
>> Hi Rémi,
>>
>> On 20/05/2021 14:49, Rémi Bernon wrote:
>>> On 4/30/21 3:58 PM, Gabriel Ivăncescu wrote:
>>>> +/*********************************************************************** 
>>>>
>>>> + *           nt_to_unix_file_name_with_actual_case
>>>> + *
>>>> + * Same as nt_to_unix_file_name, but additionally return unix file 
>>>> name
>>>> + * without path, with the actual case from the NT file name.
>>>> + */
>>>> +static NTSTATUS nt_to_unix_file_name_with_actual_case( const 
>>>> OBJECT_ATTRIBUTES *attr, char **name_ret,
>>>> +                                                       char 
>>>> **actual_case_ret, UINT disposition )
>>>> +{
>>>> +    const WCHAR *nt_filename = attr->ObjectName->Buffer;
>>>> +    char *actual_case, *p;
>>>> +    NTSTATUS status;
>>>> +    int len;
>>>> +
>>>> +    /* strip off trailing backslashes; we also accept '/' for unix 
>>>> namespaces */
>>>> +    for (len = attr->ObjectName->Length / sizeof(WCHAR); len != 0; 
>>>> len--)
>>>> +        if (nt_filename[len - 1] != '\\' && nt_filename[len - 1] != 
>>>> '/')
>>>> +            break;
>>>> +
>>>> +    /* get the last component */
>>>> +    for (nt_filename += len; nt_filename != 
>>>> attr->ObjectName->Buffer; nt_filename--)
>>>> +        if (nt_filename[-1] == '\\' || nt_filename[-1] == '/')
>>>> +            break;
>>>> +    len = attr->ObjectName->Buffer + len - nt_filename;
>>>> +
>>>> +    if (!(actual_case = malloc( len * 3 + 1 ))) return 
>>>> STATUS_NO_MEMORY;
>>>> +
>>>> +    status = nt_to_unix_file_name( attr, name_ret, disposition );
>>>> +    if (status != STATUS_SUCCESS && status != STATUS_NO_SUCH_FILE)
>>>> +    {
>>>> +        free( actual_case );
>>>> +        return status;
>>>> +    }
>>>> +
>>>> +    /* special case for '/' root itself, as it has no named 
>>>> components */
>>>> +    for (p = *name_ret; *p == '/'; p++) { }
>>>> +    if (!*p)
>>>> +        strcpy( actual_case, "/" );
>>>> +    else
>>>> +    {
>>>> +        len = ntdll_wcstoumbs( nt_filename, len, actual_case, len * 
>>>> 3, TRUE );
>>>> +        if (len > 0)
>>>> +            actual_case[len] = 0;
>>>> +        else
>>>> +        {
>>>> +            char *p = strrchr( *name_ret, '/' );
>>>> +            p = p ? p + 1 : *name_ret;
>>>> +            strcpy( actual_case, p );
>>>> +        }
>>>> +    }
>>>> +
>>>> +    *actual_case_ret = actual_case;
>>>> +    return status;
>>>> +}
>>>> +
>>>> +
>>>
>>> I feel like this is more complicated than it needs to be and that it 
>>> will still miss some cases. For instance, the strrchr is incorrect if 
>>> name_ret has trailing slashes.
>>>
>>> Since 405666b736f7e471e301f051cfbe68bcbef7e0f6 there's checks in 
>>> nt_to_unix_file_name to prevent names to have more than one trailing 
>>> slash. However, when the unix name shortcut matches, it may still 
>>> return it with a trailing slash, while when it does it component by 
>>> component, it won't.
>>>
>>> So in my opinion, to get the case of the last path component in a way 
>>> that is consistent with the matched unix path, it all should be done 
>>> in lookup_unix_name instead, as in the attached patch (although I did 
>>> it quickly and I'm not sure it's completely correct).
>>>
>>> The patch doesn't try to normalize the returned paths (stripping the 
>>> last / when the shortcut is taken), and maybe it would be better to 
>>> do so (for the part below).
>>>
>>
>> Thanks, on retrospect, that does look a lot better. Although it seems 
>> slightly off in some corner cases (when the shortcut's ret == 0):
>>
>> 1) For unix root, e.g.: \\?\unix\
>> 2) For a DOS device path with a backslash, e.g.: \\?\C:\
>>
>> In those cases the full path maps to the underlying path with an 
>> actual component (/ for root), while the file_case is an empty string.
>>
>> The simplest fix, IMO, is to simply treat an empty file_case as "the 
>> same case" as I had originally. It's literally just a couple extra 
>> checks on the server and avoids a lot of such problems. Such 
>> situations would simply avoid the case thing altogether.
>>
>> Without that, we'd have to special case the unix root again (treat it 
>> as filename) but also the other root DOS device paths by walking 
>> backwards or something. Really not worth the trouble.
>>
>> It also avoids the string copy when the case fails, removing an extra 
>> line of code in ntdll. :-)
>>
> I don't know, I feel like having file_case sometimes set and sometimes 
> not, confusing. Especially if it's sometimes set but with the same case 
> for some reason. And checking if the case it the same to discard the 
> result feels also a bit ad-hoc.
> 
> Regarding the helper itself, as it takes a case_ret parameter, I would 
> say it's expected to fill it when name_ret it, with the last component 
> original case (and an eventual trailing slash I guess, if name_ret has).
> 
> Also note that I added a new helper because I was lazy and didn't want 
> to change everywhere nt_to_unix_file_name is used, but maybe it's not 
> worth a new function.

Yeah, it still fills it right now. The only time it doesn't, is in weird 
cases when it maps to a device name (or the root / dir), or when the 
allocation fails, which makes sense to me as it uses it as fallback. 
Note that there's not really any special casing done for this in ntdll, 
it's implicit based on the code already. Just the server needs to check 
for it.

It's more along the lines of "don't apply the case check at all" rather 
than "this has a case but I don't want to send it" thing, if that makes 
sense.

For the leak: good point, I'll see if a buffer is better, or can just 
free the buffer if it's easier.