Hi Corinna, On Wed, 12 Mar 2025 16:37:41 +0100 Corinna Vinschen wrote: > On Mar 13 00:08, Takashi Yano wrote: > > On Wed, 12 Mar 2025 12:16:12 +0100 > > Corinna Vinschen wrote: > > > Hi Takashi, > > > > > > On Mar 12 12:27, Takashi Yano wrote: > > > > 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 <christian.fra...@t-online.de> > > > > Reviewed-by: Corinna Vinschen <cori...@vinschen.de>, Christian Franke > > > > <christian.fra...@t-online.de> > > > > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp> > > > > --- > > > > winsup/cygwin/sigproc.cc | 128 ++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 106 insertions(+), 22 deletions(-) > > > > > > > > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc > > > > index 8739f18f5..ab3acfd24 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,10 @@ details. */ > > > > #define WSSC 60000 // Wait for signal completion > > > > #define WPSP 40000 // Wait for proc_subproc mutex > > > > > > > > +#define PIPE_DEPTH _NSIG /* Historically, the pipe size is _NSIG > > > > packet */ > > > > +#define SIGQ_ROOM 4 > > > > +#define SIGQ_DEPTH (PIPE_DEPTH + SIGQ_ROOM) > > > > > > I'm missing a comment here. Why adding SIGQ_ROOM? > > > > First, I thought signal queue length must be larger than pipe size > > to send __SIGFLUSHFAST safely. However, on second thought, it is > > not enough for some cases while it is not necessary for other cases. > > [PATCH v3 4/6] Cygwin: signal: Do not send __SIGFLUSHFAST if the pipe/queue > > is full > > ensure the safety for sending __SIGFLUSHFAST. > > > > > Other than that, LGTM. > > > > Thanks for reviewing. > > I would be happy if you could review also [PATCH v3 2/6]-[PATCH v3 6/6]. > > I did, sorry. They all LGTM, too. I'm a bit put off by having to add > two events and one mutex to accomplish a safe __SIGFLUSHFAST (rather > hoping a single semaphore would do the trick), but I see how you use the > objects and it makes sense to me. > > > > In terms of the queue size, I wonder if we really have to restrict the > > > queue to a small number of queued signals, 69 right now. The pipe used > > > for communication will take 64K, one allocation granularity slot, anyway. > > > Linux, for instance, queues more than 60K signals. > > > > > > So, wouldn't it make sense to raise the queu depth to some higher > > > value and the pipe size so that it it's <= 64K? > > > > > > While looking into your patch, it occured to me that we have a > > > long-standing bug: We never changed __SIGQUEUE_MAX/SIGQUEUE_MAX in > > > include/cygwin/limits.h when we started to support 64 signals (we only > > > did that for 64 bit Cygwin). > > > > > > We can't change that for existing binaries actually referring the > > > SIGQUEUE_MAX macro, but we should change this, so that > > > sysconf( _SC_SIGQUEUE_MAX) returns the right value, isn't it? > > > > I don't understand what you are concious of. If we change the value > > of SIGQUEUE_MAX, what happens in terms of the binary compatibility? > > I don't think we get a problem in terms of binary compat. Old apps > get a too small value, but I'm not really sure this is a problem at > all. I was just stumbling over this and found that this should have > been changed to 64 (or 65) already ages ago.
If the patch attached (v4 1/6), modified regarding SIGQUEUE_MAX, is as you intend, I'll push the patch seriese. May I? -- Takashi Yano <takashi.y...@nifty.ne.jp>
From 6bc234a6cb6b350431bc79cdf3829d0f7a2acfe5 Mon Sep 17 00:00:00 2001 From: Takashi Yano <takashi.y...@nifty.ne.jp> Date: Fri, 7 Mar 2025 17:15:38 +0900 Subject: [PATCH v4 1/6] 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 <christian.fra...@t-online.de> Reviewed-by: Corinna Vinschen <cori...@vinschen.de>, Christian Franke <christian.fra...@t-online.de> Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp> --- 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..cf5fcbca0 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 ((DWORD) 65536 / 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 -- 2.45.1
