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

[Gabriel Ivăncescu]

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. :-)

>> diff --git a/server/fd.c b/server/fd.c
>> index 481e9a8..d73ea56 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 *file_case, data_size_t caselen, 
>> struct unicode_str nt_name,
>> +                         int create_link, int replace )
>>   {
>> +    size_t pathlen, filenamelen;
>>       struct inode *inode;
>>       struct stat st, st2;
>> -    char *name;
>> +    int different_case;
>> +    char *name, *p;
>>       if (!fd->inode || !fd->unix_name)
>>       {
>> @@ -2526,6 +2529,29 @@ static void set_fd_name( struct fd *fd, struct 
>> fd *root, const char *nameptr, da
>>           name = combined_name;
>>       }
>> +    if (!(p = strrchr( name, '/' ))) p = name;
>> +    else if (!*(++p))
>> +    {
>> +        /* get the last slash that's not trailing, but treat '/' root 
>> as a filename */
>> +        while (p > name && p[-1] == '/') p--;
>> +        p[p <= name] = 0;
>> +        while (p > name && p[-1] != '/') p--;
>> +    }
>> +    pathlen = p - name;
>> +    filenamelen = strlen( p );
>> +    different_case = (filenamelen != caselen || memcmp( p, file_case, 
>> caselen ));
>> +
> 
> Here I'm not completely sure. Looking for the last path component could 
> be made simpler with something like that:
> 
>      p = name; pathlen = 0;
>      while (*p) if (*p++ == '/' && *p) pathlen = p - name;
>      filenamelen = p - name - pathlen;
>      p = name + pathlen;
> 
> But then I don't know what we can assume, and whether we can trust 
> client input to have both name and file_case be consistent together.
> 
> Cheers,

Yeah, that's actually really clever, I like it!