For several years, I'm seeing occasional failures of the 'test-asyncsafe-spin2'
test on native Windows machines with 2 or more CPUs. These failures are caused
by the sigprocmask replacement in gnulib, that is not multithread-safe.
The first patch fixes that.
Then, there is a second race condition on native Windows (not exhibited by a
unit test): If a sigprocmask() invocation happens to be interrupted by a signal,
and the signal handler invokes sigprocmask(), things may crash because
the sigprocmask replacement in gnulib is not async-signal safe.
The second patch fixes that by not invoking sigprocmask() from a signal handler.
2025-05-16 Bruno Haible <br...@clisp.org>
asyncsafe-spin: Fix race condition on native Windows.
* lib/asyncsafe-spin.h (asyncsafe_spin_lock, asyncsafe_spin_unlock): Add
from_signal_handler parameter.
* lib/asyncsafe-spin.c (asyncsafe_spin_lock, asyncsafe_spin_unlock):
Likewise.
* modules/asyncsafe-spin (Depends-on): Add bool.
* tests/test-asyncsafe-spin1.c (main): Update.
* tests/test-asyncsafe-spin2.c (lock_mutator_thread,
lock_checker_thread): Update.
* lib/clean-temp-simple.c (clean_temp_asyncsafe_close): Update.
* lib/clean-temp.c (asyncsafe_fclose_variant): Update.
2025-05-16 Bruno Haible <br...@clisp.org>
sigprocmask: Make multithread-safe on native Windows.
* lib/sigprocmask.c: Include glthread/lock.h.
(sig_lock): New variable.
(blocked_set): Remove 'volatile'.
(sigprocmask, _gl_raise_SIGPIPE): Use the sig_lock.
* modules/sigprocmask (Depends-on): Add lock.
* doc/posix-functions/sigprocmask.texi: Mention the async-safety issue.
>From e9d8c4f480caf32ef072d85d26af68bc9a68d2fb Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 16 May 2025 15:51:13 +0200
Subject: [PATCH 1/2] sigprocmask: Make multithread-safe on native Windows.
* lib/sigprocmask.c: Include glthread/lock.h.
(sig_lock): New variable.
(blocked_set): Remove 'volatile'.
(sigprocmask, _gl_raise_SIGPIPE): Use the sig_lock.
* modules/sigprocmask (Depends-on): Add lock.
* doc/posix-functions/sigprocmask.texi: Mention the async-safety issue.
---
ChangeLog | 10 ++++++++++
doc/posix-functions/sigprocmask.texi | 6 ++++++
lib/sigprocmask.c | 20 ++++++++++++++++++--
modules/sigprocmask | 1 +
4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 95323c3a19..25cced87c6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2025-05-16 Bruno Haible <br...@clisp.org>
+
+ sigprocmask: Make multithread-safe on native Windows.
+ * lib/sigprocmask.c: Include glthread/lock.h.
+ (sig_lock): New variable.
+ (blocked_set): Remove 'volatile'.
+ (sigprocmask, _gl_raise_SIGPIPE): Use the sig_lock.
+ * modules/sigprocmask (Depends-on): Add lock.
+ * doc/posix-functions/sigprocmask.texi: Mention the async-safety issue.
+
2025-05-16 Bruno Haible <br...@clisp.org>
pthread-rwlock tests: Add a comment.
diff --git a/doc/posix-functions/sigprocmask.texi b/doc/posix-functions/sigprocmask.texi
index 7683986b3f..02bc537614 100644
--- a/doc/posix-functions/sigprocmask.texi
+++ b/doc/posix-functions/sigprocmask.texi
@@ -28,3 +28,9 @@
return convention. It's simpler to use @code{sigprocmask}, since it does
not require linking with @code{-lpthread} on some platforms:
glibc, NetBSD, OpenBSD, AIX.
+
+Note: While on POSIX platforms, @code{sigprocmask} is multithread-safe
+and async-signal safe (cf. POSIX section ``Signal Actions''
+@url{https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03}),
+the gnulib replacement on native Windows is only multithread-safe,
+not async-signal safe.
diff --git a/lib/sigprocmask.c b/lib/sigprocmask.c
index f79386c8d0..d52e191c44 100644
--- a/lib/sigprocmask.c
+++ b/lib/sigprocmask.c
@@ -24,6 +24,8 @@
#include <stdint.h>
#include <stdlib.h>
+#include "glthread/lock.h"
+
#if HAVE_MSVC_INVALID_PARAMETER_HANDLER
# include "msvc-inval.h"
#endif
@@ -84,6 +86,10 @@ signal_nothrow (int sig, handler_t handler)
# define signal signal_nothrow
#endif
+/* This lock protects the variables defined in this file from concurrent
+ modification in multiple threads. */
+gl_lock_define_initialized(static, sig_lock)
+
/* Handling of gnulib defined signals. */
#if GNULIB_defined_SIGPIPE
@@ -182,7 +188,7 @@ sigfillset (sigset_t *set)
}
/* Set of currently blocked signals. */
-static volatile sigset_t blocked_set /* = 0 */;
+static sigset_t blocked_set /* = 0 */;
/* Set of currently blocked and pending signals. */
static volatile sig_atomic_t pending_array[NSIG] /* = { 0 } */;
@@ -221,6 +227,8 @@ static volatile handler_t old_handlers[NSIG];
int
sigprocmask (int operation, const sigset_t *set, sigset_t *old_set)
{
+ gl_lock_lock (sig_lock);
+
if (old_set != NULL)
*old_set = blocked_set;
@@ -242,6 +250,7 @@ sigprocmask (int operation, const sigset_t *set, sigset_t *old_set)
new_blocked_set = blocked_set & ~*set;
break;
default:
+ gl_lock_unlock (sig_lock);
errno = EINVAL;
return -1;
}
@@ -286,6 +295,8 @@ sigprocmask (int operation, const sigset_t *set, sigset_t *old_set)
raise (sig);
}
}
+
+ gl_lock_unlock (sig_lock);
return 0;
}
@@ -334,11 +345,16 @@ rpl_signal (int sig, handler_t handler)
int
_gl_raise_SIGPIPE (void)
{
+ gl_lock_lock (sig_lock);
if (blocked_set & (1U << SIGPIPE))
- pending_array[SIGPIPE] = 1;
+ {
+ pending_array[SIGPIPE] = 1;
+ gl_lock_unlock (sig_lock);
+ }
else
{
handler_t handler = SIGPIPE_handler;
+ gl_lock_unlock (sig_lock);
if (handler == SIG_DFL)
exit (128 + SIGPIPE);
else if (handler != SIG_IGN)
diff --git a/modules/sigprocmask b/modules/sigprocmask
index 7f2e083803..79e7c7302f 100644
--- a/modules/sigprocmask
+++ b/modules/sigprocmask
@@ -8,6 +8,7 @@ m4/signalblocking.m4
Depends-on:
signal-h
stdint-h [test $HAVE_POSIX_SIGNALBLOCKING = 0]
+lock [test $HAVE_POSIX_SIGNALBLOCKING = 0]
raise [test $HAVE_POSIX_SIGNALBLOCKING = 0]
msvc-inval [test $HAVE_POSIX_SIGNALBLOCKING = 0]
--
2.43.0
>From 9ea3eecee3eafc7cdb08d6167b387374d6dc1378 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 16 May 2025 16:37:03 +0200
Subject: [PATCH 2/2] asyncsafe-spin: Fix race condition on native Windows.
* lib/asyncsafe-spin.h (asyncsafe_spin_lock, asyncsafe_spin_unlock): Add
from_signal_handler parameter.
* lib/asyncsafe-spin.c (asyncsafe_spin_lock, asyncsafe_spin_unlock):
Likewise.
* modules/asyncsafe-spin (Depends-on): Add bool.
* tests/test-asyncsafe-spin1.c (main): Update.
* tests/test-asyncsafe-spin2.c (lock_mutator_thread,
lock_checker_thread): Update.
* lib/clean-temp-simple.c (clean_temp_asyncsafe_close): Update.
* lib/clean-temp.c (asyncsafe_fclose_variant): Update.
---
ChangeLog | 14 ++++++++++++++
lib/asyncsafe-spin.c | 33 ++++++++++++++++++++++++++++++---
lib/asyncsafe-spin.h | 6 ++++--
lib/clean-temp-simple.c | 4 ++--
lib/clean-temp.c | 6 ++++--
modules/asyncsafe-spin | 1 +
tests/test-asyncsafe-spin1.c | 8 ++++----
tests/test-asyncsafe-spin2.c | 20 ++++++++++----------
8 files changed, 69 insertions(+), 23 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 25cced87c6..e628b7169f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2025-05-16 Bruno Haible <br...@clisp.org>
+
+ asyncsafe-spin: Fix race condition on native Windows.
+ * lib/asyncsafe-spin.h (asyncsafe_spin_lock, asyncsafe_spin_unlock): Add
+ from_signal_handler parameter.
+ * lib/asyncsafe-spin.c (asyncsafe_spin_lock, asyncsafe_spin_unlock):
+ Likewise.
+ * modules/asyncsafe-spin (Depends-on): Add bool.
+ * tests/test-asyncsafe-spin1.c (main): Update.
+ * tests/test-asyncsafe-spin2.c (lock_mutator_thread,
+ lock_checker_thread): Update.
+ * lib/clean-temp-simple.c (clean_temp_asyncsafe_close): Update.
+ * lib/clean-temp.c (asyncsafe_fclose_variant): Update.
+
2025-05-16 Bruno Haible <br...@clisp.org>
sigprocmask: Make multithread-safe on native Windows.
diff --git a/lib/asyncsafe-spin.c b/lib/asyncsafe-spin.c
index 5dbefa1c60..38740e817c 100644
--- a/lib/asyncsafe-spin.c
+++ b/lib/asyncsafe-spin.c
@@ -31,18 +31,45 @@ asyncsafe_spin_init (asyncsafe_spinlock_t *lock)
void
asyncsafe_spin_lock (asyncsafe_spinlock_t *lock,
+ bool from_signal_handler,
const sigset_t *mask, sigset_t *saved_mask)
{
- sigprocmask (SIG_BLOCK, mask, saved_mask); /* equivalent to pthread_sigmask */
+ /* On all platforms, when not running in a signal handler, we need to block
+ the signals until the corresponding asyncsafe_spin_unlock() invocation.
+ This is needed because if, during that period, a signal occurred and it
+ happened to run in the current thread and it were to wait on this spin
+ lock, it would hang.
+ On platforms other than native Windows, it is useful to do the same
+ thing also within a signal handler, since signals may remain enabled
+ while a signal handler runs. It is possible to do this because
+ sigprocmask() is safe to call from within a signal handler, see
+ POSIX section "Signal Actions"
+ <https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03>.
+ (In other words, sigprocmask() is atomic, because it is implemented as a
+ system call.)
+ Whereas on native Windows, sigprocmask() is not atomic, because it
+ manipulates global variables. Therefore in this case, we are *not*
+ allowed to call it from within a signal handler. */
+#if defined _WIN32 && !defined __CYGWIN__
+ if (!from_signal_handler)
+#endif
+ sigprocmask (SIG_BLOCK, mask, saved_mask); /* equivalent to pthread_sigmask */
+
glthread_spinlock_lock (lock);
}
void
-asyncsafe_spin_unlock (asyncsafe_spinlock_t *lock, const sigset_t *saved_mask)
+asyncsafe_spin_unlock (asyncsafe_spinlock_t *lock,
+ bool from_signal_handler,
+ const sigset_t *saved_mask)
{
if (glthread_spinlock_unlock (lock))
abort ();
- sigprocmask (SIG_SETMASK, saved_mask, NULL); /* equivalent to pthread_sigmask */
+
+#if defined _WIN32 && !defined __CYGWIN__
+ if (!from_signal_handler)
+#endif
+ sigprocmask (SIG_SETMASK, saved_mask, NULL); /* equivalent to pthread_sigmask */
}
void
diff --git a/lib/asyncsafe-spin.h b/lib/asyncsafe-spin.h
index 1bbf2f4702..c2a4b6bdfe 100644
--- a/lib/asyncsafe-spin.h
+++ b/lib/asyncsafe-spin.h
@@ -28,9 +28,9 @@
both in regular multithreaded code and in signal handlers:
sigset_t saved_mask;
- asyncsafe_spin_lock (&lock, &mask, &saved_mask);
+ asyncsafe_spin_lock (&lock, from_signal_handler, &mask, &saved_mask);
do_something_contentious ();
- asyncsafe_spin_unlock (&lock, &saved_mask);
+ asyncsafe_spin_unlock (&lock, from_signal_handler, &saved_mask);
The mask you specify here is the set of signals whose handlers might want to
take the same lock.
@@ -53,8 +53,10 @@ extern "C" {
extern void asyncsafe_spin_init (asyncsafe_spinlock_t *lock);
extern void asyncsafe_spin_lock (asyncsafe_spinlock_t *lock,
+ bool from_signal_handler,
const sigset_t *mask, sigset_t *saved_mask);
extern void asyncsafe_spin_unlock (asyncsafe_spinlock_t *lock,
+ bool from_signal_handler,
const sigset_t *saved_mask);
extern void asyncsafe_spin_destroy (asyncsafe_spinlock_t *lock);
diff --git a/lib/clean-temp-simple.c b/lib/clean-temp-simple.c
index 727ff559f2..95103b8ac0 100644
--- a/lib/clean-temp-simple.c
+++ b/lib/clean-temp-simple.c
@@ -130,7 +130,7 @@ clean_temp_asyncsafe_close (struct closeable_fd *element)
int ret;
int saved_errno;
- asyncsafe_spin_lock (&element->lock, fatal_signal_set, &saved_mask);
+ asyncsafe_spin_lock (&element->lock, true, fatal_signal_set, &saved_mask);
if (!element->closed)
{
ret = close (element->fd);
@@ -142,7 +142,7 @@ clean_temp_asyncsafe_close (struct closeable_fd *element)
ret = 0;
saved_errno = 0;
}
- asyncsafe_spin_unlock (&element->lock, &saved_mask);
+ asyncsafe_spin_unlock (&element->lock, true, &saved_mask);
element->done = true;
errno = saved_errno;
diff --git a/lib/clean-temp.c b/lib/clean-temp.c
index f477e7719e..e8d0cf3113 100644
--- a/lib/clean-temp.c
+++ b/lib/clean-temp.c
@@ -109,7 +109,8 @@ asyncsafe_fclose_variant (struct closeable_fd *element, FILE *fp,
int ret;
int saved_errno;
- asyncsafe_spin_lock (&element->lock, get_fatal_signal_set (), &saved_mask);
+ asyncsafe_spin_lock (&element->lock, false,
+ get_fatal_signal_set (), &saved_mask);
if (!element->closed)
{
ret = fclose_variant (fp); /* invokes close (element->fd) */
@@ -121,7 +122,8 @@ asyncsafe_fclose_variant (struct closeable_fd *element, FILE *fp,
ret = 0;
saved_errno = 0;
}
- asyncsafe_spin_unlock (&element->lock, &saved_mask);
+ asyncsafe_spin_unlock (&element->lock, false,
+ &saved_mask);
element->done = true;
errno = saved_errno;
diff --git a/modules/asyncsafe-spin b/modules/asyncsafe-spin
index d918096163..9c3d0097f1 100644
--- a/modules/asyncsafe-spin
+++ b/modules/asyncsafe-spin
@@ -7,6 +7,7 @@ lib/asyncsafe-spin.c
Depends-on:
signal-h
+bool
sigprocmask
spin
diff --git a/tests/test-asyncsafe-spin1.c b/tests/test-asyncsafe-spin1.c
index dd528f422e..9662bb255b 100644
--- a/tests/test-asyncsafe-spin1.c
+++ b/tests/test-asyncsafe-spin1.c
@@ -36,8 +36,8 @@ main (void)
/* Check a spin-lock initialized through the constant initializer. */
{
sigset_t saved_set;
- asyncsafe_spin_lock (&global_spin_lock, &set, &saved_set);
- asyncsafe_spin_unlock (&global_spin_lock, &saved_set);
+ asyncsafe_spin_lock (&global_spin_lock, false, &set, &saved_set);
+ asyncsafe_spin_unlock (&global_spin_lock, false, &saved_set);
}
/* Check a spin-lock initialized through asyncsafe_spin_init. */
@@ -50,8 +50,8 @@ main (void)
for (i = 0; i < 10; i++)
{
sigset_t saved_set;
- asyncsafe_spin_lock (&local_spin_lock, &set, &saved_set);
- asyncsafe_spin_unlock (&local_spin_lock, &saved_set);
+ asyncsafe_spin_lock (&local_spin_lock, false, &set, &saved_set);
+ asyncsafe_spin_unlock (&local_spin_lock, false, &saved_set);
}
asyncsafe_spin_destroy (&local_spin_lock);
diff --git a/tests/test-asyncsafe-spin2.c b/tests/test-asyncsafe-spin2.c
index 106aca24b9..b6e0563797 100644
--- a/tests/test-asyncsafe-spin2.c
+++ b/tests/test-asyncsafe-spin2.c
@@ -56,10 +56,10 @@
#include "asyncsafe-spin.h"
#if !ENABLE_LOCKING
# define asyncsafe_spin_init(lock) (void)(lock)
-# define asyncsafe_spin_lock(lock, mask, saved_mask) \
- ((void)(lock), (void)(mask), (void)(saved_mask))
-# define asyncsafe_spin_unlock(lock, saved_mask) \
- ((void)(lock), (void)(saved_mask))
+# define asyncsafe_spin_lock(lock, from, mask, saved_mask) \
+ ((void)(lock), (void)(from), (void)(mask), (void)(saved_mask))
+# define asyncsafe_spin_unlock(lock, from, saved_mask) \
+ ((void)(lock), (void)(from), (void)(saved_mask))
# define asyncsafe_spin_destroy(lock) (void)(lock)
#endif
@@ -130,7 +130,7 @@ lock_mutator_thread (void *arg)
int i1, i2, value;
dbgprintf ("Mutator %p before lock\n", gl_thread_self_pointer ());
- asyncsafe_spin_lock (&my_lock, &signals_to_block, &saved_signals);
+ asyncsafe_spin_lock (&my_lock, false, &signals_to_block, &saved_signals);
dbgprintf ("Mutator %p after lock\n", gl_thread_self_pointer ());
i1 = random_account ();
@@ -140,13 +140,13 @@ lock_mutator_thread (void *arg)
account[i2] -= value;
dbgprintf ("Mutator %p before unlock\n", gl_thread_self_pointer ());
- asyncsafe_spin_unlock (&my_lock, &saved_signals);
+ asyncsafe_spin_unlock (&my_lock, false, &saved_signals);
dbgprintf ("Mutator %p after unlock\n", gl_thread_self_pointer ());
dbgprintf ("Mutator %p before check lock\n", gl_thread_self_pointer ());
- asyncsafe_spin_lock (&my_lock, &signals_to_block, &saved_signals);
+ asyncsafe_spin_lock (&my_lock, false, &signals_to_block, &saved_signals);
check_accounts ();
- asyncsafe_spin_unlock (&my_lock, &saved_signals);
+ asyncsafe_spin_unlock (&my_lock, false, &saved_signals);
dbgprintf ("Mutator %p after check unlock\n", gl_thread_self_pointer ());
yield ();
@@ -166,9 +166,9 @@ lock_checker_thread (void *arg)
sigset_t saved_signals;
dbgprintf ("Checker %p before check lock\n", gl_thread_self_pointer ());
- asyncsafe_spin_lock (&my_lock, &signals_to_block, &saved_signals);
+ asyncsafe_spin_lock (&my_lock, false, &signals_to_block, &saved_signals);
check_accounts ();
- asyncsafe_spin_unlock (&my_lock, &saved_signals);
+ asyncsafe_spin_unlock (&my_lock, false, &saved_signals);
dbgprintf ("Checker %p after check unlock\n", gl_thread_self_pointer ());
yield ();
--
2.43.0
