Re: Re: [PATCH 00/12] cifs: fix string conversions from wire format to local charset (try #3)

Jeff Layton wrote:
> On Wed, 29 Apr 2009 15:44:37 -0500
> Steve French <[email protected]> wrote:
> 
>> On Wed, Apr 29, 2009 at 3:24 PM, Jeff Layton <[email protected]> wrote:
>>> This is the third set of patches for this problem. The main changes
>>> from the last set are some minor renames of some of the functions,
>>> some fleshing out of comments and a fix for an endianness problem
>>> that the earlier set introduced to CIFSTCon.
>>>
>>> This patchset is intended to fix the string conversions in cifs that
>>> convert strings from "wire-format" (little-endian UTF-16) to whatever
>>> character set is in use on the client.
>>>
>>> This patch is built upon work by Suresh Jayaraman. This shouldn't be
>> Good job on these (and also to Suresh and others at Novell for tracking
>> some of the initial problems and proposing the first set of changes).
>> Via IRC and email I made various cosmetic suggestions to the patch
>> (and one correction) which Jeff incorporated in the latest patches.   As
>> we discussed via IRC, all but patches 9 and 11 (which are not fixes to
>> string conversions) can go in the tree as soon as we get an ACK from Suresh
>> (or unless we get a NACK from mailing list feedback).
>>
> 
> Thanks. I've just sent a new set that incorporates Suresh's comments
> too. I think this final set is ready to be committed...
> 
> On the matter of patches 9 and 11, Suresh is correct that you'll need to
> fix up those callers if you don't take them. I'm a little confused as
> to why you don't want to just take those patches.
> 
> The code that patch 9 removes is really not usable today, AFAICT. It
> needs endianness fixes and (most importantly) to be tested before it
> should be hooked up to anything. I'm not opposed to the concept, but
> this code should never have been added to the tree before it was ready
> to be used.

I would also want to see patches 9 and 11 merged mainly because the cifs
code would look lot less confusing without them and they are currently
unusable. I think they could be part of some testing branch/branch
linux-next if there are strong reasons for keep them and we forsee some
future developments/changes.

> On patch 11, you had at an earlier time agreed that the existing NTLMSSP
> code was a mess and that it should be removed and rewritten. The
> argument for keeping it around was that you were worried that someone
> might be using it and it would break them.
> 
> So far, I've been *completely* unable to make that code work. Due to
> how CIFSSMBNegotiate works, I don't think that code is reachable.
> Removing it can't break anyone. What's the argument for keeping it?
> 
> It's not like this code won't live on in git forever. If you feel the
> need to salvage this code at a later date, you can always revert the
> patches.
> 
>> I don't know whether this should go in stable or not.  It is rather large
>> and would have trouble merging if we went back before 2.6.29 but
>> I don't object given the possible effect of an incorrect conversion
>> overrun.
>>
> 
> I'm leaning against stable for this. It's a pretty big set.

> We had some earlier patches that fixed most, if not all of the places
> where the destination buffer could be overrun. I consider this code
> to mainly be cleanup and to ensure that we don't walk past the end of
> the source buffer.

Yes, except for a few that fixes potential buffer overruns due to
obviously less buffer sizes, others need not go, I think.

Thanks,

-- 
Suresh Jayaraman