Fix multi-threaded fuse session exit

Issue with previous code was that fuse_session_exit()
didn't wake up the semaphore in fuse_loop_mt.c.
Lock, semaphore and all uses of checking for "exited"
are now moved to struct fuse_session to have it
available for the signal handler.

This also removes internal fuse_session_reset() calls,
as that makes testing hard. From git history I also
don't see why it was added.

Closes: https://github.com/libfuse/libfuse/issues/997
Signed-off-by: Bernd Schubert <bschubert@ddn.com>

5 files changed, 61 insertions, 39 deletions

diff --git a/lib/fuse.c b/lib/fuse.c
index 49f5711..02ceed2 100644
--- a/lib/fuse.c
+++ b/lib/fuse.c
@@ -4595,7 +4595,6 @@ static int fuse_session_loop_remember(struct fuse *f)
 	}
 
 	free(fbuf.mem);
-	fuse_session_reset(se);
 	return res < 0 ? -1 : 0;
 }
 
diff --git a/lib/fuse_i.h b/lib/fuse_i.h
index 48b8294..1f59944 100644
--- a/lib/fuse_i.h
+++ b/lib/fuse_i.h
@@ -10,6 +10,8 @@
 #include "fuse_lowlevel.h"
 #include "util.h"
 
+#include <pthread.h>
+#include <semaphore.h>
 #include <stdint.h>
 #include <stdbool.h>
 #include <errno.h>
@@ -55,7 +57,6 @@ struct fuse_notify_req {
 
 struct fuse_session {
 	char *mountpoint;
-	volatile int exited;
 	int fd;
 	struct fuse_custom_io *io;
 	struct mount_opts *mo;
@@ -83,6 +84,11 @@ struct fuse_session {
 	 */
 	struct libfuse_version version;
 
+	/* thread synchronization */
+	_Atomic bool mt_exited;
+	pthread_mutex_t mt_lock;
+	sem_t mt_finish;
+
 	/* true if reading requests from /dev/fuse are handled internally */
 	bool buf_reallocable;
 };
diff --git a/lib/fuse_loop_mt.c b/lib/fuse_loop_mt.c
index d6be998..c13b476 100644
--- a/lib/fuse_loop_mt.c
+++ b/lib/fuse_loop_mt.c
@@ -53,14 +53,12 @@ struct fuse_worker {
 	struct fuse_mt *mt;
 };
 
+/* synchronization via se->mt_lock */
 struct fuse_mt {
-	pthread_mutex_t lock;
 	int numworker;
 	int numavail;
 	struct fuse_session *se;
 	struct fuse_worker main;
-	sem_t finish;
-	int exit;
 	int error;
 	int clone_fd;
 	int max_idle;
@@ -131,32 +129,32 @@ static void *fuse_do_work(void *data)
 {
 	struct fuse_worker *w = (struct fuse_worker *) data;
 	struct fuse_mt *mt = w->mt;
+	struct fuse_session *se = mt->se;
 
 #ifdef HAVE_PTHREAD_SETNAME_NP
 	pthread_setname_np(pthread_self(), "fuse_worker");
 #endif
 
-	while (!fuse_session_exited(mt->se)) {
+	while (!fuse_session_exited(se)) {
 		int isforget = 0;
 		int res;
 
 		pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
-		res = fuse_session_receive_buf_internal(mt->se, &w->fbuf,
-							w->ch);
+		res = fuse_session_receive_buf_internal(se, &w->fbuf, w->ch);
 		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);
 		if (res == -EINTR)
 			continue;
 		if (res <= 0) {
 			if (res < 0) {
-				fuse_session_exit(mt->se);
+				fuse_session_exit(se);
 				mt->error = res;
 			}
 			break;
 		}
 
-		pthread_mutex_lock(&mt->lock);
-		if (mt->exit) {
-			pthread_mutex_unlock(&mt->lock);
+		pthread_mutex_lock(&se->mt_lock);
+		if (fuse_session_exited(se)) {
+			pthread_mutex_unlock(&se->mt_lock);
 			return NULL;
 		}
 
@@ -176,11 +174,11 @@ static void *fuse_do_work(void *data)
 			mt->numavail--;
 		if (mt->numavail == 0 && mt->numworker < mt->max_threads)
 			fuse_loop_start_thread(mt);
-		pthread_mutex_unlock(&mt->lock);
+		pthread_mutex_unlock(&se->mt_lock);
 
-		fuse_session_process_buf_internal(mt->se, &w->fbuf, w->ch);
+		fuse_session_process_buf_internal(se, &w->fbuf, w->ch);
 
-		pthread_mutex_lock(&mt->lock);
+		pthread_mutex_lock(&se->mt_lock);
 		if (!isforget)
 			mt->numavail++;
 
@@ -191,14 +189,14 @@ static void *fuse_do_work(void *data)
 		 * delayed, a moving average might be useful for that.
 		 */
 		if (mt->max_idle != -1 && mt->numavail > mt->max_idle && mt->numworker > 1) {
-			if (mt->exit) {
-				pthread_mutex_unlock(&mt->lock);
+			if (fuse_session_exited(se)) {
+				pthread_mutex_unlock(&se->mt_lock);
 				return NULL;
 			}
 			list_del_worker(w);
 			mt->numavail--;
 			mt->numworker--;
-			pthread_mutex_unlock(&mt->lock);
+			pthread_mutex_unlock(&se->mt_lock);
 
 			pthread_detach(w->thread_id);
 			fuse_buf_free(&w->fbuf);
@@ -206,11 +204,10 @@ static void *fuse_do_work(void *data)
 			free(w);
 			return NULL;
 		}
-		pthread_mutex_unlock(&mt->lock);
+		pthread_mutex_unlock(&se->mt_lock);
 	}
 
-	sem_post(&mt->finish);
-
+	sem_post(&se->mt_finish);
 	return NULL;
 }
 
@@ -354,9 +351,9 @@ static int fuse_loop_start_thread(struct fuse_mt *mt)
 static void fuse_join_worker(struct fuse_mt *mt, struct fuse_worker *w)
 {
 	pthread_join(w->thread_id, NULL);
-	pthread_mutex_lock(&mt->lock);
+	pthread_mutex_lock(&mt->se->mt_lock);
 	list_del_worker(w);
-	pthread_mutex_unlock(&mt->lock);
+	pthread_mutex_unlock(&mt->se->mt_lock);
 	fuse_buf_free(&w->fbuf);
 	fuse_chan_put(w->ch);
 	free(w);
@@ -392,22 +389,21 @@ int err;
 	mt.max_threads = config->max_threads;
 	mt.main.thread_id = pthread_self();
 	mt.main.prev = mt.main.next = &mt.main;
-	sem_init(&mt.finish, 0, 0);
-	pthread_mutex_init(&mt.lock, NULL);
 
-	pthread_mutex_lock(&mt.lock);
+	pthread_mutex_lock(&se->mt_lock);
 	err = fuse_loop_start_thread(&mt);
-	pthread_mutex_unlock(&mt.lock);
+	pthread_mutex_unlock(&se->mt_lock);
 	if (!err) {
-		/* sem_wait() is interruptible */
 		while (!fuse_session_exited(se))
-			sem_wait(&mt.finish);
+			sem_wait(&se->mt_finish);
+		if (se->debug)
+			fuse_log(FUSE_LOG_DEBUG,
+				 "fuse: session exited, terminating workers\n");
 
-		pthread_mutex_lock(&mt.lock);
+		pthread_mutex_lock(&se->mt_lock);
 		for (w = mt.main.next; w != &mt.main; w = w->next)
 			pthread_cancel(w->thread_id);
-		mt.exit = 1;
-		pthread_mutex_unlock(&mt.lock);
+		pthread_mutex_unlock(&se->mt_lock);
 
 		while (mt.main.next != &mt.main)
 			fuse_join_worker(&mt, mt.main.next);
@@ -415,11 +411,9 @@ int err;
 		err = mt.error;
 	}
 
-	pthread_mutex_destroy(&mt.lock);
-	sem_destroy(&mt.finish);
+	pthread_mutex_destroy(&se->mt_lock);
 	if(se->error != 0)
 		err = se->error;
-	fuse_session_reset(se);
 
 	if (created_config) {
 		fuse_loop_cfg_destroy(config);
diff --git a/lib/fuse_lowlevel.c b/lib/fuse_lowlevel.c
index cf8a7b0..d66e6ab 100644
--- a/lib/fuse_lowlevel.c
+++ b/lib/fuse_lowlevel.c
@@ -19,6 +19,8 @@
 #include "mount_util.h"
 #include "util.h"
 
+#include <pthread.h>
+#include <stdatomic.h>
 #include <stdint.h>
 #include <stdbool.h>
 #include <stdio.h>
@@ -3006,6 +3008,8 @@ void fuse_session_destroy(struct fuse_session *se)
 	if (llp != NULL)
 		fuse_ll_pipe_free(llp);
 	pthread_key_delete(se->pipe_key);
+	sem_destroy(&se->mt_finish);
+	pthread_mutex_destroy(&se->mt_lock);
 	pthread_mutex_destroy(&se->lock);
 	free(se->cuse_data);
 	if (se->fd != -1)
@@ -3358,6 +3362,8 @@ fuse_session_new_versioned(struct fuse_args *args,
 	list_init_nreq(&se->notify_list);
 	se->notify_ctr = 1;
 	pthread_mutex_init(&se->lock, NULL);
+	sem_init(&se->mt_finish, 0, 0);
+	pthread_mutex_init(&se->mt_lock, NULL);
 
 	err = pthread_key_create(&se->pipe_key, fuse_ll_pipe_destructor);
 	if (err) {
@@ -3382,6 +3388,8 @@ fuse_session_new_versioned(struct fuse_args *args,
 	return se;
 
 out5:
+	sem_destroy(&se->mt_finish);
+	pthread_mutex_destroy(&se->mt_lock);
 	pthread_mutex_destroy(&se->lock);
 out4:
 	fuse_opt_free_args(args);
@@ -3605,18 +3613,22 @@ int fuse_req_getgroups(fuse_req_t req, int size, gid_t list[])
 __attribute__((no_sanitize_thread))
 void fuse_session_exit(struct fuse_session *se)
 {
-	se->exited = 1;
+	atomic_store_explicit(&se->mt_exited, 1, memory_order_relaxed);
+	sem_post(&se->mt_finish);
 }
 
 __attribute__((no_sanitize_thread))
 void fuse_session_reset(struct fuse_session *se)
 {
-	se->exited = 0;
+	se->mt_exited = false;
 	se->error = 0;
 }
 
 __attribute__((no_sanitize_thread))
 int fuse_session_exited(struct fuse_session *se)
 {
-	return se->exited;
+	bool exited =
+		atomic_load_explicit(&se->mt_exited, memory_order_relaxed);
+
+	return exited ? 1 : 0;
 }
diff --git a/lib/fuse_signals.c b/lib/fuse_signals.c
index 6ac7230..3848aec 100644
--- a/lib/fuse_signals.c
+++ b/lib/fuse_signals.c
@@ -22,7 +22,12 @@
 #include <execinfo.h>
 #endif
 
+/*
+ * Must not handle SIGCANCEL, as that is used to wake up threads from
+ * syscalls reading requests from /dev/fuse
+ */
 static int teardown_sigs[] = { SIGHUP, SIGINT, SIGTERM };
+
 static int ignore_sigs[] = { SIGPIPE};
 static int fail_sigs[] = { SIGILL, SIGTRAP, SIGABRT, SIGBUS, SIGFPE, SIGSEGV };
 static struct fuse_session *fuse_instance;
@@ -53,8 +58,14 @@ static void dump_stack(void)
 
 static void exit_handler(int sig)
 {
-	if (fuse_instance == NULL)
+	if (fuse_instance == NULL) {
+		fuse_log(FUSE_LOG_ERR, "fuse_instance is NULL\n");
 		return;
+	}
+
+	if (fuse_instance->debug)
+		fuse_log(FUSE_LOG_ERR, "exit_handler called with sig %d\n",
+			 sig);
 
 	fuse_session_exit(fuse_instance);