[PATCH] fix crash in smb.conf processing

[Alexander Bokovoy]

On Mon, 25 Jan 2016, Uri Simchoni wrote:
> 
> 
> On 01/25/2016 07:53 AM, Alexander Bokovoy wrote:
> >On Mon, 25 Jan 2016, Uri Simchoni wrote:
> >>On 01/22/2016 03:50 PM, Alexander Bokovoy wrote:
> >>>Hi,
> >>>
> >>>please review and push.
> >>>
> >>>The bug is pretty serious and as such is blocker for FreeIPA use of
> >>>Samba in Fedora 23 -- after commit
> >>>795c543d858b2452f062a02846c2f908fe4cffe4 smb.conf processing was
> >>>improved but nobody did test it for cases when config is re-read several
> >>>times like it happens with the registry backend for smb.conf.
> >>>
> >>>More details are in the commit message.
> >>>
> >>>If you want to test on Fedora 23, my COPR repository
> >>>https://copr.fedorainfracloud.org/coprs/abbra/samba-test/  has pre-built
> >>>packages.
> >>>
> >>It seems to me like even before commit
> >>795c543d858b2452f062a02846c2f908fe4cffe4, the default string values were
> >>dangling pointers after free of the Globals.ctx, only none used them.
> >>Wouldn't it be simpler to hang them on a different talloc context?
> >>
> >>It can be a context that either does not ever get freed, or gets freed only
> >>in lp_save_defaults() (along with  cleanup to param table, but one that does
> >>not have to ask about the ownership of the strings).
> >Since nothing else allocates/frees these, it is not worth to separate
> >the notion of contexts. We already have two contexts involved in the
> >processing of params...
> Fair enough (assuming the only use for default values, as it seems to me
> now, is by testparm utility, so having default values set to NULL is not an
> issue).
> 
> With an added BUG: reference you can add my RB.
Patch description updated.


-- 
/ Alexander Bokovoy
-------------- next part --------------
>From d28ed92117b2109f0e4aeed631ed00a0d32ba655 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <ab at samba.org>
Date: Fri, 22 Jan 2016 11:44:03 +0200
Subject: [PATCH] s3-parm: clean up defaults when removing global parameters

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11693

When globals are re-initialized, they are cleared and globals' talloc
context is freed. However, parm_table still contains a reference to the
global value in the defaults. This confuses lpcfg_string_free() after
commit 795c543d858b2452f062a02846c2f908fe4cffe4 because it tries to
free already freed pointer which is passed by lp_save_defaults():

....
    case P_STRING:
    case P_USTRING:
                  lpcfg_string_set(Globals.ctx,
                                   &parm_table[i].def.svalue,
                                   *(char **)lp_parm_ptr(NULL, &parm_table[i]));
....

here &parm_table[i].def.svalue is passed to lpcfg_string_free() but it
is a pointer to a value allocated with previous Globals.ctx which
already was freed.

This specifically affects registry backend of smb.conf in lp_load_ex()
where init_globals() called explicitly to re-init globals after
lp_save_defaults() if we have registry backend defined.

Reviewed-by: Uri Simchoni <uri at samba.org>
Signed-off-by: Alexander Bokovoy <ab at samba.org>
---
 source3/param/loadparm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 9f4a2b4..f8ecab7 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -399,8 +399,25 @@ static void free_parameters_by_snum(int snum)
  */
 static void free_global_parameters(void)
 {
+	uint32_t i;
+	struct parm_struct *parm;
+
 	free_param_opts(&Globals.param_opt);
 	free_parameters_by_snum(GLOBAL_SECTION_SNUM);
+
+	/* Reset references in the defaults because the context is going to be freed */
+	for (i=0; parm_table[i].label; i++) {
+		parm = &parm_table[i];
+		if ((parm->type == P_STRING) ||
+		    (parm->type == P_USTRING)) {
+			if ((parm->def.svalue != NULL) &&
+			    (*(parm->def.svalue) != '\0')) {
+				if (talloc_parent(parm->def.svalue) == Globals.ctx) {
+					parm->def.svalue = NULL;
+				}
+			}
+		}
+	}
 	TALLOC_FREE(Globals.ctx);
 }
 
-- 
2.5.0