[Xen-devel] Re: [PATCH] xenstore: correctly handle errors from read_message

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel