[PATCH] Two messaging fixes

[Volker Lendecke]

Hi!

Hopefully the comments are descriptive enough. Proper
rundown of structures is hard...

Review&push would be appreciated.

Thanks,

Volker

-- 
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
-------------- next part --------------
From a70ecb341775eaa1e8cd6d647d76f4f297549d43 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 22 Oct 2014 15:02:49 +0000
Subject: [PATCH 1/2] poll_funcs_tevent: Fix a valgrind error

The valgrind error happened in poll_funcs_tevent_handle_destructor in

  if (handle->ctx->refcount == 0)

handle->ctx was already gone at the time this destructor
was called. It happened because during messaging_init the
messaging_dgm subsystem was free'ed.  The unix_msg context and the
poll_funcs_tevent_context are children of messaging_dgm_context. How
was poll_funcs_tevent_handle_destructor still called? While working
on the new notify subsystem I've added some messaging_read_send
tevent_reqs, which register themselves with the dgm_context via
messaging_dgm_register_tevent_context. They were not gone yet. When
later these were also run down due to another talloc_free somewhere else,
this destructor referenced dead memory.

This code now protects the poll_funcs_tevent_handle against the
poll_funcs_tevent_context going away first with the loop

       for (h = ctx->handles; h != NULL; h = h->next) {
               h->ctx = NULL;
       }

in poll_funcs_tevent_context_destructor together with

       if (handle->ctx == NULL) {
               return 0;
       }

in poll_funcs_tevent_handle_destructor.

A side-effect of this code is that messaging_read_send request won't be
satisfied anymore after a reinit_after_fork kicked in. But I think this is the
right thing anyway: Every process should register its own message handlers
explicitly.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/poll_funcs/poll_funcs_tevent.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/source3/lib/poll_funcs/poll_funcs_tevent.c b/source3/lib/poll_funcs/poll_funcs_tevent.c
index ee800ba..565cdaf 100644
--- a/source3/lib/poll_funcs/poll_funcs_tevent.c
+++ b/source3/lib/poll_funcs/poll_funcs_tevent.c
@@ -19,6 +19,7 @@
 #include "poll_funcs_tevent.h"
 #include "tevent.h"
 #include "system/select.h"
+#include "dlinklist.h"
 
 /*
  * A poll_watch is asked for by the engine using this library via
@@ -53,7 +54,7 @@ struct poll_funcs_state {
 };
 
 struct poll_funcs_tevent_context {
-	unsigned refcount;
+	struct poll_funcs_tevent_handle *handles;
 	struct poll_funcs_state *state;
 	unsigned slot;		/* index into state->contexts[] */
 	struct tevent_context *ev;
@@ -67,6 +68,7 @@ struct poll_funcs_tevent_context {
  * multiple times. So we have to share one poll_funcs_tevent_context.
  */
 struct poll_funcs_tevent_handle {
+	struct poll_funcs_tevent_handle *prev, *next;
 	struct poll_funcs_tevent_context *ctx;
 };
 
@@ -352,7 +354,7 @@ static struct poll_funcs_tevent_context *poll_funcs_tevent_context_new(
 		return NULL;
 	}
 
-	ctx->refcount = 0;
+	ctx->handles = NULL;
 	ctx->state = state;
 	ctx->ev = ev;
 	ctx->slot = slot;
@@ -385,7 +387,14 @@ fail:
 static int poll_funcs_tevent_context_destructor(
 	struct poll_funcs_tevent_context *ctx)
 {
+	struct poll_funcs_tevent_handle *h;
+
 	ctx->state->contexts[ctx->slot] = NULL;
+
+	for (h = ctx->handles; h != NULL; h = h->next) {
+		h->ctx = NULL;
+	}
+
 	return 0;
 }
 
@@ -427,7 +436,7 @@ void *poll_funcs_tevent_register(TALLOC_CTX *mem_ctx, struct poll_funcs *f,
 	}
 
 	handle->ctx = state->contexts[slot];
-	handle->ctx->refcount += 1;
+	DLIST_ADD(handle->ctx->handles, handle);
 	talloc_set_destructor(handle, poll_funcs_tevent_handle_destructor);
 	return handle;
 fail:
@@ -438,14 +447,17 @@ fail:
 static int poll_funcs_tevent_handle_destructor(
 	struct poll_funcs_tevent_handle *handle)
 {
-	if (handle->ctx->refcount == 0) {
+	if (handle->ctx == NULL) {
+		return 0;
+	}
+	if (handle->ctx->handles == NULL) {
 		abort();
 	}
-	handle->ctx->refcount -= 1;
 
-	if (handle->ctx->refcount != 0) {
-		return 0;
+	DLIST_REMOVE(handle->ctx->handles, handle);
+
+	if (handle->ctx->handles == NULL) {
+		TALLOC_FREE(handle->ctx);
 	}
-	TALLOC_FREE(handle->ctx);
 	return 0;
 }
-- 
1.7.9.5


From 1a3deedca31f0531c0f811b50f49bfa1325f81c7 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 22 Oct 2014 15:11:43 +0000
Subject: [PATCH 2/2] messaging3: Fix running down a messaging_context

When you do a talloc_free(msg_ctx), existing waiters can't and don't have to
clean up behind themselves properly anymore. The msg_ctx the cleanup function
refers to is just gone.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/messages.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/source3/lib/messages.c b/source3/lib/messages.c
index aaaee52..d4c580f 100644
--- a/source3/lib/messages.c
+++ b/source3/lib/messages.c
@@ -267,7 +267,23 @@ static void messaging_recv_cb(const uint8_t *msg, size_t msg_len,
 
 static int messaging_context_destructor(struct messaging_context *ctx)
 {
+	unsigned i;
+
 	messaging_dgm_destroy();
+
+	for (i=0; i<ctx->num_new_waiters; i++) {
+		if (ctx->new_waiters[i] != NULL) {
+			tevent_req_set_cleanup_fn(ctx->new_waiters[i], NULL);
+			ctx->new_waiters[i] = NULL;
+		}
+	}
+	for (i=0; i<ctx->num_waiters; i++) {
+		if (ctx->waiters[i] != NULL) {
+			tevent_req_set_cleanup_fn(ctx->waiters[i], NULL);
+			ctx->waiters[i] = NULL;
+		}
+	}
+
 	return 0;
 }
 
-- 
1.7.9.5