https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=8ac22f8f16a81a54b9c4ce3a2f9aaf4a48456cdd
commit 8ac22f8f16a81a54b9c4ce3a2f9aaf4a48456cdd Author: Takashi Yano <[email protected]> Date: Fri Mar 7 17:15:38 2025 +0900 Cygwin: signal: Redesign signal queue handling The previous implementation of the signal queue behaves as: 1) Signals in the queue are processed in a disordered manner. 2) If the same signal is already in the queue, new signal is discarded. Strictly speaking, these behaviours do not violate POSIX. However, these could be a cause of unexpected behaviour in some software. In Linux, some important signals such as SIGSTOP/SIGCONT do not seem to behave like that. This patch prevents SIGKILL, SIGSTOP, SIGCONT, and SIGRT* from that issue. Moreover, if SA_SIGINFO is set in sa_flags, the signal is treated almost as the same. Addresses: https://cygwin.com/pipermail/cygwin/2025-March/257582.html Fixes: 7ac6173643b1 ("(pending_signals): New class.") Reported by: Christian Franke <[email protected]> Reviewed-by: Corinna Vinschen <[email protected]>, Christian Franke <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Diff: --- winsup/cygwin/include/cygwin/limits.h | 2 +- winsup/cygwin/sigproc.cc | 126 ++++++++++++++++++++++++++++------ 2 files changed, 105 insertions(+), 23 deletions(-) diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h index ea3e2836a..204154da9 100644 --- a/winsup/cygwin/include/cygwin/limits.h +++ b/winsup/cygwin/include/cygwin/limits.h @@ -41,7 +41,7 @@ details. */ #define __RTSIG_MAX 33 #define __SEM_VALUE_MAX 1147483648 -#define __SIGQUEUE_MAX 32 +#define __SIGQUEUE_MAX 1024 #define __STREAM_MAX 20 #define __SYMLOOP_MAX 10 #define __TIMER_MAX 32 diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index 8739f18f5..e399c686b 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -21,6 +21,7 @@ details. */ #include "cygtls.h" #include "ntdll.h" #include "exception.h" +#include <assert.h> /* * Convenience defines @@ -28,6 +29,8 @@ details. */ #define WSSC 60000 // Wait for signal completion #define WPSP 40000 // Wait for proc_subproc mutex +#define PIPE_DEPTH (wincap.allocation_granularity ()/ sizeof (sigpacket)) + /* * Global variables */ @@ -104,15 +107,16 @@ static void wait_sig (VOID *arg); class pending_signals { - sigpacket sigs[_NSIG + 1]; + sigpacket sigs[SIGQUEUE_MAX]; sigpacket start; + int queue_left; SRWLOCK queue_lock; bool retry; void lock () { AcquireSRWLockExclusive (&queue_lock); } void unlock () { ReleaseSRWLockExclusive (&queue_lock); } public: - pending_signals (): queue_lock (SRWLOCK_INIT) {} + pending_signals (): queue_left (SIGQUEUE_MAX), queue_lock (SRWLOCK_INIT) {} void add (sigpacket&); bool pending () {retry = !!start.next; return retry;} void clear (int sig, bool need_lock); @@ -441,15 +445,22 @@ sig_clear (int sig, bool need_lock) void pending_signals::clear (int sig, bool need_lock) { - sigpacket *q = sigs + sig; - if (!sig || !q->si.si_signo) + sigpacket *q = &start; + + if (!sig) return; + if (need_lock) lock (); - q->si.si_signo = 0; - q->prev->next = q->next; - if (q->next) - q->next->prev = q->prev; + while ((q = q->next)) + if (q->si.si_signo == sig) + { + q->si.si_signo = 0; + q->prev->next = q->next; + if (q->next) + q->next->prev = q->prev; + queue_left++; + } if (need_lock) unlock (); } @@ -469,6 +480,7 @@ pending_signals::clear (_cygtls *tls) q->prev->next = q->next; if (q->next) q->next->prev = q->prev; + queue_left++; } unlock (); } @@ -509,7 +521,7 @@ sigproc_init () char char_sa_buf[1024]; PSECURITY_ATTRIBUTES sa = sec_user_nih ((PSECURITY_ATTRIBUTES) char_sa_buf, cygheap->user.sid()); DWORD err = fhandler_pipe::create (sa, &my_readsig, &my_sendsig, - _NSIG * sizeof (sigpacket), "sigwait", + PIPE_DEPTH * sizeof (sigpacket), "sigwait", PIPE_ADD_PID); if (err) { @@ -1311,23 +1323,85 @@ talktome (siginfo_t *si) new cygthread (commune_process, size, si, "commune"); } -/* Add a packet to the beginning of the queue. +static inline bool +is_sigsys (int sig) +{ + return sig == SIGKILL || sig == SIGSTOP || sig == SIGCONT; +} + +static inline bool +is_sigrt (int sig) +{ + return sig >= SIGRTMIN && sig <= SIGRTMAX; +} + +static inline bool +is_sigsysrt (int sig) +{ + return is_sigsys (sig) || is_sigrt (sig); +} + +/* Add a packet to the end of the queue to process signals + in the order they are issued except for SIGRT*. Should only be called from signal thread. */ void pending_signals::add (sigpacket& pack) { - sigpacket *se; + sigpacket *se = NULL, *q = &start; + bool queue_once = !is_sigsysrt (pack.si.si_signo) + && !(global_sigs[pack.si.si_signo].sa_flags & SA_SIGINFO); - se = sigs + pack.si.si_signo; - if (se->si.si_signo) - return; - *se = pack; lock (); - se->next = start.next; - se->prev = &start; - se->prev->next = se; - if (se->next) - se->next->prev = se; + if (pack.si.si_signo != SIGKILL) + while (q->next) + { + /* Linux man signal(7) says: + "If different real-time signals are sent to a process, they are + delivered starting with the lowest-numbered signal." */ + if (is_sigrt (q->next->si.si_signo) && is_sigrt (pack.si.si_signo) + && q->next->si.si_signo > pack.si.si_signo) + break; + /* Linux man signal(7) says: + "If both standard and real-time signals are pending for a process, + POSIX leaves it unspecified which is delivered first. Linux, like + many other implementations, gives priority to standard signals in + this case." */ + if (is_sigrt (q->next->si.si_signo) && !is_sigrt (pack.si.si_signo)) + break; + q = q->next; + /* Linux man signal(7) says: + "if multiple instances of a standard signal are delivered while + that signal is currently blocked, then only one instance is + queued." */ + /* POSIX.1-2004 says on sigaction(): + "If SA_SIGINFO is set in sa_flags, then subsequent occurrences + of sig generated by sigqueue() or as a result of any signal- + generating function that supports the specification of an + application-defined value (when sig is already pending) shall + be queued in FIFO order until delivered or accepted;" */ + if (queue_once && q->si.si_signo == pack.si.si_signo) + { + unlock (); + return; + } + } + + assert (queue_left > 0); + for (int i = 0; i < SIGQUEUE_MAX; i++) + if (sigs[i].si.si_signo == 0) + { + se = sigs + i; + *se = pack; + break; + } + assert (se != NULL); + queue_left--; + + if (q->next) + q->next->prev = se; + se->next = q->next; + se->prev = q; + q->next = se; unlock (); } @@ -1354,12 +1428,12 @@ wait_sig (VOID *) { DWORD nb; sigpacket pack = {}; - if (sigq.retry) + if (sigq.retry || sigq.queue_left == 0) pack.si.si_signo = __SIGFLUSH; else if (sigq.start.next && PeekNamedPipe (my_readsig, NULL, 0, NULL, &nb, NULL) && !nb) { - Sleep (GetTickCount () - t0 > 10 ? 1 : 0); + Sleep ((sig_held || GetTickCount () - t0 > 10) ? 1 : 0); pack.si.si_signo = __SIGFLUSH; } else if (!ReadFile (my_readsig, &pack, sizeof (pack), &nb, NULL)) @@ -1505,7 +1579,15 @@ wait_sig (VOID *) q->prev->next = q->next; if (q->next) q->next->prev = q->prev; + sigq.queue_left++; } + else if (is_sigsysrt (q->si.si_signo) + || ((global_sigs[q->si.si_signo].sa_flags & SA_SIGINFO) + && NOTSTATE (myself, PID_STOPPED))) + /* Stop processing further to prevent the signals from + being processed in a disorderd manner if the signal + is a realtime signal or SA_SIGINFO is set. */ + break; } sigq.unlock (); /* At least one signal still queued? The event is used in select
