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

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.
> 
> 
> Signed-off-by: Suresh Jayaraman <[email protected]>
> ---
>  fs/cifs/cifssmb.c |   29 +++++++++++++++++------------
>  1 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> 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?

>  	} 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]>