[PATCH] Various source3 fixes
Andrew Bartlett
abartlet at samba.org
Mon Mar 31 22:15:53 MDT 2014
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>
More information about the samba-technical
mailing list