Re: Re: [PATCH] cifs: add helper to simplify unicode to NLS conversion and use it (try #2)

Jeff Layton wrote:
> On Mon, 13 Apr 2009 15:13:16 +0530
> Suresh Jayaraman <[email protected]> wrote:

>> 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 <[email protected]>
>> ---
>>
>>  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.

Did you mean output nbytes by 2?

As Peter pointed out, unneeded stack var uni can be removed. Other than
that, do you see any other changes/improvements?

>> 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?

I think so, perhaps Steve is the right person to ask, Steve?

> 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?

My comment was based on the earlier comment and I have not seen
experienced this issue. I also need to understand this better.

Thanks,

-- 
Suresh Jayaraman