PATCH: Bug 10284
Volker Lendecke
Volker.Lendecke at SerNet.DE
Tue Nov 26 05:30:21 MST 2013
On Fri, Nov 22, 2013 at 09:41:21AM +0100, Volker Lendecke wrote:
> Hi!
>
> Attached find a patchset to fix $SUBJECT.
>
> Please review & push!
Anybody? The bug reporter confirms that it does not make
things significantly worse, and it improves the behaviour
under the new test that comes with the patch set.
Volker
>
> 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
> 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
>
--
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
More information about the samba-technical
mailing list