[linux-cifs-client] Re: [PATCH] cifs: add helper to simplify
unicode to NLS conversion and use it (try #2)
Jeff Layton
jlayton at redhat.com
Mon Apr 13 11:39:56 GMT 2009
On Mon, 13 Apr 2009 15:13:16 +0530
Suresh Jayaraman <sjayaraman at suse.de> wrote:
> Thanks for the excellent review comments. Have incorporated all of them
> except the concern that whether "src" is null terminated or not. I think
> it is not required as cifs_strfromUCS_le() takes care of the boundary
> check. Please review the updated patch below.
>
> Add helper to simplify unicode(UCS/UTF-16) to NLS conversion. The helper
> function calculates the memory needed exactly instead of using size
> assumptions and consolidates some common code in there.
>
> Signed-off-by: Suresh Jayaraman <sjayaraman at suse.de>
> ---
>
> fs/cifs/cifs_unicode.h | 28 ++++++++++++++++++++
> fs/cifs/cifsproto.h | 2 +
> fs/cifs/cifssmb.c | 31 ++++++++++++++++++++++
> fs/cifs/connect.c | 67 +++++++++++++++++++----------------------------
> fs/cifs/sess.c | 46 +++++++++++++++-----------------
> 5 files changed, 110 insertions(+), 64 deletions(-)
>
> diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
> index 14eb9a2..06a267b 100644
> --- a/fs/cifs/cifs_unicode.h
> +++ b/fs/cifs/cifs_unicode.h
> @@ -159,6 +159,34 @@ UniStrnlen(const wchar_t *ucs1, int maxlen)
> }
>
> /*
> + * UniStrnlenBytes: Return the length of a NLS string in bytes. Also, populates
> + * 'nchars' with the length of string in 16 bit Unicode chars.
> + */
> +static inline size_t
> +UniStrnlenBytes(const wchar_t *str, int maxlen, int *nchars,
> + const struct nls_table *codepage)
> +{
> + int nc;
> + size_t i = 0, nbytes = 0;
> + wchar_t uni = *str;
> + char buf[NLS_MAX_CHARSET_SIZE]; /* enough for one char at a time */
> +
> + while (*str++ && maxlen) {
> + nc = codepage->uni2char(uni, buf, NLS_MAX_CHARSET_SIZE);
> + if (nc > 0)
> + nbytes += nc;
> + else
> + nbytes += 1; /* for '?' */
> + i++;
> + if (i >= maxlen)
> + break;
> + }
> + *nchars = i;
Peter has a good point that we can get nchars by dividing the input
string size by 2. It might mean a few less operations but I doubt it
will be measurable.
> +
> + return nbytes;
> +}
> +
> +/*
> * UniStrncat: Concatenate length limited string
> */
> static inline wchar_t *
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 4167716..d0861d7 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -383,4 +383,6 @@ extern int CIFSSMBSetPosixACL(const int xid, struct cifsTconInfo *tcon,
> const struct nls_table *nls_codepage, int remap_special_chars);
> extern int CIFSGetExtAttr(const int xid, struct cifsTconInfo *tcon,
> const int netfid, __u64 *pExtAttrBits, __u64 *pMask);
> +extern int cifs_ucs_to_nls(char **dst, const char *src, const int maxlen,
> + int *plen, const struct nls_table *nls_codepage);
> #endif /* _CIFSPROTO_H */
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index bc09c99..ca99d35 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -81,6 +81,37 @@ static struct {
> #endif /* CONFIG_CIFS_WEAK_PW_HASH */
> #endif /* CIFS_POSIX */
>
> +
> +/*
> + * Calculates, allocates memory and converts to a NLS string.
> + * Note: caller is responsible for freeing dst if function returned 0.
> + * returns:
> + * on success - 0
> + * on failure - errno
> + */
> +int
> +cifs_ucs_to_nls(char **dst, const char *src, const int maxlen, int *plen,
> + const struct nls_table *nls_codepage)
> +{
> + size_t nbytes;
> +
> + nbytes = UniStrnlenBytes((wchar_t *)src, maxlen, plen, nls_codepage);
> + *dst = kzalloc(nbytes + 2, GFP_KERNEL);
> + if (!*dst)
> + goto err_exit;
> +
> + cifs_strfromUCS_le(*dst, (__le16 *)src, *plen, nls_codepage);
> + /* Assumes NLS string has always 1 byte NULL terminator
> + * BB Need to be fixed otherwise */
> + (*dst)[*plen] = 0;
> +
> + return 0;
> +
> +err_exit:
> + cERROR(1, ("Failed to allocate buffer for string\n"));
> + return -ENOMEM;
> +}
> +
> /* Allocates buffer into dst and copies smb string from src to it.
> * caller is responsible for freeing dst if function returned 0.
> * returns:
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 0de3b56..17a6ef4 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2667,39 +2667,29 @@ CIFSSessSetup(unsigned int xid, struct cifsSesInfo *ses,
> remaining_words =
> BCC(smb_buffer_response) / 2;
> }
> - len =
> - UniStrnlen((wchar_t *) bcc_ptr,
> - remaining_words - 1);
> -/* We look for obvious messed up bcc or strings in response so we do not go off
> - the end since (at least) WIN2K and Windows XP have a major bug in not null
> - terminating last Unicode string in response */
> - if (ses->serverOS)
> - kfree(ses->serverOS);
> - ses->serverOS = kzalloc(2 * (len + 1),
> - GFP_KERNEL);
> - if (ses->serverOS == NULL)
> + /* Win2K and Windows XP seem to have a major bug
> + * in not null terminating last Unicode string
> + * in response */
> + kfree(ses->serverOS);
> + rc = cifs_ucs_to_nls(&(ses->serverOS), bcc_ptr,
> + remaining_words - 1, &len,
> + nls_codepage);
> + if (rc)
> goto sesssetup_nomem;
> - cifs_strfromUCS_le(ses->serverOS,
> - (__le16 *)bcc_ptr,
> - len, nls_codepage);
> bcc_ptr += 2 * (len + 1);
> remaining_words -= len + 1;
> - ses->serverOS[2 * len] = 0;
> - ses->serverOS[1 + (2 * len)] = 0;
> if (remaining_words > 0) {
> - len = UniStrnlen((wchar_t *)bcc_ptr,
> - remaining_words-1);
> + /* Win2K and Windows XP seem to have a
> + * major bug in not null terminating
> + * last Unicode string in response */
> kfree(ses->serverNOS);
> - ses->serverNOS = kzalloc(2 * (len + 1),
> - GFP_KERNEL);
> - if (ses->serverNOS == NULL)
> + rc = cifs_ucs_to_nls(&(ses->serverNOS),
> + bcc_ptr,
> + remaining_words - 1,
> + &len, nls_codepage);
> + if (rc)
> goto sesssetup_nomem;
> - cifs_strfromUCS_le(ses->serverNOS,
> - (__le16 *)bcc_ptr,
> - len, nls_codepage);
> bcc_ptr += 2 * (len + 1);
> - ses->serverNOS[2 * len] = 0;
> - ses->serverNOS[1 + (2 * len)] = 0;
> if (strncmp(ses->serverNOS,
> "NT LAN Manager 4", 16) == 0) {
> cFYI(1, ("NT4 server"));
> @@ -2707,22 +2697,19 @@ CIFSSessSetup(unsigned int xid, struct cifsSesInfo *ses,
> }
> remaining_words -= len + 1;
> if (remaining_words > 0) {
> - len = UniStrnlen((wchar_t *) bcc_ptr, remaining_words);
> - /* last string is not always null terminated
> - (for e.g. for Windows XP & 2000) */
> - if (ses->serverDomain)
> - kfree(ses->serverDomain);
> - ses->serverDomain =
> - kzalloc(2*(len+1),
> - GFP_KERNEL);
> - if (ses->serverDomain == NULL)
> + /* Win2K and Windows XP seem to
> + * have a major bug in not null
> + * terminating last Unicode
> + * string in response */
> + kfree(ses->serverDomain);
> + rc = cifs_ucs_to_nls(
> + &(ses->serverDomain),
> + bcc_ptr,
> + remaining_words, &len,
> + nls_codepage);
> + if (rc)
> goto sesssetup_nomem;
> - cifs_strfromUCS_le(ses->serverDomain,
> - (__le16 *)bcc_ptr,
> - len, nls_codepage);
> bcc_ptr += 2 * (len + 1);
> - ses->serverDomain[2*len] = 0;
> - ses->serverDomain[1+(2*len)] = 0;
> } else { /* else no more room so create
> dummy domain string */
> if (ses->serverDomain)
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 5c68b42..1c50063 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -301,33 +301,32 @@ static int decode_unicode_ssetup(char **pbcc_area, int bleft,
> words_left = bleft / 2;
>
> /* save off server operating system */
> - len = UniStrnlen((wchar_t *) data, words_left);
>
> -/* We look for obvious messed up bcc or strings in response so we do not go off
> - the end since (at least) WIN2K and Windows XP have a major bug in not null
> - terminating last Unicode string in response */
> + /* Win2K and Windows XP seem to have a major bug in not null terminating
> + * last unicode string in response */
> + kfree(ses->serverOS);
> + rc = cifs_ucs_to_nls(&(ses->serverOS), data, words_left, &len, nls_cp);
> + if (rc)
> + return rc;
> +
> if (len >= words_left)
> return rc;
>
So will this code make it so that we work around this bug in win2k and
XP? Or will it return an error when it gets one of these improperly
terminated packets?
It seems like if that's a danger then before doing the string
coversion, maybe we should detect when the response isn't properly NULL
terminated and fix up the packet so that it is. Then we could just run
it through the helper routines above without having to these extra
checks. Or maybe I'm misunderstanding the problem?
> - kfree(ses->serverOS);
> - /* UTF-8 string will not grow more than four times as big as UCS-16 */
> - ses->serverOS = kzalloc((4 * len) + 2 /* trailing null */, GFP_KERNEL);
> - if (ses->serverOS != NULL)
> - cifs_strfromUCS_le(ses->serverOS, (__le16 *)data, len, nls_cp);
> data += 2 * (len + 1);
> words_left -= len + 1;
>
> /* save off server network operating system */
> - len = UniStrnlen((wchar_t *) data, words_left);
>
> - if (len >= words_left)
> + /* Win2K and Windows XP seem to have a major bug in not null terminating
> + * last unicode string in response */
> + kfree(ses->serverNOS);
> + rc = cifs_ucs_to_nls(&(ses->serverNOS), data, words_left, &len, nls_cp);
> + if (rc)
> return rc;
>
> - kfree(ses->serverNOS);
> - ses->serverNOS = kzalloc((4 * len) + 2 /* trailing null */, GFP_KERNEL);
> + if (len >= words_left)
> + return rc;
> if (ses->serverNOS != NULL) {
> - cifs_strfromUCS_le(ses->serverNOS, (__le16 *)data, len,
> - nls_cp);
> if (strncmp(ses->serverNOS, "NT LAN Manager 4", 16) == 0) {
> cFYI(1, ("NT4 server"));
> ses->flags |= CIFS_SES_NT4;
> @@ -337,19 +336,18 @@ static int decode_unicode_ssetup(char **pbcc_area, int bleft,
> words_left -= len + 1;
>
> /* save off server domain */
> - len = UniStrnlen((wchar_t *) data, words_left);
> +
> + /* Win2K and Windows XP seem to have a major bug in not null terminating
> + * last unicode string in response */
> + kfree(ses->serverDomain);
> + rc = cifs_ucs_to_nls(&(ses->serverDomain), data, words_left, &len,
> + nls_cp);
> + if (rc)
> + return rc;
>
> if (len > words_left)
> return rc;
>
> - kfree(ses->serverDomain);
> - ses->serverDomain = kzalloc(2 * (len + 1), GFP_KERNEL); /* BB FIXME wrong length */
> - if (ses->serverDomain != NULL) {
> - cifs_strfromUCS_le(ses->serverDomain, (__le16 *)data, len,
> - nls_cp);
> - ses->serverDomain[2*len] = 0;
> - ses->serverDomain[(2*len) + 1] = 0;
> - }
> data += 2 * (len + 1);
> words_left -= len + 1;
>
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list