Re: [PATCH 2/3] cifs: Make cifs_strlcpy_to_host use this helper and fix incorrect NULL termination

On Tue, Apr 21, 2009 at 11:15 AM, Jeff Layton <[email protected]> wrote:
> On Tue, 21 Apr 2009 17:55:15 +0530
> Suresh Jayaraman <[email protected]> wrote:
>
>> Make cifs_strlcpy_to_host use UniStrnlenBytes(). Also fix a bug
>> introduced by a previous patch. The previous patch while fixing the
>> buffer size, left out the NULL termination that is based on length of
>> src buffer(in UCS characters) as it is. Fix this by using the length of
>> dst buffer got from cifs_strfromUCS_le() for NULL termination.
>> index a02c43b..aab1d32 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -91,26 +91,31 @@ static int
>>  cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen,
>>                const bool is_unicode, const struct nls_table *nls_codepage)
>>  {
>> -     int plen;
>> +     int src_len, dst_len;
>> +     size_t nbytes;
>>
>>       if (is_unicode) {
>> -             plen = UniStrnlen((wchar_t *)src, maxlen);
>> -             *dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
>> +             nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, &src_len,
>> +                                      nls_codepage);
>> +             *dst = kmalloc(nbytes + 2, GFP_KERNEL);
>>               if (!*dst)
>> -                     goto cifs_strlcpy_to_host_ErrExit;
>> -             cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
>> -             (*dst)[plen] = 0;
>> -             (*dst)[plen+1] = 0; /* needed for Unicode */
>> +                     goto err_exit;
>> +             dst_len = cifs_strfromUCS_le(*dst, (__le16 *)src, src_len,
>> +                                          nls_codepage);
>> +             /*
>> +              * cifs_strfromUCS_le() ensures single byte NULL termination
>> +              */
>> +             (*dst)[dst_len + 1] = 0; /* needed for Unicode, to be safe */
>
> This is fine for now, but maybe we should just fix cifs_strfromUCS_le
> to do the double-byte termination? Is a single-byte terminator ever
> useful?
>
> Fixing that will of course mean auditing all of the callers, but I
> think they all have big enough buffers for this now. Right?

Jeff's argument makes sense :)


>>       } else {
>> -             plen = strnlen(src, maxlen);
>> -             *dst = kmalloc(plen + 2, GFP_KERNEL);
>> +             src_len = strnlen(src, maxlen);
>> +             *dst = kmalloc(src_len + 1, GFP_KERNEL);
>>               if (!*dst)
>> -                     goto cifs_strlcpy_to_host_ErrExit;
>> -             strlcpy(*dst, src, plen);
>> +                     goto err_exit;
>> +             strlcpy(*dst, src, src_len);
>                                   ^^^^^^^
> should be src_len+1.
>
>>       }
>>       return 0;
>>
>> -cifs_strlcpy_to_host_ErrExit:
>> +err_exit:
>>       cERROR(1, ("Failed to allocate buffer for string\n"));
>>       return -ENOMEM;
>>  }
>> _______________________________________________
>> linux-cifs-client mailing list
>> [email protected]
>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>
>
>
> --
> Jeff Layton <[email protected]>
>



-- 
Thanks,

Steve