From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeremy Fitzhardinge <jeremy@goop.org>
Subject: [PATCH 1 of 2] xs: make sure mutexes are cleaned up and
 memory freed if the read thread is cancelled
Date: Tue, 11 May 2010 15:02:54 -0700
Message-ID: <4BE9D40E.4010700@goop.org>
References: <4BE9D3B9.2050206@goop.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <4BE9D3B9.2050206@goop.org>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Keir Fraser <keir.fraser@eu.citrix.com>
Cc: Xen-devel <xen-devel@lists.xensource.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

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@citrix.com>

diff -r d77a88f938c6 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Tue May 11 14:05:28 2010 +0100
+++ b/tools/xenstore/xs.c	Tue May 11 14:55:14 2010 -0700
@@ -85,6 +85,8 @@
 #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 @@
 #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
 
@@ -262,7 +266,6 @@
 
 #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);
 	}
@@ -860,44 +863,53 @@
 {
 	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);
@@ -906,14 +918,16 @@
 		mutex_unlock(&h->reply_mutex);
 	}
 
-	return 0;
+	ret = 0;
 
- error:
-	saved_errno = errno;
-	free(msg);
-	free(body);
+error_freebody:
+	cleanup_pop(ret == -1);
+error_freemsg:
+	cleanup_pop(ret == -1);
+error:
 	errno = saved_errno;
-	return -1;
+
+	return ret;
 }
 
 #ifdef USE_PTHREAD