[PATCH] Fix configuration reload in s3/loadparm

[Volker Lendecke]

Cancelled the autobuild.

Volker

On Wed, Nov 22, 2017 at 02:17:05PM +0100, Swen Schillig wrote:
> On Wed, 2017-11-22 at 13:21 +0100, Volker Lendecke via samba-technical
> wrote:
> > On Wed, Nov 22, 2017 at 12:46:03PM +0100, Ralph Böhme via samba-
> > technical wrote:
> > > Attached patchset fixes reloading config in s3/loadparm. This
> > > popped up when
> > > a user went through the following config changes:
> > > 
> > > 1. Set [global] smb encrypt = required
> > > 2. Restart Samba
> > > 3. Remove the line smb encrypt = required
> > > 4. smbcontrol smbd reload-config
> > > 
> > > Expected result: unencryptd access allowed.
> > > Actual result: unencryptd access denied.
> > > 
> > > Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
> > > 
> > > Took me some time to figure out, that this happens because in
> > > s3/loadparm we
> > > have the defaults in the sDefault struct and any service option
> > > appearing in the
> > > global section modifies it. As we have no copy of the original
> > > value, if the
> > > user removes the config setting and reloads, the original value
> > > can't be
> > > restored and will still be what was initially set. *ouch*
> > > 
> > > It looks like we have the same problem in the toplevel loadparm,
> > > but afacit all
> > > the stuff that uses it doesn't support configuration reload, so I'm
> > > leaving that
> > > as is.
> > > 
> > > Please review & push if happy. Thanks!
> > 
> > Happy. Pushed. Thanks, Volker
> > 
> Volker, Ralph
> 
> I'm new to the project so please excuse me if the following is total rubbish.
> I was wondering if the code shouldn't talloc_free(lp_ctx) in case of an error for lp_ctx->sDefault = taloc_zero(...)
> So like this
> 
> +	lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
> +	if (lp_ctx->sDefault == NULL) {
> +		DBG_ERR("talloc_zero failed\n");
> +		TALLOC_FREE(lp_ctx);
> +		return NULL;
> +	}
> 
> I'm not concerned about the first 2 caller lp_do_parameter(), lp_set_cmdline()
> where TALLOC_FREE() is called if NULL is returned by setup_lp_context(),
> but I'm not really sure about the next two and especially about dump_a_parameter()
> where the memory for lp_ctx might sit there for ages.
> 
> Cheers Swen
> 

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de