[PATCH] Various source3 fixes
Andrew Bartlett
abartlet at samba.org
Tue Apr 1 18:26:05 MDT 2014
On Tue, 2014-04-01 at 12:27 +0200, David Disseldorp wrote:
> On Tue, 01 Apr 2014 11:39:20 +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.
>
> Looks like emacs offers a show-trailing-whitespace variable:
> https://www.gnu.org/software/emacs/manual/html_node/emacs/Useless-Whitespace.html
>
> > > 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()?
>
> If the event loop were to be moved out from its current nesting, then
> the code-path would need to be converted back to avoid stackframe use.
> This change isn't fixing anything, and IMO doesn't make the code any
> more readable.
I've dropped that change.
> > 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?
>
> It's not a failure case...
> 588 if (!NT_STATUS_IS_OK(status)) {
> 589 return status;
> 590 }
> 591 if (!NT_STATUS_IS_OK(result)) {
> 592 return result;
> 593 }
> 594
> 595 num_groups = rid_array->count;
> 596 if (num_groups == 0) {
> 597 return result;
> 598 }
Ah, now I see what you mean. I was confused because I didn't
intentionally change the old (incorrect) behaviour. See improved
patches attached.
> > > 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()).
>
> I share all of these objections.
>
> > > - 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.
>
> It'd be great if we didn't need these guards against sloppy memory
> allocations, but considering bso#8449, I see what you mean.
New patches are attached, skipping those indicated as already being in
or on their way to autobuild.
Please review/push!
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: 17300 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140402/7faeeea0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-auth_samba4-Fix-auth_samba4-to-correctly-provide-a-m.patch
Type: text/x-patch
Size: 2500 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140402/7faeeea0/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-s3-auth-Remember-to-always-free-the-talloc_stackfram.patch
Type: text/x-patch
Size: 1132 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140402/7faeeea0/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-s3-auth-Add-prototype-for-plugin-function-to-reduce-.patch
Type: text/x-patch
Size: 844 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140402/7faeeea0/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-winbindd-Ensure-we-do-not-look-at-rid_array-before-c.patch
Type: text/x-patch
Size: 1407 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140402/7faeeea0/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-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/20140402/7faeeea0/attachment-0005.bin>
More information about the samba-technical
mailing list