From d7ed4576207082e482ab6b4ad775bef198b18b44 Mon Sep 17 00:00:00 2001 From: Andy Wingo Date: Fri, 21 Jun 2024 11:11:46 +0200 Subject: [PATCH] Ensure the signal-delivery thread is completely stopped before fork * libguile/scmsigs.c: Use raw pthread_create / pthread_join instead of Guile's scm_spawn_thread, to ensure that the thread is entirely stopped before a fork. * libguile/scmsigs.h (scm_i_is_signal_delivery_thread): New internal procedure, replacing a manual check against scm_i_signal_delivery_thread. * libguile/threads.c: Use the new procedure. Based on a patch by Rob Browning. Thanks! --- libguile/scmsigs.c | 104 +++++++++++++++++++++++++++++++++++---------- libguile/scmsigs.h | 5 ++- libguile/threads.c | 11 ++--- 3 files changed, 89 insertions(+), 31 deletions(-) diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c index be96dbd5c..9cd16ea42 100644 --- a/libguile/scmsigs.c +++ b/libguile/scmsigs.c @@ -27,6 +27,7 @@ #include /* for mingw */ #include #include +#include #include #ifdef HAVE_PROCESS_H @@ -85,12 +86,13 @@ static SCM *signal_handlers; static SCM signal_handler_asyncs; static SCM signal_handler_threads; -/* The signal delivery thread. */ -SCM scm_i_signal_delivery_thread = SCM_BOOL_F; +enum thread_state { STOPPED, RUNNING, STOPPING }; /* The mutex held when launching the signal delivery thread. */ static scm_i_pthread_mutex_t signal_delivery_thread_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER; +static enum thread_state signal_delivery_thread_state = STOPPED; +static scm_i_pthread_t signal_delivery_pthread; /* saves the original C handlers, when a new handler is installed. @@ -151,7 +153,7 @@ read_signal_pipe_data (void * data) return NULL; } -static SCM +static void* signal_delivery_thread (void *data) { int sig; @@ -198,23 +200,43 @@ signal_delivery_thread (void *data) close (signal_pipe[0]); signal_pipe[0] = -1; + signal_delivery_thread_state = STOPPED; - return SCM_UNSPECIFIED; /* not reached unless all other threads exited */ + return NULL; /* not reached unless all other threads exited */ +} + +static void* +run_signal_delivery_thread (void *arg) +{ + return scm_with_guile (signal_delivery_thread, arg); } static void start_signal_delivery_thread (void) { - SCM signal_thread; - scm_i_pthread_mutex_lock (&signal_delivery_thread_mutex); + if (signal_delivery_thread_state != STOPPED) + abort (); + if (pipe2 (signal_pipe, O_CLOEXEC) != 0) scm_syserror (NULL); - signal_thread = scm_spawn_thread (signal_delivery_thread, NULL, - scm_handle_by_message, - "signal delivery thread"); - scm_i_signal_delivery_thread = signal_thread; + + signal_delivery_thread_state = RUNNING; + + /* As with the finalizer thread, we use the raw pthread API and + scm_with_guile, to avoid blocking on any lock that scm_spawn_thread + might want to take. */ + int err = pthread_create (&signal_delivery_pthread, NULL, + run_signal_delivery_thread, NULL); + if (err) + { + close (signal_pipe[0]); signal_pipe[0] = -1; + close (signal_pipe[1]); signal_pipe[1] = -1; + fprintf (stderr, "error creating signal delivery thread: %s\n", + strerror (err)); + signal_delivery_thread_state = STOPPED; + } scm_i_pthread_mutex_unlock (&signal_delivery_thread_mutex); } @@ -227,22 +249,43 @@ scm_i_ensure_signal_delivery_thread () scm_i_pthread_once (&once, start_signal_delivery_thread); } +/* Precondition: there is only the current thread and possibly the + signal delivery thread. */ static void stop_signal_delivery_thread () { scm_i_pthread_mutex_lock (&signal_delivery_thread_mutex); + if (signal_delivery_thread_state != RUNNING) + goto done; - if (scm_is_true (scm_i_signal_delivery_thread)) + signal_delivery_thread_state = STOPPING; + close (signal_pipe[1]); + signal_pipe[1] = -1; + + int res = pthread_join (signal_delivery_pthread, NULL); + if (res) + fprintf (stderr, "error joining signal delivery thread: %s\n", + strerror (res)); + else { - close (signal_pipe[1]); - signal_pipe[1] = -1; - scm_join_thread (scm_i_signal_delivery_thread); - scm_i_signal_delivery_thread = SCM_BOOL_F; + if (signal_delivery_thread_state != STOPPED) + abort (); } + done: scm_i_pthread_mutex_unlock (&signal_delivery_thread_mutex); } +static int +is_signal_delivery_thread (scm_i_pthread_t thread) +{ + scm_i_pthread_mutex_lock (&signal_delivery_thread_mutex); + int res = (signal_delivery_thread_state == RUNNING && + pthread_equal (thread, signal_delivery_pthread)); + scm_i_pthread_mutex_unlock (&signal_delivery_thread_mutex); + return res; +} + #else /* !SCM_USE_PTHREAD_THREADS */ static void @@ -274,6 +317,12 @@ stop_signal_delivery_thread () return; } +static int +is_signal_delivery_thread (scm_i_pthread_t thread) +{ + return 0; +} + #endif /* !SCM_USE_PTHREAD_THREADS */ /* Perform pre-fork cleanup by stopping the signal delivery thread. */ @@ -283,6 +332,12 @@ scm_i_signals_pre_fork () stop_signal_delivery_thread (); } +int +scm_i_is_signal_delivery_thread (struct scm_thread *t) +{ + return is_signal_delivery_thread (t->pthread); +} + /* Perform post-fork setup by restarting the signal delivery thread if it was active before fork. This happens in both the parent and the child process. */ @@ -753,15 +808,20 @@ SCM_DEFINE (scm_raise, "raise", 1, 0, 0, void scm_i_close_signal_pipe() { - /* SIGNAL_DELIVERY_THREAD_MUTEX is only locked while the signal delivery - thread is being launched. The thread that calls this function is - already holding the thread admin mutex, so if the delivery thread hasn't - been launched at this point, it never will be before shutdown. */ - scm_i_pthread_mutex_lock (&signal_delivery_thread_mutex); + /* There is at most one other Guile thread. It may be the signal + delivery thread. If it is the signal delivery thread, the mutex + will not be locked. If the mutex is locked, then, we have nothing + to do. */ + if (scm_i_pthread_mutex_trylock (&signal_delivery_thread_mutex)) + return; #if SCM_USE_PTHREAD_THREADS - if (scm_is_true (scm_i_signal_delivery_thread)) - close (signal_pipe[1]); + if (signal_delivery_thread_state == RUNNING) + { + signal_delivery_thread_state = STOPPING; + close (signal_pipe[1]); + signal_pipe[1] = -1; + } #endif scm_i_pthread_mutex_unlock (&signal_delivery_thread_mutex); diff --git a/libguile/scmsigs.h b/libguile/scmsigs.h index 876690fa5..eefba70a0 100644 --- a/libguile/scmsigs.h +++ b/libguile/scmsigs.h @@ -1,7 +1,7 @@ #ifndef SCM_SCMSIGS_H #define SCM_SCMSIGS_H -/* Copyright 1995-1998,2000,2002,2006-2008,2018,2023 +/* Copyright 1995-1998,2000,2002,2006-2008,2018,2023-2024 Free Software Foundation, Inc. This file is part of Guile. @@ -46,6 +46,7 @@ SCM_INTERNAL void scm_i_ensure_signal_delivery_thread (void); SCM_INTERNAL void scm_i_signals_pre_fork (void); SCM_INTERNAL void scm_i_signals_post_fork (void); -SCM_INTERNAL SCM scm_i_signal_delivery_thread; +struct scm_thread; +SCM_INTERNAL int scm_i_is_signal_delivery_thread (struct scm_thread *t); #endif /* SCM_SCMSIGS_H */ diff --git a/libguile/threads.c b/libguile/threads.c index e7c6787f1..77e99da74 100644 --- a/libguile/threads.c +++ b/libguile/threads.c @@ -1,4 +1,4 @@ -/* Copyright 1995-1998,2000-2014,2018-2019,2023 +/* Copyright 1995-1998,2000-2014,2018-2019,2023-2024 Free Software Foundation, Inc. This file is part of Guile. @@ -492,9 +492,8 @@ on_thread_exit (void *v) t->handle = SCM_PACK (0); /* If there's only one other thread, it could be the signal delivery - thread, so we need to notify it to shut down by closing its read pipe. - If it's not the signal delivery thread, then closing the read pipe isn't - going to hurt. */ + thread, in which case we should shut it down also by closing its + read pipe. */ if (thread_count <= 1) scm_i_close_signal_pipe (); @@ -1692,9 +1691,7 @@ SCM_DEFINE (scm_all_threads, "all-threads", 0, 0, 0, for (t = all_threads; t && n > 0; t = t->next_thread) { - if (!t->exited - && (scm_is_false (scm_i_signal_delivery_thread) - || (!scm_is_eq (t->handle, scm_i_signal_delivery_thread)))) + if (!t->exited && !scm_i_is_signal_delivery_thread (t)) { SCM_SETCAR (*l, t->handle); l = SCM_CDRLOC (*l);