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

[Jeff Layton]

On Wed, 29 Apr 2009 15:44:37 -0500
Steve French <smfrench at gmail.com> wrote:

> On Wed, Apr 29, 2009 at 3:24 PM, Jeff Layton <jlayton at redhat.com> 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.

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.

These patches seem more like "normal" bugfixes to me, but I can be
convinced to reconsider that if someone thinks otherwise.

-- 
Jeff Layton <jlayton at redhat.com>