[PATCH] Fix configuration reload in s3/loadparm
Jeremy Allison
jra at samba.org
Mon Nov 27 21:07:28 UTC 2017
On Wed, Nov 22, 2017 at 02:43:17PM +0100, Ralph Böhme via samba-technical wrote:
>
>
> On Wed, Nov 22, 2017 at 02:31:33PM +0100, Volker Lendecke wrote:
> > On Wed, Nov 22, 2017 at 02:17:05PM +0100, Swen Schillig wrote:
> > > 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;
> > > + }
>
> good catch, thanks!
>
> > Cancelled the autobuild.
>
> Thanks! Attached is an updated patchset with the missing TALLOC_FREE(lp_ctx).
Thanks for the update. LGTM. Pushed, thanks !
Jeremy.
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> From 6a75793debf90d7f9467877bddd7763043711849 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 22 Nov 2017 11:49:57 +0100
> Subject: [PATCH 1/3] s3/loadparm: allocate a fresh sDefault object per lp_ctx
>
> This is in preperation of preventing direct access to sDefault in all
> places that currently modify it.
>
> As currently s3/loadparm is afaict not accessing lp_ctx->sDefault, but
> changes sDefault indirectly through lp_parm_ptr() this change is just a
> safety measure to prevent future breakage.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/param/loadparm.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 485d3f75b04..f09ef42dae6 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -961,7 +961,14 @@ static struct loadparm_context *setup_lp_context(TALLOC_CTX *mem_ctx)
> return NULL;
> }
>
> - lp_ctx->sDefault = &sDefault;
> + 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;
> + }
> +
> + *lp_ctx->sDefault = sDefault;
> lp_ctx->services = NULL; /* We do not want to access this directly */
> lp_ctx->bInGlobalSection = bInGlobalSection;
> lp_ctx->flags = flags_list;
> --
> 2.13.6
>
>
> From db9b859340c7b84cbd4044faea415ec631ea971d Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 21 Nov 2017 14:28:48 +0100
> Subject: [PATCH 2/3] s3/loadparm: ensure default service options are not
> changed
>
> Rename sDefault to _sDefault and make it const. sDefault is make a copy
> of _sDefault in in the initialisation function lp_load_ex().
>
> As we may end up in setup_lp_context() without going through
> lp_load_ex(), sDefault may still be uninitialized at that point, so I'm
> initializing lp_ctx->sDefault from _sDefault.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/param/loadparm.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index f09ef42dae6..cf938a60a75 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -111,7 +111,7 @@ static bool defaults_saved = false;
> static struct loadparm_global Globals;
>
> /* This is a default service used to prime a services structure */
> -static struct loadparm_service sDefault =
> +static const struct loadparm_service _sDefault =
> {
> .valid = true,
> .autoloaded = false,
> @@ -249,6 +249,12 @@ static struct loadparm_service sDefault =
> .dummy = ""
> };
>
> +/*
> + * This is a copy of the default service structure. Service options in the
> + * global section would otherwise overwrite the initial default values.
> + */
> +static struct loadparm_service sDefault;
> +
> /* local variables */
> static struct loadparm_service **ServicePtrs = NULL;
> static int iNumServices = 0;
> @@ -968,7 +974,7 @@ static struct loadparm_context *setup_lp_context(TALLOC_CTX *mem_ctx)
> return NULL;
> }
>
> - *lp_ctx->sDefault = sDefault;
> + *lp_ctx->sDefault = _sDefault;
> lp_ctx->services = NULL; /* We do not want to access this directly */
> lp_ctx->bInGlobalSection = bInGlobalSection;
> lp_ctx->flags = flags_list;
> @@ -3858,6 +3864,7 @@ static bool lp_load_ex(const char *pszFname,
> bInGlobalSection = true;
> bGlobalOnly = global_only;
> bAllowIncludeRegistry = allow_include_registry;
> + sDefault = _sDefault;
>
> lp_ctx = setup_lp_context(talloc_tos());
>
> --
> 2.13.6
>
>
> From 29313b8f2956b39bb0377f1cf0ff4a69da38523b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 21 Nov 2017 14:34:28 +0100
> Subject: [PATCH 3/3] s3/loadparm: don't mark IPC$ as autoloaded
>
> A related problem that affects configuration for the hidden IPC$
> share. This share is marked a "autoloaded" and such shares are not
> reloaded when requested. That resulted in the tcon to IPC$ still using
> encrpytion after running the following sequence of changes:
>
> 1. stop Samba
> 2. set [global] smb encrypt = required
> 3. start Samba
> 4. remove [global] smb encrypt = required
> 5. smbcontrol smbd reload-config
> 6a bin/smbclient -U slow%x //localhost/raw -c quit, or
> 6b bin/smbclient -U slow%x -mNT1 //localhost/raw -c ls
>
> In 6a the client simply encrypted packets on the IPC$ tcon. In 6b the
> client got a tcon failure with NT_STATUS_ACCESS_DENIED, but silently
> ignore the error.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/param/loadparm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index cf938a60a75..c18e9c8e9e4 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -1606,7 +1606,7 @@ static bool lp_add_ipc(const char *ipc_name, bool guest_ok)
> ServicePtrs[i]->guest_ok = guest_ok;
> ServicePtrs[i]->printable = false;
> ServicePtrs[i]->browseable = sDefault.browseable;
> - ServicePtrs[i]->autoloaded = true;
> + ServicePtrs[i]->autoloaded = false;
>
> DEBUG(3, ("adding IPC service\n"));
>
> --
> 2.13.6
>
More information about the samba-technical
mailing list