PATCH: Bug 10284

Volker Lendecke Volker.Lendecke at SerNet.DE
Fri Nov 22 01:41:21 MST 2013


Hi!

Attached find a patchset to fix $SUBJECT.

Please review & push!

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 6fcca70fea894a2d433ac6a9e077484ff4d8fe4b Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 21 Nov 2013 21:05:29 +0100
Subject: [PATCH 1/2] smbd: Fix bug 10284

If we msg_read_send on a nonempty channel, we create one
tevent_immediate. If we directly receive another message and from
within the msg_read_send's tevent_req callback we immediately do
another msg_read_send, we end up with two tevent_immediate events for
msg_channel_trigger with just one incoming message. Test to follow.

This patch simplifies msg_channel.c by removing the explicit immediate
events. Instead, it relies on the implicit immediate event available
via tevent_req_defer_callback. For messages received from tdb with
a msg_read_send req pending, we directly finish that request without
putting the message on the queue.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=10284
Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/msg_channel.c |  100 ++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 69 deletions(-)

diff --git a/source3/lib/msg_channel.c b/source3/lib/msg_channel.c
index 625d07c..8e23fd4 100644
--- a/source3/lib/msg_channel.c
+++ b/source3/lib/msg_channel.c
@@ -41,9 +41,6 @@ static void msg_channel_init_got_ctdb(struct tevent_req *subreq);
 static void msg_channel_init_got_msg(struct messaging_context *msg,
 			       void *priv, uint32_t msg_type,
 			       struct server_id server_id, DATA_BLOB *data);
-static void msg_channel_trigger(struct tevent_context *ev,
-				struct tevent_immediate *im,
-				void *priv);
 static int msg_channel_destructor(struct msg_channel *s);
 
 struct tevent_req *msg_channel_init_send(TALLOC_CTX *mem_ctx,
@@ -157,6 +154,12 @@ fail:
 	return err;
 }
 
+struct msg_read_state {
+	struct tevent_context *ev;
+	struct msg_channel *channel;
+	struct messaging_rec *rec;
+};
+
 static void msg_channel_init_got_msg(struct messaging_context *msg,
 				     void *priv, uint32_t msg_type,
 				     struct server_id server_id,
@@ -167,7 +170,6 @@ static void msg_channel_init_got_msg(struct messaging_context *msg,
 	struct messaging_rec *rec;
 	struct messaging_rec **msgs;
 	size_t num_msgs;
-	struct tevent_immediate *im;
 
 	rec = talloc(s, struct messaging_rec);
 	if (rec == NULL) {
@@ -184,6 +186,19 @@ static void msg_channel_init_got_msg(struct messaging_context *msg,
 	}
 	rec->buf.length = data->length;
 
+	if (s->pending_req != NULL) {
+		struct tevent_req *req = s->pending_req;
+		struct msg_read_state *state = tevent_req_data(
+			req, struct msg_read_state);
+
+		s->pending_req = NULL;
+
+		state->rec = talloc_move(state, &rec);
+		tevent_req_defer_callback(req, s->ev);
+		tevent_req_done(req);
+		return;
+	}
+
 	num_msgs = talloc_array_length(s->msgs);
 	msgs = talloc_realloc(s, s->msgs, struct messaging_rec *, num_msgs+1);
 	if (msgs == NULL) {
@@ -192,28 +207,11 @@ static void msg_channel_init_got_msg(struct messaging_context *msg,
 	s->msgs = msgs;
 	s->msgs[num_msgs] = talloc_move(s->msgs, &rec);
 
-	if (s->pending_req == NULL) {
-		return;
-	}
-
-	im = tevent_create_immediate(s);
-	if (im == NULL) {
-		goto fail;
-	}
-	tevent_schedule_immediate(im, s->ev, msg_channel_trigger, s);
 	return;
 fail:
 	TALLOC_FREE(rec);
 }
 
-struct msg_read_state {
-	struct tevent_context *ev;
-	struct tevent_req *req;
-	struct msg_channel *channel;
-	struct messaging_rec *rec;
-};
-
-static int msg_read_state_destructor(struct msg_read_state *s);
 static void msg_read_got_ctdb(struct tevent_req *subreq);
 
 struct tevent_req *msg_read_send(TALLOC_CTX *mem_ctx,
@@ -221,7 +219,6 @@ struct tevent_req *msg_read_send(TALLOC_CTX *mem_ctx,
 				 struct msg_channel *channel)
 {
 	struct tevent_req *req;
-	struct tevent_immediate *im;
 	struct msg_read_state *state;
 	void *msg_tdb_event;
 	size_t num_msgs;
@@ -231,28 +228,28 @@ struct tevent_req *msg_read_send(TALLOC_CTX *mem_ctx,
 		return NULL;
 	}
 	state->ev = ev;
-	state->req = req;
 	state->channel = channel;
 
 	if (channel->pending_req != NULL) {
 		tevent_req_error(req, EBUSY);
 		return tevent_req_post(req, ev);
 	}
-	channel->pending_req = req;
-	channel->ev = ev;
-	talloc_set_destructor(state, msg_read_state_destructor);
 
 	num_msgs = talloc_array_length(channel->msgs);
 	if (num_msgs != 0) {
-		im = tevent_create_immediate(channel);
-		if (tevent_req_nomem(im, req)) {
-			return tevent_req_post(req, ev);
-		}
-		tevent_schedule_immediate(im, channel->ev, msg_channel_trigger,
-					  channel);
-		return req;
+		state->rec = talloc_move(state, &channel->msgs[0]);
+		memmove(channel->msgs, channel->msgs+1,
+			sizeof(struct messaging_rec *) * (num_msgs-1));
+		channel->msgs = talloc_realloc(
+			channel, channel->msgs, struct messaging_rec *,
+			num_msgs - 1);
+		tevent_req_done(req);
+		return tevent_req_post(req, ev);
 	}
 
+	channel->pending_req = req;
+	channel->ev = ev;
+
 	msg_tdb_event = messaging_tdb_event(state, channel->msg, ev);
 	if (tevent_req_nomem(msg_tdb_event, req)) {
 		return tevent_req_post(req, ev);
@@ -271,42 +268,6 @@ struct tevent_req *msg_read_send(TALLOC_CTX *mem_ctx,
 	return req;
 }
 
-static int msg_read_state_destructor(struct msg_read_state *s)
-{
-	assert(s->channel->pending_req == s->req);
-	s->channel->pending_req = NULL;
-	return 0;
-}
-
-static void msg_channel_trigger(struct tevent_context *ev,
-			       struct tevent_immediate *im,
-			       void *priv)
-{
-	struct msg_channel *channel;
-	struct tevent_req *req;
-	struct msg_read_state *state;
-	size_t num_msgs;
-
-	channel = talloc_get_type_abort(priv, struct msg_channel);
-	req = channel->pending_req;
-	state = tevent_req_data(req, struct msg_read_state);
-
-	talloc_set_destructor(state, NULL);
-	msg_read_state_destructor(state);
-
-	num_msgs = talloc_array_length(channel->msgs);
-	assert(num_msgs > 0);
-
-	state->rec = talloc_move(state, &channel->msgs[0]);
-
-	memmove(channel->msgs, channel->msgs+1,
-		sizeof(struct messaging_rec *) * (num_msgs-1));
-	channel->msgs = talloc_realloc(
-		channel, channel->msgs, struct messaging_rec *, num_msgs - 1);
-
-	tevent_req_done(req);
-}
-
 static void msg_read_got_ctdb(struct tevent_req *subreq)
 {
 	struct tevent_req *req = tevent_req_callback_data(
@@ -368,5 +329,6 @@ int msg_read_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
 		return err;
 	}
 	*prec = talloc_move(mem_ctx, &state->rec);
+	tevent_req_received(req);
 	return 0;
 }
-- 
1.7.9.5


From 9706cdcd300d835c9f866e82375778b57b55374d Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 21 Nov 2013 16:16:33 +0100
Subject: [PATCH 2/2] torture3: Reproducer for bug 10284

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/selftest/tests.py  |    1 +
 source3/torture/proto.h    |    1 +
 source3/torture/test_msg.c |   86 ++++++++++++++++++++++++++++++++++++++++++++
 source3/torture/torture.c  |    1 +
 4 files changed, 89 insertions(+)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 63f119f..142c70d 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -64,6 +64,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
         "CLEANUP1",
         "CLEANUP2",
         "CLEANUP4",
+        "LOCAL-MSG2",
         "BAD-NBT-SESSION"]
 
 for t in tests:
diff --git a/source3/torture/proto.h b/source3/torture/proto.h
index d430aef..b7eacdf 100644
--- a/source3/torture/proto.h
+++ b/source3/torture/proto.h
@@ -107,6 +107,7 @@ bool run_cleanup3(int dummy);
 bool run_cleanup4(int dummy);
 bool run_ctdb_conn(int dummy);
 bool run_msg_test(int dummy);
+bool run_msg_test2(int dummy);
 bool run_notify_bench2(int dummy);
 bool run_notify_bench3(int dummy);
 bool run_dbwrap_watch1(int dummy);
diff --git a/source3/torture/test_msg.c b/source3/torture/test_msg.c
index 2171598..d57379d 100644
--- a/source3/torture/test_msg.c
+++ b/source3/torture/test_msg.c
@@ -129,3 +129,89 @@ bool run_msg_test(int dummy)
 	TALLOC_FREE(ev);
 	return (ret == 0);
 }
+
+/*
+ * Reproducer for bug 10284
+ */
+
+static void msg_callback(struct tevent_req *subreq);
+
+struct msg_test2_state {
+	struct tevent_context *ev;
+	struct messaging_context *msg;
+	struct msg_channel *channel;
+	struct messaging_rec *rec;
+	struct tevent_req *req;
+};
+
+bool run_msg_test2(int dummy)
+{
+	struct msg_test2_state s;
+	NTSTATUS status;
+	int i, ret;
+
+	s.ev = samba_tevent_context_init(talloc_tos());
+	if (s.ev == NULL) {
+		fprintf(stderr, "tevent_context_init failed\n");
+		return false;
+	}
+
+	s.msg = messaging_init(s.ev, s.ev);
+	if (s.msg == NULL) {
+		fprintf(stderr, "messaging_init failed\n");
+		return false;
+	}
+
+	ret = msg_channel_init(s.ev, s.msg, MSG_PING, &s.channel);
+	if (ret != 0) {
+		fprintf(stderr, "msg_channel_init returned %s\n",
+			strerror(ret));
+		return false;
+	}
+
+	status = messaging_send(s.msg, messaging_server_id(s.msg), MSG_PING,
+				&data_blob_null);
+	if (!NT_STATUS_IS_OK(status)) {
+		fprintf(stderr, "messaging_send returned %s\n",
+			nt_errstr(status));
+		return false;
+	}
+
+	ret = tevent_loop_once(s.ev);
+	if (ret == -1) {
+		fprintf(stderr, "tevent_loop_once failed: %s\n",
+			strerror(errno));
+		return false;
+	}
+
+	s.req = msg_read_send(s.ev, s.ev, s.channel);
+	if (s.req == NULL) {
+		fprintf(stderr, "msg_read_send failed\n");
+		return false;
+	}
+	tevent_req_set_callback(s.req, msg_callback, &s);
+
+	status = messaging_send(s.msg, messaging_server_id(s.msg), MSG_PING,
+				&data_blob_null);
+	if (!NT_STATUS_IS_OK(status)) {
+		fprintf(stderr, "messaging_send returned %s\n",
+			nt_errstr(status));
+		return false;
+	}
+
+	for (i=0; i<5; i++) {
+		tevent_loop_once(s.ev);
+	}
+
+	return true;
+}
+
+static void msg_callback(struct tevent_req *subreq)
+{
+	struct msg_test2_state *s = _tevent_req_callback_data(subreq);
+	struct messaging_rec *rec;
+	msg_read_recv(subreq, NULL, &rec);
+	TALLOC_FREE(subreq);
+	subreq = msg_read_send(s->ev, s->ev, s->channel);
+	tevent_req_set_callback(subreq, msg_callback, s);
+}
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index 37a44b2..6789d85 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9573,6 +9573,7 @@ static struct {
 	{ "LOCAL-TALLOC-DICT", run_local_talloc_dict, 0},
 	{ "LOCAL-CTDB-CONN", run_ctdb_conn, 0},
 	{ "LOCAL-MSG", run_msg_test, 0},
+	{ "LOCAL-MSG2", run_msg_test2, 0},
 	{ "LOCAL-DBWRAP-WATCH1", run_dbwrap_watch1, 0 },
 	{ "LOCAL-BASE64", run_local_base64, 0},
 	{ "LOCAL-RBTREE", run_local_rbtree, 0},
-- 
1.7.9.5



More information about the samba-technical mailing list