tools/xenstore: libxenstore: fix threading bug which cause xend startup hang

If a multithreaded caller creates a thread which calls xs_read_watch,
before it has set any watches with xs_watch, the thread in
xs_read_watch will enter read_message and sit reading the xenstored fd
without the appropriate locks held.  Other threads can then
concurrently read the xenstored fd, which naturally does not work very
well.

Symptoms of this bug which I have been able to reproduce include
failure of xend startup to finish, due to a deadlock; results could
also include reading corrupted data from xenstore.

In this patch we arrange for xs_read_watch to always rely on the
reader thread created by xs_watch.  If no watches have been set, then
xs_read_watch will block until one has been.  If the library is
compiled non-threaded xs_read_watch unconditionally does the reading
in the current thread.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

1 files changed, 34 insertions, 10 deletions

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 9e0356f30d..112feec8a5 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -79,6 +79,20 @@ struct xs_handle {
 
 	/* One request at a time. */
 	pthread_mutex_t request_mutex;
+
+	/* Lock discipline:
+	 *  Only holder of the request lock may write to h->fd.
+	 *  Only holder of the request lock may access read_thr_exists.
+	 *  If read_thr_exists==0, only holder of request lock may read h->fd;
+	 *  If read_thr_exists==1, only the read thread may read h->fd.
+	 *  Only holder of the reply lock may access reply_list.
+	 *  Only holder of the watch lock may access watch_list.
+	 * Lock hierarchy:
+	 *  The order in which to acquire locks is
+	 *     request_mutex
+	 *     reply_mutex
+	 *     watch_mutex
+	 */
 };
 
 #define mutex_lock(m)		pthread_mutex_lock(m)
@@ -662,21 +676,27 @@ char **xs_read_watch(struct xs_handle *h, unsigned int *num)
 	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. */
 #ifdef USE_PTHREAD
-	while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
+	/* Wait on the condition variable for a watch to fire.
+	 * If the reader thread doesn't exist yet, then that's because
+	 * we haven't called xs_watch.	Presumably the application
+	 * will do so later; in the meantime we just block.
+	 */
+	while (list_empty(&h->watch_list) && h->fd != -1)
 		condvar_wait(&h->watch_condvar, &h->watch_mutex);
-#endif
+#else /* !defined(USE_PTHREAD) */
+	/* Read from comms channel ourselves if there are no threads
+	 * and therefore no reader thread. */
+
+	assert(!read_thread_exists(h)); /* not threadsafe but worth a check */
+	if ((read_message(h) == -1))
+		return NULL;
+
+#endif /* !defined(USE_PTHREAD) */
+
 	if (list_empty(&h->watch_list)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
@@ -900,6 +920,10 @@ char *xs_debug_command(struct xs_handle *h, const char *cmd,
 
 static int read_message(struct xs_handle *h)
 {
+	/* IMPORTANT: It is forbidden to call this function without
+	 * acquiring the request lock and checking that h->read_thr_exists
+	 * is false.  See "Lock discipline" in struct xs_handle, above. */
+         
 	struct xs_stored_msg *msg = NULL;
 	char *body = NULL;
 	int saved_errno = 0;