Segfault in smbd: messaging_dgm_fde_active

[Stefan Metzmacher]

Hi,

> Ping!
> 
> I know it is the crunch before the release but it would be good not to
> forget this.

I looked at it and found the problem.

With auth_winbind as AD DC, smbd creates two temporary
imessaging_context structures, both using messaging_dgm_ref()
internally.

Then we free both of them when the authentication is done.
So 'next' becomes a stale pointer in msg_dgm_ref_recv().

The attached patch fixes this. I'm currently running
a private autobuild with it.

Please review and push:-)

metze
-------------- next part --------------
From b4a5833fbd8a804822f2934f397d818302870a51 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Mon, 9 Jul 2018 12:33:34 +0200
Subject: [PATCH] s3:messages: make the loop in msg_dgm_ref_recv() more robust
 against stale pointers

The interaction between msg_dgm_ref_recv() and msg_dgm_ref_destructor()
doesn't allow two references from messaging_dgm_ref() to be free'd
during the loop in msg_dgm_ref_recv().

In addition to the global 'refs' list, we also need to
have a global 'next_ref' pointer, which can be cleared in
msg_dgm_ref_destructor().

As AD DC we hit this when using irpc in auth_winbind,
which uses imessaging_client_init().
In addition to the main messaging_dgm_ref() in smbd,
source3/auth/auth_samba4.c: prepare_gensec() and
make_auth4_context_s4() also generate a temporary
imessaging_context for auth_context->msg_ctx from within
auth_generic_prepare().

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13514

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/lib/messages_dgm_ref.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/source3/lib/messages_dgm_ref.c b/source3/lib/messages_dgm_ref.c
index 39d22700740c..4cf80a789987 100644
--- a/source3/lib/messages_dgm_ref.c
+++ b/source3/lib/messages_dgm_ref.c
@@ -35,6 +35,7 @@ struct msg_dgm_ref {
 
 static pid_t dgm_pid = 0;
 static struct msg_dgm_ref *refs = NULL;
+static struct msg_dgm_ref *next_ref = NULL;
 
 static int msg_dgm_ref_destructor(struct msg_dgm_ref *r);
 static void msg_dgm_ref_recv(struct tevent_context *ev,
@@ -121,16 +122,16 @@ static void msg_dgm_ref_recv(struct tevent_context *ev,
 			     const uint8_t *msg, size_t msg_len,
 			     int *fds, size_t num_fds, void *private_data)
 {
-	struct msg_dgm_ref *r, *next;
+	struct msg_dgm_ref *r;
 
 	/*
 	 * We have to broadcast incoming messages to all refs. The first ref
 	 * that grabs the fd's will get them.
 	 */
-	for (r = refs; r != NULL; r = next) {
+	for (r = refs; r != NULL; r = next_ref) {
 		bool active;
 
-		next = r->next;
+		next_ref = r->next;
 
 		active = messaging_dgm_fde_active(r->fde);
 		if (!active) {
@@ -152,6 +153,10 @@ static int msg_dgm_ref_destructor(struct msg_dgm_ref *r)
 	}
 	DLIST_REMOVE(refs, r);
 
+	if (r == next_ref) {
+		next_ref = NULL;
+	}
+
 	TALLOC_FREE(r->fde);
 
 	DBG_DEBUG("refs=%p\n", refs);
-- 
2.17.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180709/2804e7db/signature.sig>