Xen devel - Aug 2010 - [PATCH] 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 <dgdegra@tycho.nsa.gov>


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

On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> 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 <dgdegra@tycho.nsa.gov>
> 
> diff -r 3c4c3d48a835 tools/xenstore/xs.c
> --- a/tools/xenstore/xs.c	Thu Aug 26 11:16:56 2010 +0100
> +++ b/tools/xenstore/xs.c	Mon Aug 30 10:56:11 2010 -0400
> @@ -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;
> +	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.


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

On 08/30/2010 01:13 PM, Stefano Stabellini wrote:
> On Mon, 30 Aug 2010, Daniel De Graaf wrote:
>> 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 <dgdegra@tycho.nsa.gov>
>>
>> diff -r 3c4c3d48a835 tools/xenstore/xs.c
>> --- a/tools/xenstore/xs.c	Thu Aug 26 11:16:56 2010 +0100
>> +++ b/tools/xenstore/xs.c	Mon Aug 30 10:56:11 2010 -0400
>> @@ -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;
>> +	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.

-- 

Daniel De Graaf
National Security Agency

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

On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> On 08/30/2010 01:13 PM, Stefano Stabellini wrote:
> > On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> >> 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 <dgdegra@tycho.nsa.gov>
> >>
> >> diff -r 3c4c3d48a835 tools/xenstore/xs.c
> >> --- a/tools/xenstore/xs.c	Thu Aug 26 11:16:56 2010 +0100
> >> +++ b/tools/xenstore/xs.c	Mon Aug 30 10:56:11 2010 -0400
> >> @@ -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;
> >> +	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.
I understand now, thanks for the explanation.
I reckon the patch is correct and I am going to apply it unless somebody
has any other comments.


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