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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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.

-- 

Daniel De Graaf
National Security Agency

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