[PATCH] Various source3 fixes

[Andrew Bartlett]

On Tue, 2014-04-01 at 11:39 +1300, Andrew Bartlett wrote:
> On Mon, 2014-03-31 at 17:45 +0200, David Disseldorp wrote:
> > Hi Andrew,
> > 
> > On Mon, 31 Mar 2014 19:35:51 +1300, Andrew Bartlett wrote:
> > 
> > > While working I've fixed up a few things in source3.  See attached or at
> > > https://git.samba.org/abartlet/samba.git/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/ad-dc-winbindd-pre
> > 
> > With these changes, I see the following selftest failures:
> > Testsuite[samba3.blackbox.smbclient_auth.plain (plugin_s4_dc)]
> > Testsuite[samba3.blackbox.smbclient_auth.plain (plugin_s4_dc) member creds]
> 
> I'll look into that.
> 
> > I have a few other comments/questions regarding the patches:
> > 
> > s3-auth: Finally change make_user_info_*() use a parent talloc context
> > - Please fix the white-space damage (and your editor)
> 
> If you can suggest changes I should make to emacs I'm happy to make
> them. 
> 
> > s3-auth: Make auth_samba4 use the talloc_stackframe() context for short-term memory
> > - what's the purpose of this change? The existing code already frees the
> >   short-lived auth4_context. auth_check_password() contains a nested
> >   event loop, so IMO using a stackframe should be avoided.
> 
> Why would a nested event loop interfere with a talloc_stackframe()?
> 
> Having created a talloc_stackframe() here, I figured we should use it.
> This memory just doesn't belong on the parent memory context, and it
> makes it clearer for the next step. 
> 
> > winbindd: Ensure we do not look at rid_array before checking if it was returned
> > - *pnum_groups should be zeroed when num_groups == 0
> 
> Why should we be filling that in in the failure case? 
> 
> > selftest: Rename wbinfo_s3 to wbinfo_simple and run against plugin_s4_dc
> > - test_wbinfo_simple.sh usage still refers to wbinfo_s3
> > - fix whitespace damage in tests array
> > 
> > libsmb: Provide a talloc_stackframe() to external users of libsmb_setget.c
> > - Where are we leaking onto talloc_tos()?.
> >   lp_set_cmdline and setup_logging() allocate onto the NULL ctx.
> 
> One of my primary objections to talloc_tos() is that any code anywhere
> in source3 can allocate memory onto this, without warning to the
> callers, on the (false) assumption that this is perfectly safe, as there
> is always a talloc_stackframe() there somewhere. 
> 
> This causes bugs like https://bugzilla.samba.org/show_bug.cgi?id=8449
> (which i'll fix or close now I've just seen a reference to it) and 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=728666
> 
> To work around this, we have to wrap each and every library call into
> Samba with talloc_stackframe() and TALLOC_FREE(frame).  (This is also my
> substantive objection to the use of talloc_tos()).
> 
> > - Can't we just fix the leak here instead?
> 
> The new case that introduced this was in the proposed loadparm patches
> (which failed make test because of it), and that lead me to check why
> this guard wasn't in place here.  In that patch series I've also put
> similar guards elsewhere in the stack for this reason. 

I've added the fix for bug #8449 and fixed a number of the issues
mentioned above.  Can you look at this again and see if between these
fixes and my exponentiations that this is now OK?

I had to drop the plugin_s4_dc winbind testing for now, I'll do more
with that later however. 

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-auth-Finally-change-make_user_info_-use-a-parent-.patch
Type: text/x-patch
Size: 17140 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-s3-auth-Make-auth_samba4-use-the-talloc_stackframe-c.patch
Type: text/x-patch
Size: 1552 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-auth_samba4-Fix-auth_samba4-to-correctly-provide-a-m.patch
Type: text/x-patch
Size: 2636 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-winbindd-Ensure-we-do-not-look-at-rid_array-before-c.patch
Type: text/x-patch
Size: 1242 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-selftest-Rename-wbinfo_s3-to-wbinfo_simple-and-reord.patch
Type: text/x-patch
Size: 6133 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-libsmb-Provide-a-talloc_stackframe-to-external-users.patch
Type: text/x-patch
Size: 2246 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-pam_smbpass-Wrap-calls-in-talloc_stackframe-to-avoid.patch
Type: text/x-patch
Size: 8827 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-libsmbclient-Wrap-more-function-calls-in-talloc_stac.patch
Type: text/x-patch
Size: 4154 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140401/5428178c/attachment-0007.bin>