[Xen-changelog] [xen-4.0-testing] xs: avoid pthread_join deadlock in xs_daemon_close

["Xen patchbot-4.0-testing" <patchbot-4.0-testing@xxxxxxxxxxxxxxxxxxx>]

# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# Date 1273742457 -3600
# Node ID 0f3b8247d88179e4b085e4cbf126d48c9197d60e
# Parent  760c8ec816adad5253ef563bc816cd93a384f3d0
xs: avoid pthread_join deadlock in xs_daemon_close

Doing a pthread_cancel and join on the reader thread while holding all
the request/reply/watch mutexes can deadlock if the thread needs to
take any of those mutexes to exit.  Kill off the reader thread before
taking any mutexes (which should be redundant if we're
single-threaded at that point).

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
xen-unstable changeset:   21354:9de69d816b11
xen-unstable date:        Wed May 12 08:49:13 2010 +0100

xs: make sure mutexes are cleaned up and memory freed if the read
thread is cancelled

If the read thread is terminated with pthread cancel, it must make
sure all memory is freed and mutexes are unlocked.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>
xen-unstable changeset:   21353:2dd3141b3e3e
xen-unstable date:        Wed May 12 08:48:14 2010 +0100
---
 tools/xenstore/xs.c |   62 +++++++++++++++++++++++++++++++---------------------
 1 files changed, 38 insertions(+), 24 deletions(-)

diff -r 760c8ec816ad -r 0f3b8247d881 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c       Thu May 13 10:16:44 2010 +0100
+++ b/tools/xenstore/xs.c       Thu May 13 10:20:57 2010 +0100
@@ -85,6 +85,8 @@ struct xs_handle {
 #define mutex_unlock(m)                pthread_mutex_unlock(m)
 #define condvar_signal(c)      pthread_cond_signal(c)
 #define condvar_wait(c,m,hnd)  pthread_cond_wait(c,m)
+#define cleanup_push(f, a)     pthread_cleanup_push((void (*)(void *))(f), 
(void *)(a))
+#define cleanup_pop(run)       pthread_cleanup_pop(run)
 
 static void *read_thread(void *arg);
 
@@ -102,6 +104,8 @@ struct xs_handle {
 #define mutex_unlock(m)                ((void)0)
 #define condvar_signal(c)      ((void)0)
 #define condvar_wait(c,m,hnd)  read_message(hnd)
+#define cleanup_push(f, a)     ((void)0)
+#define cleanup_pop(run)       ((void)0)
 
 #endif
 
@@ -227,17 +231,16 @@ void xs_daemon_close(struct xs_handle *h
 {
        struct xs_stored_msg *msg, *tmsg;
 
+#ifdef USE_PTHREAD
+       if (h->read_thr_exists) {
+               pthread_cancel(h->read_thr);
+               pthread_join(h->read_thr, NULL);
+       }
+#endif
+
        mutex_lock(&h->request_mutex);
        mutex_lock(&h->reply_mutex);
        mutex_lock(&h->watch_mutex);
-
-#ifdef USE_PTHREAD
-       if (h->read_thr_exists) {
-               /* XXX FIXME: May leak an unpublished message buffer. */
-               pthread_cancel(h->read_thr);
-               pthread_join(h->read_thr, NULL);
-       }
-#endif
 
        list_for_each_entry_safe(msg, tmsg, &h->reply_list, list) {
                free(msg->body);
@@ -846,44 +849,53 @@ static int read_message(struct xs_handle
 {
        struct xs_stored_msg *msg = NULL;
        char *body = NULL;
-       int saved_errno;
+       int saved_errno = 0;
+       int ret = -1;
 
        /* Allocate message structure and read the message header. */
        msg = malloc(sizeof(*msg));
        if (msg == NULL)
                goto error;
-       if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr)))
-               goto error;
+       cleanup_push(free, msg);
+       if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr))) { /* Cancellation 
point */
+               saved_errno = errno;
+               goto error_freemsg;
+       }
 
        /* Allocate and read the message body. */
        body = msg->body = malloc(msg->hdr.len + 1);
        if (body == NULL)
-               goto error;
-       if (!read_all(h->fd, body, msg->hdr.len))
-               goto error;
+               goto error_freemsg;
+       cleanup_push(free, body);
+       if (!read_all(h->fd, body, msg->hdr.len)) { /* Cancellation point */
+               saved_errno = errno;
+               goto error_freebody;
+       }
+
        body[msg->hdr.len] = '\0';
 
        if (msg->hdr.type == XS_WATCH_EVENT) {
                mutex_lock(&h->watch_mutex);
+               cleanup_push(pthread_mutex_unlock, &h->watch_mutex);
 
                /* Kick users out of their select() loop. */
                if (list_empty(&h->watch_list) &&
                    (h->watch_pipe[1] != -1))
-                       while (write(h->watch_pipe[1], body, 1) != 1)
+                       while (write(h->watch_pipe[1], body, 1) != 1) /* 
Cancellation point */
                                continue;
 
                list_add_tail(&msg->list, &h->watch_list);
 
                condvar_signal(&h->watch_condvar);
 
-               mutex_unlock(&h->watch_mutex);
+               cleanup_pop(1);
        } else {
                mutex_lock(&h->reply_mutex);
 
                /* There should only ever be one response pending! */
                if (!list_empty(&h->reply_list)) {
                        mutex_unlock(&h->reply_mutex);
-                       goto error;
+                       goto error_freebody;
                }
 
                list_add_tail(&msg->list, &h->reply_list);
@@ -892,14 +904,16 @@ static int read_message(struct xs_handle
                mutex_unlock(&h->reply_mutex);
        }
 
-       return 0;
-
- error:
-       saved_errno = errno;
-       free(msg);
-       free(body);
+       ret = 0;
+
+error_freebody:
+       cleanup_pop(ret == -1);
+error_freemsg:
+       cleanup_pop(ret == -1);
+error:
        errno = saved_errno;
-       return -1;
+
+       return ret;
 }
 
 #ifdef USE_PTHREAD

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog