[PATCH] Various source3 fixes

Andrew Bartlett abartlet at samba.org
Mon Mar 31 16:39:20 MDT 2014


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. 

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






More information about the samba-technical mailing list