More s3compat-for-review patches

[Andrew Bartlett]

On Thu, 2010-05-27 at 08:23 +0200, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> > On Wed, 2010-05-26 at 11:45 +1000, Andrew Bartlett wrote:
> >> On Wed, 2010-05-26 at 09:54 +1000, Andrew Bartlett wrote:
> >>> I've updated my s3compat-for-review tree, which now includes the use of
> >>> the source3/ ntlmssp_sign.s and ntlmssp.h in common code.
> >>>
> >>> I would appreciate any useful comments that folks have, as I would like
> >>> to merge this code.  Most of the patches should be able to be
> >>> cherry-picked on their own, if only some are acceptable.
> >>>
> >>> http://gitweb.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/s3compat-for-review
> >>
> >> I've updated this branch following feedback on IRC from simo.  (A number
> >> of the patches to move functions into different files have been split
> >> into multiple patches). 
> >>
> >> It passes 'make selftest' with the same errors as master.
> > 
> > If I don't get any further comments (and I believe I've addressed simo's
> > concerns about the file split-up being taken any further), I intend to
> > push this as-is at the end of this week (tomorrow). 
> 
> Please don't push it as is.
> 
> I reviewed it yesterday via gitweb on my cellphone (in readonly mode:-)
> 
> I'd like to review it more detailed today, as I found some minor details
> I'd like to be fixed before it's pushed to master.
> 
> The first things I noticed are:
> - we don't need a ntlmssp_private.h (at least in this patch stream).

Yes and no.  There is no reason ntlmssp_debug_flags needs to be used
outside the NTLMSSP code, but must be shared between the sign and main
code.  

I agree the other changes are not strictly required, but as you may have
guessed or seen in my s3compat branch, I need those for the way GENSEC
uses the common code.  I don't see how they do any harm. 

> - for "s3:auth Make AUTH_NTLMSSP_STATE a private structure"
>   the change to source3/auth/auth_util.c is unrelated and changes
>   the behavior. It should be its own commit.

Indeed!  Sorry about that. 

>   Would it make sense to handle the case where
>   auth_ntlmssp_server_info() is called twice without segfaulting?

I don't think so - we would have to resort to talloc references and hope
that the code that it calls handles that well.  In any case, I wanted to
be very careful to preserve the existing behaviour, because of all the
warnings I've been given not to change behaviour. 

> - for "ntlmssp Make the ntlmssp.h from source3/ a common header"
>   the change to source3/Makefile.in is completely unrelated.

Again, sorry. 

> - for "s3:idmap Use idmap.idl defined structures and constants"
>   I'd like to get the ack from Michael as he's currently working on
>   idmap changes.
>   But for sure deleting source3/librpc/gen_ndr/README is wrong there.

Yeah.  I get in trouble here because Samba3 builds in
source3/librpc/gen_ndr/ while s3compat builds in
source4/bin/default/source3/librpc/gen_ndr.  I do builds of Samba3 in my
tree to check it's all OK, and I have to remove the generated files
before I go back to a waf build, or else it won't rebuild them. 

Is there a good way to ensure that we wipe these conflicting files other
than the clearly risky rm *?  I guess I'll try a 'make clean' in the
source3 autoconf build and see if that does the trick. 

> I like this commits:
> "s3:winbind tidy up connecting the winbind sockets."
> "s3:winbind Avoid use-after-free in s3compat wrapper of winbindd"
> but we should defer the code moving in winbindd to happen after this
> changes.

Thankyou for taking the time to review this, I do really appreciate it. 

I'm sorry this patch set was so under-prepared.  I'll try and do a
better job of it in future. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100527/6eca831a/attachment.pgp>