Attachment 12954 Details for Bug 12580 – Patches for v4-5

[patch] Patches for v4-5

BZ12580-v4-5.patch (text/plain), 14.98 KB, created by Amitay Isaacs on 2017-02-16 23:36:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Amitay Isaacs

Created: 2017-02-16 23:36:34 UTC

Size: 14.98 KB

patch

obsolete

>From 01fa13ea444e184ad7ec1ec28e68581e2f69d38a Mon Sep 17 00:00:00 2001
>From: Amitay Isaacs <amitay@gmail.com>
>Date: Mon, 6 Feb 2017 15:54:55 +1100
>Subject: [PATCH 1/2] ctdb-common: Fix use-after-free error in
> comm_fd_handler()
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12580
>
>comm_write_send() creates a new tevent_req and adds it to the queue
>of requests to be processed.  If this tevent_req is freed, then the
>queue entry is not removed causing use-after-free error.
>
>If the tevent_req returned by comm_write_send() is freed, then that
>request should be removed from the queue and any pending actions based
>on that request should also be removed.
>
>Signed-off-by: Amitay Isaacs <amitay@gmail.com>
>Reviewed-by: Martin Schwenke <martin@meltin.net>
>(cherry picked from commit 9db7785fc6ffbaad434ee189c0f46c488358aab5)
>---
> ctdb/common/comm.c | 46 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 6 deletions(-)
>
>diff --git a/ctdb/common/comm.c b/ctdb/common/comm.c
>index 7f370da..12f4970 100644
>--- a/ctdb/common/comm.c
>+++ b/ctdb/common/comm.c
>@@ -251,14 +251,22 @@ static void comm_read_failed(struct tevent_req *req)
>  * Write packets
>  */
> 
>+struct comm_write_entry {
>+	struct comm_context *comm;
>+	struct tevent_queue_entry *qentry;
>+	struct tevent_req *req;
>+};
>+
> struct comm_write_state {
> 	struct tevent_context *ev;
> 	struct comm_context *comm;
>+	struct comm_write_entry *entry;
> 	struct tevent_req *subreq;
> 	uint8_t *buf;
> 	size_t buflen, nwritten;
> };
> 
>+static int comm_write_entry_destructor(struct comm_write_entry *entry);
> static void comm_write_trigger(struct tevent_req *req, void *private_data);
> static void comm_write_done(struct tevent_req *subreq);
> 
>@@ -269,6 +277,7 @@ struct tevent_req *comm_write_send(TALLOC_CTX *mem_ctx,
> {
> 	struct tevent_req *req;
> 	struct comm_write_state *state;
>+	struct comm_write_entry *entry;
> 
> 	req = tevent_req_create(mem_ctx, &state, struct comm_write_state);
> 	if (req == NULL) {
>@@ -280,15 +289,38 @@ struct tevent_req *comm_write_send(TALLOC_CTX *mem_ctx,
> 	state->buf = buf;
> 	state->buflen = buflen;
> 
>-	if (!tevent_queue_add_entry(comm->queue, ev, req,
>-				    comm_write_trigger, NULL)) {
>-		talloc_free(req);
>-		return NULL;
>+	entry = talloc_zero(state, struct comm_write_entry);
>+	if (tevent_req_nomem(entry, req)) {
>+		return tevent_req_post(req, ev);
> 	}
> 
>+	entry->comm = comm;
>+	entry->req = req;
>+	entry->qentry = tevent_queue_add_entry(comm->queue, ev, req,
>+					       comm_write_trigger, NULL);
>+	if (tevent_req_nomem(entry->qentry, req)) {
>+		return tevent_req_post(req, ev);
>+	}
>+
>+	state->entry = entry;
>+	talloc_set_destructor(entry, comm_write_entry_destructor);
>+
> 	return req;
> }
> 
>+static int comm_write_entry_destructor(struct comm_write_entry *entry)
>+{
>+	struct comm_context *comm = entry->comm;
>+
>+	if (comm->write_req == entry->req) {
>+		comm->write_req = NULL;
>+		TEVENT_FD_NOT_WRITEABLE(comm->fde);
>+	}
>+
>+	TALLOC_FREE(entry->qentry);
>+	return 0;
>+}
>+
> static void comm_write_trigger(struct tevent_req *req, void *private_data)
> {
> 	struct comm_write_state *state = tevent_req_data(
>@@ -333,6 +365,8 @@ static void comm_write_done(struct tevent_req *subreq)
> 	}
> 
> 	state->nwritten = nwritten;
>+	state->entry->qentry = NULL;
>+	TALLOC_FREE(state->entry);
> 	tevent_req_done(req);
> }
> 
>@@ -382,8 +416,8 @@ static void comm_fd_handler(struct tevent_context *ev,
> 		struct comm_write_state *write_state;
> 
> 		if (comm->write_req == NULL) {
>-			/* This should never happen */
>-			abort();
>+			TEVENT_FD_NOT_WRITEABLE(comm->fde);
>+			return;
> 		}
> 
> 		write_state = tevent_req_data(comm->write_req,
>-- 
>2.9.3
>
>
>From 552316882f6f2bf83a9bcc80b98173752523f988 Mon Sep 17 00:00:00 2001
>From: Amitay Isaacs <amitay@gmail.com>
>Date: Tue, 7 Feb 2017 15:18:02 +1100
>Subject: [PATCH 2/2] ctdb-tests: Add more comm tests
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12580
>
>Signed-off-by: Amitay Isaacs <amitay@gmail.com>
>Reviewed-by: Martin Schwenke <martin@meltin.net>
>(cherry picked from commit 024a2c20d2bcdbcc43d16d492c7cd2d09b93c8f0)
>---
> ctdb/tests/cunit/comm_test_001.sh |  10 +-
> ctdb/tests/src/comm_test.c        | 309 +++++++++++++++++++++++++++++++++-----
> 2 files changed, 283 insertions(+), 36 deletions(-)
>
>diff --git a/ctdb/tests/cunit/comm_test_001.sh b/ctdb/tests/cunit/comm_test_001.sh
>index 5d20db2..ac09f5c 100755
>--- a/ctdb/tests/cunit/comm_test_001.sh
>+++ b/ctdb/tests/cunit/comm_test_001.sh
>@@ -2,6 +2,12 @@
> 
> . "${TEST_SCRIPTS_DIR}/unit.sh"
> 
>-ok "100 2048 500 4096 1024 8192 200 16384 300 32768 400 65536 1048576 "
> 
>-unit_test comm_test
>+ok_null
>+unit_test comm_test 1
>+
>+ok_null
>+unit_test comm_test 2
>+
>+ok "100 2048 500 4096 1024 8192 200 16384 300 32768 400 65536 1048576 "
>+unit_test comm_test 3
>diff --git a/ctdb/tests/src/comm_test.c b/ctdb/tests/src/comm_test.c
>index 2189435..5e1d694 100644
>--- a/ctdb/tests/src/comm_test.c
>+++ b/ctdb/tests/src/comm_test.c
>@@ -26,7 +26,218 @@
> #include "common/pkt_write.c"
> #include "common/comm.c"
> 
>-static void dead_handler(void *private_data)
>+/*
>+ * Test read_handler and dead_handler
>+ */
>+
>+static void test1_read_handler(uint8_t *buf, size_t buflen,
>+			       void *private_data)
>+{
>+	int *result = (int *)private_data;
>+
>+	*result = -1;
>+}
>+
>+static void test1_dead_handler(void *private_data)
>+{
>+	int *result = (int *)private_data;
>+
>+	*result = 1;
>+}
>+
>+static void test1(void)
>+{
>+	TALLOC_CTX *mem_ctx;
>+	struct tevent_context *ev;
>+	struct comm_context *comm;
>+	int fd[2];
>+	int result = 0;
>+	uint32_t data[2];
>+	int ret;
>+	ssize_t n;
>+
>+	mem_ctx = talloc_new(NULL);
>+	assert(mem_ctx != NULL);
>+
>+	ev = tevent_context_init(mem_ctx);
>+	assert(ev != NULL);
>+
>+	ret = pipe(fd);
>+	assert(ret == 0);
>+
>+	ret = comm_setup(ev, ev, fd[0], test1_read_handler, &result,
>+			 test1_dead_handler, &result, &comm);
>+	assert(ret == 0);
>+
>+	data[0] = 2 * sizeof(uint32_t);
>+	data[1] = 0;
>+
>+	n = write(fd[1], (void *)&data, data[0]);
>+	assert(n == data[0]);
>+
>+	while (result == 0) {
>+		tevent_loop_once(ev);
>+	}
>+
>+	assert(result == -1);
>+
>+	result = 0;
>+	close(fd[1]);
>+
>+	while (result == 0) {
>+		tevent_loop_once(ev);
>+	}
>+
>+	assert(result == 1);
>+
>+	talloc_free(mem_ctx);
>+}
>+
>+/*
>+ * Test that the tevent_req returned by comm_write_send() can be free'd.
>+ */
>+
>+struct test2_state {
>+	TALLOC_CTX *mem_ctx;
>+	bool done;
>+};
>+
>+static void test2_read_handler(uint8_t *buf, size_t buflen,
>+			       void *private_data)
>+{
>+	struct test2_state *state = (struct test2_state *)private_data;
>+
>+	TALLOC_FREE(state->mem_ctx);
>+}
>+
>+static void test2_dead_handler(void *private_data)
>+{
>+	abort();
>+}
>+
>+struct test2_write_state {
>+	int count;
>+};
>+
>+static void test2_write_done(struct tevent_req *subreq);
>+
>+static struct tevent_req *test2_write_send(TALLOC_CTX *mem_ctx,
>+					   struct tevent_context *ev,
>+					   struct comm_context *comm,
>+					   uint8_t *buf, size_t buflen)
>+{
>+	struct tevent_req *req, *subreq;
>+	struct test2_write_state *state;
>+	int i;
>+
>+	req = tevent_req_create(mem_ctx, &state, struct test2_write_state);
>+	if (req == NULL) {
>+		return NULL;
>+	}
>+
>+	state->count = 0;
>+
>+	for (i=0; i<10; i++) {
>+		subreq = comm_write_send(state, ev, comm, buf, buflen);
>+		if (tevent_req_nomem(subreq, req)) {
>+			return tevent_req_post(req, ev);
>+		}
>+		tevent_req_set_callback(subreq, test2_write_done, req);
>+	}
>+
>+	return req;
>+}
>+
>+static void test2_write_done(struct tevent_req *subreq)
>+{
>+	struct tevent_req *req = tevent_req_callback_data(
>+		subreq, struct tevent_req);
>+	struct test2_write_state *state = tevent_req_data(
>+		req, struct test2_write_state);
>+	bool status;
>+	int ret;
>+
>+	status = comm_write_recv(subreq, &ret);
>+	TALLOC_FREE(subreq);
>+	if (! status) {
>+		tevent_req_error(req, ret);
>+		return;
>+	}
>+
>+	state->count += 1;
>+
>+	if (state->count == 10) {
>+		tevent_req_done(req);
>+	}
>+}
>+
>+static void test2_timer_handler(struct tevent_context *ev,
>+				struct tevent_timer *te,
>+				struct timeval cur_time,
>+				void *private_data)
>+{
>+	struct test2_state *state = (struct test2_state *)private_data;
>+
>+	state->done = true;
>+}
>+
>+static void test2(void)
>+{
>+	TALLOC_CTX *mem_ctx;
>+	struct tevent_context *ev;
>+	struct comm_context *comm_reader, *comm_writer;
>+	struct test2_state test2_state;
>+	struct tevent_req *req;
>+	struct tevent_timer *te;
>+	int fd[2];
>+	uint32_t data[2];
>+	int ret;
>+
>+	mem_ctx = talloc_new(NULL);
>+	assert(mem_ctx != NULL);
>+
>+	test2_state.mem_ctx = talloc_new(mem_ctx);
>+	assert(test2_state.mem_ctx != NULL);
>+
>+	test2_state.done = false;
>+
>+	ev = tevent_context_init(mem_ctx);
>+	assert(ev != NULL);
>+
>+	ret = pipe(fd);
>+	assert(ret == 0);
>+
>+	ret = comm_setup(ev, ev, fd[0], test2_read_handler, &test2_state,
>+			 test2_dead_handler, NULL, &comm_reader);
>+	assert(ret == 0);
>+
>+	ret = comm_setup(ev, ev, fd[1], NULL, NULL, test2_dead_handler, NULL,
>+			 &comm_writer);
>+	assert(ret == 0);
>+
>+	data[0] = 2 * sizeof(uint32_t);
>+	data[1] = 0;
>+
>+	req = test2_write_send(test2_state.mem_ctx, ev, comm_writer,
>+			       (uint8_t *)data, data[0]);
>+	assert(req != NULL);
>+
>+	te = tevent_add_timer(ev, ev, tevent_timeval_current_ofs(5,0),
>+			      test2_timer_handler, &test2_state);
>+	assert(te != NULL);
>+
>+	while (! test2_state.done) {
>+		tevent_loop_once(ev);
>+	}
>+
>+	talloc_free(mem_ctx);
>+}
>+
>+/*
>+ * Test that data is written and read correctly.
>+ */
>+
>+static void test3_dead_handler(void *private_data)
> {
> 	int dead_data = *(int *)private_data;
> 
>@@ -34,14 +245,14 @@ static void dead_handler(void *private_data)
> 
> 	if (dead_data == 1) {
> 		/* reader */
>-		printf("writer closed pipe\n");
>+		fprintf(stderr, "writer closed pipe\n");
> 	} else {
> 		/* writer */
>-		printf("reader closed pipe\n");
>+		fprintf(stderr, "reader closed pipe\n");
> 	}
> }
> 
>-struct writer_state {
>+struct test3_writer_state {
> 	struct tevent_context *ev;
> 	struct comm_context *comm;
> 	uint8_t *buf;
>@@ -49,15 +260,15 @@ struct writer_state {
> 	int count, id;
> };
> 
>-static void writer_next(struct tevent_req *subreq);
>+static void test3_writer_next(struct tevent_req *subreq);
> 
>-static struct tevent_req *writer_send(TALLOC_CTX *mem_ctx,
>-				      struct tevent_context *ev,
>-				      struct comm_context *comm,
>-				      size_t *pkt_size, int count)
>+static struct tevent_req *test3_writer_send(TALLOC_CTX *mem_ctx,
>+					    struct tevent_context *ev,
>+					    struct comm_context *comm,
>+					    size_t *pkt_size, int count)
> {
> 	struct tevent_req *req, *subreq;
>-	struct writer_state *state;
>+	struct test3_writer_state *state;
> 	size_t max_size = 0, buflen;
> 	int i;
> 
>@@ -67,7 +278,7 @@ static struct tevent_req *writer_send(TALLOC_CTX *mem_ctx,
> 		}
> 	}
> 
>-	req = tevent_req_create(mem_ctx, &state, struct writer_state);
>+	req = tevent_req_create(mem_ctx, &state, struct test3_writer_state);
> 	if (req == NULL) {
> 		return NULL;
> 	}
>@@ -95,16 +306,16 @@ static struct tevent_req *writer_send(TALLOC_CTX *mem_ctx,
> 		return tevent_req_post(req, ev);
> 	}
> 
>-	tevent_req_set_callback(subreq, writer_next, req);
>+	tevent_req_set_callback(subreq, test3_writer_next, req);
> 	return req;
> }
> 
>-static void writer_next(struct tevent_req *subreq)
>+static void test3_writer_next(struct tevent_req *subreq)
> {
> 	struct tevent_req *req = tevent_req_callback_data(
> 		subreq, struct tevent_req);
>-	struct writer_state *state = tevent_req_data(
>-		req, struct writer_state);
>+	struct test3_writer_state *state = tevent_req_data(
>+		req, struct test3_writer_state);
> 	bool ret;
> 	int err;
> 	size_t buflen;
>@@ -130,10 +341,10 @@ static void writer_next(struct tevent_req *subreq)
> 		return;
> 	}
> 
>-	tevent_req_set_callback(subreq, writer_next, req);
>+	tevent_req_set_callback(subreq, test3_writer_next, req);
> }
> 
>-static void writer_recv(struct tevent_req *req, int *perr)
>+static void test3_writer_recv(struct tevent_req *req, int *perr)
> {
> 	if (tevent_req_is_unix_error(req, perr)) {
> 		return;
>@@ -141,7 +352,7 @@ static void writer_recv(struct tevent_req *req, int *perr)
> 	*perr = 0;
> }
> 
>-static void writer(int fd, size_t *pkt_size, int count)
>+static void test3_writer(int fd, size_t *pkt_size, int count)
> {
> 	TALLOC_CTX *mem_ctx;
> 	struct tevent_context *ev;
>@@ -157,31 +368,32 @@ static void writer(int fd, size_t *pkt_size, int count)
> 	assert(ev != NULL);
> 
> 	err = comm_setup(mem_ctx, ev, fd, NULL, NULL,
>-			 dead_handler, &dead_data, &comm);
>+			 test3_dead_handler, &dead_data, &comm);
> 	assert(err == 0);
> 	assert(comm != NULL);
> 
>-	req = writer_send(mem_ctx, ev, comm, pkt_size, count);
>+	req = test3_writer_send(mem_ctx, ev, comm, pkt_size, count);
> 	assert(req != NULL);
> 
> 	tevent_req_poll(req, ev);
> 
>-	writer_recv(req, &err);
>+	test3_writer_recv(req, &err);
> 	assert(err == 0);
> 
> 	talloc_free(mem_ctx);
> }
> 
>-struct reader_state {
>+struct test3_reader_state {
> 	size_t *pkt_size;
> 	int count, received;
> 	bool done;
> };
> 
>-static void reader_handler(uint8_t *buf, size_t buflen, void *private_data)
>+static void test3_reader_handler(uint8_t *buf, size_t buflen,
>+				 void *private_data)
> {
>-	struct reader_state *state = talloc_get_type_abort(
>-		private_data, struct reader_state);
>+	struct test3_reader_state *state = talloc_get_type_abort(
>+		private_data, struct test3_reader_state);
> 
> 	assert(buflen == state->pkt_size[state->received]);
> 	printf("%zi ", buflen);
>@@ -193,12 +405,12 @@ static void reader_handler(uint8_t *buf, size_t buflen, void *private_data)
> 	}
> }
> 
>-static void reader(int fd, size_t *pkt_size, int count)
>+static void test3_reader(int fd, size_t *pkt_size, int count)
> {
> 	TALLOC_CTX *mem_ctx;
> 	struct tevent_context *ev;
> 	struct comm_context *comm;
>-	struct reader_state *state;
>+	struct test3_reader_state *state;
> 	int dead_data = 1;
> 	int err;
> 
>@@ -208,7 +420,7 @@ static void reader(int fd, size_t *pkt_size, int count)
> 	ev = tevent_context_init(mem_ctx);
> 	assert(ev != NULL);
> 
>-	state = talloc_zero(mem_ctx, struct reader_state);
>+	state = talloc_zero(mem_ctx, struct test3_reader_state);
> 	assert(state != NULL);
> 
> 	state->pkt_size = pkt_size;
>@@ -216,8 +428,8 @@ static void reader(int fd, size_t *pkt_size, int count)
> 	state->received = 0;
> 	state->done = false;
> 
>-	err = comm_setup(mem_ctx, ev, fd, reader_handler, state,
>-			 dead_handler, &dead_data, &comm);
>+	err = comm_setup(mem_ctx, ev, fd, test3_reader_handler, state,
>+			 test3_dead_handler, &dead_data, &comm);
> 	assert(err == 0);
> 	assert(comm != NULL);
> 
>@@ -228,7 +440,7 @@ static void reader(int fd, size_t *pkt_size, int count)
> 	talloc_free(mem_ctx);
> }
> 
>-int main(void)
>+static void test3(void)
> {
> 	int fd[2];
> 	int ret;
>@@ -237,7 +449,6 @@ int main(void)
> 			      200, 16384, 300, 32768, 400, 65536,
> 			      1024*1024 };
> 
>-
> 	ret = pipe(fd);
> 	assert(ret == 0);
> 
>@@ -247,14 +458,44 @@ int main(void)
> 	if (pid == 0) {
> 		/* Child process */
> 		close(fd[0]);
>-		writer(fd[1], pkt_size, 13);
>+		test3_writer(fd[1], pkt_size, 13);
> 		close(fd[1]);
> 		exit(0);
> 	}
> 
> 	close(fd[1]);
>-	reader(fd[0], pkt_size, 13);
>+	test3_reader(fd[0], pkt_size, 13);
> 	close(fd[0]);
>+}
>+
>+
>+int main(int argc, const char **argv)
>+{
>+	int num;
>+
>+	if (argc != 2) {
>+		fprintf(stderr, "%s <testnum>\n", argv[0]);
>+		exit(1);
>+	}
>+
>+	num = atoi(argv[1]);
>+
>+	switch (num) {
>+	case 1:
>+		test1();
>+		break;
>+
>+	case 2:
>+		test2();
>+		break;
>+
>+	case 3:
>+		test3();
>+		break;
>+
>+	default:
>+		fprintf(stderr, "Unknown test number %s\n", argv[1]);
>+	}
> 
> 	return 0;
> }
>-- 
>2.9.3
>

Flags:

martins: review+

Actions: View

Attachments on bug 12580: 12952 | 12954 | 12955