xenstore: Fix deadlock in xs_read_watch

When read_message returns -1 while a read is pending, an attempt is made to lock h->request_mutex which is already locked by the reader. Change the read thread to only exit on thread cancellation, where read_thr_exists will return 0.

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

diff -r 6bebaf40e925 tools/xenstore/xs.c

tools/xenstore/xs.c +1 -0

@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}

Quoted 21 lines · Daniel De Graaf

When read_message returns -1 while a read is pending, an attempt is made to lock h->request_mutex which is already locked by the reader. Change the read thread to only exit on thread cancellation, where read_thr_exists will return 0.

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

diff -r 6bebaf40e925 tools/xenstore/xs.c

tools/xenstore/xs.c +1 -0

@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}

@@ -667,7 +668,7 @@
 	mutex_lock(&h->watch_mutex);
 

tools/xenstore/xs.c line 674

Quoted 6 lines

tools/xenstore/xs.c +1 -1

 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
+	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
 		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
 	if (!read_thread_exists(h)) {
 		mutex_unlock(&h->watch_mutex);

read_from_thread is not declared in this function

Quoted 22 lines · Stefano Stabellini

Quoted 21 lines · Daniel De Graaf

When read_message returns -1 while a read is pending, an attempt is made to lock h->request_mutex which is already locked by the reader. Change the read thread to only exit on thread cancellation, where read_thr_exists will return 0.

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

diff -r 6bebaf40e925 tools/xenstore/xs.c

tools/xenstore/xs.c +1 -0

@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}

@@ -667,7 +668,7 @@
 	mutex_lock(&h->watch_mutex);
 

Quoted 9 lines

tools/xenstore/xs.c line 680

Quoted 6 lines

tools/xenstore/xs.c +1 -1

 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
+	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
 		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
 	if (!read_thread_exists(h)) {
 		mutex_unlock(&h->watch_mutex);

read_from_thread is not declared in this function

Sorry about that, looks like I picked the wrong patch to send. Corrected patch attached.

Quoted 33 lines · Daniel De Graaf

Quoted 22 lines · Stefano Stabellini

Quoted 21 lines · Daniel De Graaf

When read_message returns -1 while a read is pending, an attempt is made to lock h->request_mutex which is already locked by the reader. Change the read thread to only exit on thread cancellation, where read_thr_exists will return 0.

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

diff -r 6bebaf40e925 tools/xenstore/xs.c

tools/xenstore/xs.c +1 -0

@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}

@@ -667,7 +668,7 @@
 	mutex_lock(&h->watch_mutex);
 

Quoted 9 lines

tools/xenstore/xs.c line 680

Quoted 6 lines

tools/xenstore/xs.c +1 -1

 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
+	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
 		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
 	if (!read_thread_exists(h)) {
 		mutex_unlock(&h->watch_mutex);

read_from_thread is not declared in this function

Quoted 3 lines

Sorry about that, looks like I picked the wrong patch to send. Corrected patch attached.

According to the description you gave, this patch fixes a problem occurring when read_message returns -1 while a read is pending in read_thread that is relevant only when USE_PTHREAD.
Why are you changing a code path that is only compiled when !USE_PTHREAD?
Also shouldn't you be checking the return value of read_message anyway? Considering that h->read_thr_exists is set to zero only on xs_daemon_close, and in that case pthread_cancel is called right after, I doubt that any of the operation run in read_thread after the loop will be executed.