[PATCH] Various source3 fixes

[David Disseldorp]

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.

> 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         }

> > 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.

Cheers, David