The branch, master has been updated
       via  5e7ae1b ctdb-scripts: Initialise CTDB_NFS_CALLOUT in statd-callout
       via  024a2c2 ctdb-tests: Add more comm tests
       via  9db7785 ctdb-common: Fix use-after-free error in comm_fd_handler()
      from  07bbd7f docs/vfs_ceph: document user_id parameter

https://git.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit 5e7ae1b1e2fa8137aaa6a2a2f446156ae61f4c84
Author: Martin Schwenke <mar...@meltin.net>
Date:   Tue Feb 14 09:04:41 2017 +1100

    ctdb-scripts: Initialise CTDB_NFS_CALLOUT in statd-callout
    
    Some configurations may set CTDB_NFS_CALLOUT to the empty string.
    They may do this if they allow a choice of NFS implementations.  In
    this case the default call-out for Linux kernel NFS should be used.
    However, statd-callout does not call nfs_callout_init() to set the
    default.  Therefore, statd-callout is unable to restart the lock
    manager, so the grace period is never entered.
    
    statd-callout must call nfs_callout_init() before trying to restart
    the lock manager.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=12589
    
    Signed-off-by: Martin Schwenke <mar...@meltin.net>
    Reviewed-by: Amitay Isaacs <ami...@gmail.com>
    
    Autobuild-User(master): Martin Schwenke <mart...@samba.org>
    Autobuild-Date(master): Thu Feb 16 09:21:03 CET 2017 on sn-devel-144

commit 024a2c20d2bcdbcc43d16d492c7cd2d09b93c8f0
Author: Amitay Isaacs <ami...@gmail.com>
Date:   Tue Feb 7 15:18:02 2017 +1100

    ctdb-tests: Add more comm tests
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=12580
    
    Signed-off-by: Amitay Isaacs <ami...@gmail.com>
    Reviewed-by: Martin Schwenke <mar...@meltin.net>

commit 9db7785fc6ffbaad434ee189c0f46c488358aab5
Author: Amitay Isaacs <ami...@gmail.com>
Date:   Mon Feb 6 15:54:55 2017 +1100

    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 <ami...@gmail.com>
    Reviewed-by: Martin Schwenke <mar...@meltin.net>

-----------------------------------------------------------------------

Summary of changes:
 ctdb/common/comm.c                |  46 +++++-
 ctdb/config/statd-callout         |   1 +
 ctdb/tests/cunit/comm_test_001.sh |  10 +-
 ctdb/tests/src/comm_test.c        | 309 +++++++++++++++++++++++++++++++++-----
 4 files changed, 324 insertions(+), 42 deletions(-)


Changeset truncated at 500 lines:

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,
diff --git a/ctdb/config/statd-callout b/ctdb/config/statd-callout
index 3f2dd39..38f847b 100755
--- a/ctdb/config/statd-callout
+++ b/ctdb/config/statd-callout
@@ -128,6 +128,7 @@ case "$1" in
        # where the lock manager will respond "strangely" immediately
        # after restarting it, which causes clients to fail to reclaim
        # their locks.
+       nfs_callout_init
        "$CTDB_NFS_CALLOUT" "stop" "nlockmgr" >/dev/null 2>&1
         sleep 2
        "$CTDB_NFS_CALLOUT" "start" "nlockmgr" >/dev/null 2>&1
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;
 


-- 
Samba Shared Repository

Reply via email to