xenstore: correctly handle errors from read_message

The return value of read_message needs to be checked in order to avoid waiting forever for a message if there is an error on the communication channel with xenstore. Currently, this is only checked if USE_PTHREAD is defined (by checking for read thread exit), and that path is prone to deadlock if request_mutex is held while waiting.

Since the failure of read_message leaves the socket in an undefined state, close the socket and force all threads waiting on a read to return.

This also fixes xs_read_watch in the case where a read thread is not running (in particular, this will happen if !USE_PTHREAD) by having it read from the communication channel in the same way as read_reply.

Signed-off-by Daniel De Graaf <[email protected]>

diff -r 3c4c3d48a835 tools/xenstore/xs.c

tools/xenstore/xs.c +20 -14

@@ -84,7 +84,7 @@
 #define mutex_lock(m)		pthread_mutex_lock(m)
 #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 condvar_wait(c,m)	pthread_cond_wait(c,m)
 #define cleanup_push(f, a)	\
     pthread_cleanup_push((void (*)(void *))(f), (void *)(a))
 /*

@@ -111,7 +111,7 @@
 #define mutex_lock(m)		((void)0)
 #define mutex_unlock(m)		((void)0)
 #define condvar_signal(c)	((void)0)
-#define condvar_wait(c,m,hnd)	read_message(hnd)
+#define condvar_wait(c,m)	((void)0)
 #define cleanup_push(f, a)	((void)0)
 #define cleanup_pop(run)	((void)0)
 #define read_thread_exists(h)	(0)

@@ -337,21 +337,20 @@
 
 	read_from_thread = read_thread_exists(h);
 
-#ifdef USE_PTHREAD
 	/* Read from comms channel ourselves if there is no reader thread. */
 	if (!read_from_thread && (read_message(h) == -1))
 		return NULL;
-#endif
 
 	mutex_lock(&h->reply_mutex);
-	while (list_empty(&h->reply_list) && (!read_from_thread || read_thread_exists(h)))
-		condvar_wait(&h->reply_condvar, &h->reply_mutex, h);
-	if (read_from_thread && !read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->reply_condvar, &h->reply_mutex);
+#endif
+	if (list_empty(&h->reply_list)) {
 		mutex_unlock(&h->reply_mutex);
 		errno = EINVAL;
 		return NULL;
 	}
-	assert(!list_empty(&h->reply_list));
 	msg = list_top(&h->reply_list, struct xs_stored_msg, list);
 	list_del(&msg->list);
 	assert(list_empty(&h->reply_list));

@@ -663,13 +662,22 @@
 	struct xs_stored_msg *msg;
 	char **ret, *strings, c = 0;
 	unsigned int num_strings, i;
+	int read_from_thread;
+
+	read_from_thread = read_thread_exists(h);
+
+	/* Read from comms channel ourselves if there is no reader thread. */
+	if (!read_from_thread && (read_message(h) == -1))
+		return NULL;
 
 	mutex_lock(&h->watch_mutex);
 
 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
-		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
-	if (!read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->watch_condvar, &h->watch_mutex);
+#endif
+	if (list_empty(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
 		return NULL;

@@ -965,21 +973,27 @@
 static void *read_thread(void *arg)
 {
 	struct xs_handle *h = arg;
+	int fd;
 
 	while (read_message(h) != -1)
 		continue;
 
-	/* Kick anyone waiting for a reply */
-	pthread_mutex_lock(&h->request_mutex);
-	h->read_thr_exists = 0;

Quoted 108 lines · Daniel De Graaf

The return value of read_message needs to be checked in order to avoid waiting forever for a message if there is an error on the communication channel with xenstore. Currently, this is only checked if USE_PTHREAD is defined (by checking for read thread exit), and that path is prone to deadlock if request_mutex is held while waiting.

Since the failure of read_message leaves the socket in an undefined state, close the socket and force all threads waiting on a read to return.

This also fixes xs_read_watch in the case where a read thread is not running (in particular, this will happen if !USE_PTHREAD) by having it read from the communication channel in the same way as read_reply.

Signed-off-by Daniel De Graaf <[email protected]>

diff -r 3c4c3d48a835 tools/xenstore/xs.c

tools/xenstore/xs.c +26 -15

@@ -84,7 +84,7 @@
 #define mutex_lock(m)		pthread_mutex_lock(m)
 #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 condvar_wait(c,m)	pthread_cond_wait(c,m)
 #define cleanup_push(f, a)	\
     pthread_cleanup_push((void (*)(void *))(f), (void *)(a))
 /*

@@ -111,7 +111,7 @@
 #define mutex_lock(m)		((void)0)
 #define mutex_unlock(m)		((void)0)
 #define condvar_signal(c)	((void)0)
-#define condvar_wait(c,m,hnd)	read_message(hnd)
+#define condvar_wait(c,m)	((void)0)
 #define cleanup_push(f, a)	((void)0)
 #define cleanup_pop(run)	((void)0)
 #define read_thread_exists(h)	(0)

@@ -337,21 +337,20 @@
 
 	read_from_thread = read_thread_exists(h);
 
-#ifdef USE_PTHREAD
 	/* Read from comms channel ourselves if there is no reader thread. */
 	if (!read_from_thread && (read_message(h) == -1))
 		return NULL;
-#endif
 
 	mutex_lock(&h->reply_mutex);
-	while (list_empty(&h->reply_list) && (!read_from_thread || read_thread_exists(h)))
-		condvar_wait(&h->reply_condvar, &h->reply_mutex, h);
-	if (read_from_thread && !read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->reply_condvar, &h->reply_mutex);
+#endif
+	if (list_empty(&h->reply_list)) {
 		mutex_unlock(&h->reply_mutex);
 		errno = EINVAL;
 		return NULL;
 	}
-	assert(!list_empty(&h->reply_list));
 	msg = list_top(&h->reply_list, struct xs_stored_msg, list);
 	list_del(&msg->list);
 	assert(list_empty(&h->reply_list));

@@ -663,13 +662,22 @@
 	struct xs_stored_msg *msg;
 	char **ret, *strings, c = 0;
 	unsigned int num_strings, i;
+	int read_from_thread;
+
+	read_from_thread = read_thread_exists(h);
+
+	/* Read from comms channel ourselves if there is no reader thread. */
+	if (!read_from_thread && (read_message(h) == -1))
+		return NULL;
 
 	mutex_lock(&h->watch_mutex);
 
 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
-		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
-	if (!read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->watch_condvar, &h->watch_mutex);
+#endif
+	if (list_empty(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
 		return NULL;

@@ -965,21 +973,27 @@
 static void *read_thread(void *arg)
 {
 	struct xs_handle *h = arg;
+	int fd;
 
 	while (read_message(h) != -1)
 		continue;
 
-	/* Kick anyone waiting for a reply */
-	pthread_mutex_lock(&h->request_mutex);
-	h->read_thr_exists = 0;
-	pthread_mutex_unlock(&h->request_mutex);
+	/* An error return from read_message leaves the socket in an undefined
+	 * state; we might have read only the header and not the message after
+	 * it, or (more commonly) the other end has closed the connection.
+	 * Since further communication is unsafe, close the socket.
+	 */
+	fd = h->fd;

tools/xenstore/xs.c line 989

Quoted 3 lines

tools/xenstore/xs.c +2 -0

+	h->fd = -1;
+	close(fd);
 

I like this patch, however shouldn't you be setting h->read_thr_exists to 0 here?
If you don't set read_thr_exists to 0 the variable becomes meaningless and you can go ahead and remove it altogether.

Quoted 109 lines · Stefano Stabellini

Quoted 108 lines · Daniel De Graaf

The return value of read_message needs to be checked in order to avoid waiting forever for a message if there is an error on the communication channel with xenstore. Currently, this is only checked if USE_PTHREAD is defined (by checking for read thread exit), and that path is prone to deadlock if request_mutex is held while waiting.

Since the failure of read_message leaves the socket in an undefined state, close the socket and force all threads waiting on a read to return.

This also fixes xs_read_watch in the case where a read thread is not running (in particular, this will happen if !USE_PTHREAD) by having it read from the communication channel in the same way as read_reply.

Signed-off-by Daniel De Graaf <[email protected]>

diff -r 3c4c3d48a835 tools/xenstore/xs.c

tools/xenstore/xs.c +26 -15

@@ -84,7 +84,7 @@
 #define mutex_lock(m)		pthread_mutex_lock(m)
 #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 condvar_wait(c,m)	pthread_cond_wait(c,m)
 #define cleanup_push(f, a)	\
     pthread_cleanup_push((void (*)(void *))(f), (void *)(a))
 /*

@@ -111,7 +111,7 @@
 #define mutex_lock(m)		((void)0)
 #define mutex_unlock(m)		((void)0)
 #define condvar_signal(c)	((void)0)
-#define condvar_wait(c,m,hnd)	read_message(hnd)
+#define condvar_wait(c,m)	((void)0)
 #define cleanup_push(f, a)	((void)0)
 #define cleanup_pop(run)	((void)0)
 #define read_thread_exists(h)	(0)

@@ -337,21 +337,20 @@
 
 	read_from_thread = read_thread_exists(h);
 
-#ifdef USE_PTHREAD
 	/* Read from comms channel ourselves if there is no reader thread. */
 	if (!read_from_thread && (read_message(h) == -1))
 		return NULL;
-#endif
 
 	mutex_lock(&h->reply_mutex);
-	while (list_empty(&h->reply_list) && (!read_from_thread || read_thread_exists(h)))
-		condvar_wait(&h->reply_condvar, &h->reply_mutex, h);
-	if (read_from_thread && !read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->reply_condvar, &h->reply_mutex);
+#endif
+	if (list_empty(&h->reply_list)) {
 		mutex_unlock(&h->reply_mutex);
 		errno = EINVAL;
 		return NULL;
 	}
-	assert(!list_empty(&h->reply_list));
 	msg = list_top(&h->reply_list, struct xs_stored_msg, list);
 	list_del(&msg->list);
 	assert(list_empty(&h->reply_list));

@@ -663,13 +662,22 @@
 	struct xs_stored_msg *msg;
 	char **ret, *strings, c = 0;
 	unsigned int num_strings, i;
+	int read_from_thread;
+
+	read_from_thread = read_thread_exists(h);
+
+	/* Read from comms channel ourselves if there is no reader thread. */
+	if (!read_from_thread && (read_message(h) == -1))
+		return NULL;
 
 	mutex_lock(&h->watch_mutex);
 
 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
-		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
-	if (!read_thread_exists(h)) {
+#ifdef USE_PTHREAD
+	while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
+		condvar_wait(&h->watch_condvar, &h->watch_mutex);
+#endif
+	if (list_empty(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
 		return NULL;

@@ -965,21 +973,27 @@
 static void *read_thread(void *arg)
 {
 	struct xs_handle *h = arg;
+	int fd;
 
 	while (read_message(h) != -1)
 		continue;
 
-	/* Kick anyone waiting for a reply */
-	pthread_mutex_lock(&h->request_mutex);
-	h->read_thr_exists = 0;
-	pthread_mutex_unlock(&h->request_mutex);
+	/* An error return from read_message leaves the socket in an undefined
+	 * state; we might have read only the header and not the message after
+	 * it, or (more commonly) the other end has closed the connection.
+	 * Since further communication is unsafe, close the socket.
+	 */
+	fd = h->fd;

Quoted 8 lines

tools/xenstore/xs.c line 989

Quoted 3 lines

tools/xenstore/xs.c +2 -0

+	h->fd = -1;
+	close(fd);
 

I like this patch, however shouldn't you be setting h->read_thr_exists to 0 here?
If you don't set read_thr_exists to 0 the variable becomes meaningless and you can go ahead and remove it altogether.

The variable exists as a way to detect if the thread has been started so that xs_daemon_close can cancel/join if needed; it starts at 0 and is set to 1 on thread creation. Since pthread_t is supposed to be opaque, we can't test it directly to see if the thread has been created. Setting it to zero within the thread will leave the thread hanging because it will never be joined and has not been detached.