[PATCH] Fix configuration reload in s3/loadparm
Ralph Böhme
slow at samba.org
Wed Nov 22 13:43:17 UTC 2017
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).
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
-------------- next part --------------
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