On Tue, 11 Mar 2025 11:28:57 +0100
Corinna Vinschen wrote:
> On Mar 11 17:56, Takashi Yano wrote:
> > On Mon, 10 Mar 2025 21:57:52 +0100
> > Corinna Vinschen wrote:
> > > On Mar 9 13:28, Christian Franke wrote:
> > > > Takashi Yano wrote:
> > > > > ...
> > > > > With this patch prevents all signals from that issues by redesigning
> > > > > the signal queue, Only the exception is the case that the process is
> > > > > in the PID_STOPPED state. In this case, SIGCONT/SIGKILL should be
> > > > > processed prior to the other signals in the queue.
> > > > >
> > > > > 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:
> > > > > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp>
> > > > > ...
> > > > > void
> > > > > pending_signals::add (sigpacket& pack)
> > > > > {
> > > > > ...
> > > > > + if (q->si.si_signo == pack.si.si_signo)
> > > > > + q->usecount++;
> > > > > ...
> > > > >
> > > >
> > > > This should possibly also compare the si.si_sigval fields. Otherwise
> > > > values
> > > > would be lost if the same real-time signal is issued multiple times with
> > > > different value parameters.
> > >
> > > Looks like this doesn't only affect RT signals. I just read POSIX.1-2024
> > > on sigaction,
> > > https://pubs.opengroup.org/onlinepubs/9799919799/functions/sigaction.html
> > > and this is what it has to say in terms of queuing:
> > >
> > > If SA_SIGINFO is not set in sa_flags, then the disposition of
> > > subsequent occurrences of sig when it is already pending is
> > > implementation-defined; the signal-catching function shall be invoked
> > > with a single argument. 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;
> > >
> > > This isn't quite what the Linux man pages describe. Signal(7) says:
> > >
> > > Standard signals do not queue. If multiple instances of a standard
> > > signal are generated while that signal is blocked, then only one
> > > instance of the signal is marked as pending (and the signal will be
> > > delivered just once when it is unblocked). In the case where a
> > > standard signal is already pending, the siginfo_t structure (see
> > > sigaction(2)) associated with that signal is not overwritten on
> > > arrival of subsequent instances of the same signal. Thus, the process
> > > will receive the information associated with the first instance of the
> > > signal.
> > >
> > > Am I just confused or do these two description not match?
> >
> > Yeah, I think Linux is not fully compliant with POSIX.
> > My v2 patch intends signal queue behaves like Linux when SA_SIGINFO
> > is not set. On the contrary, it behaves as POSIX states if SA_SIGINFO
> > is set.
> >
> > Does this make sense?
>
> Absolutely.
>
> The word "implementation-defined" in the POSIX docs give you allowance
> to queue signales all the time, though. Just something to keep in mind
> if that simplifies things.
OK.
BTW, I found another bug in signal handling.
The following testcase (that is the modified version of Christian's one)
hangs with my v2 signal queue patch. It seems that the signal handler
is called from inside of the signal handler, _cygtls::context seems
to be destroyed. To confirm this, I tested the patch attached.
The patch is not good enough yet, however, the test case works with
this patch.
Any idea?
#include <sched.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
static volatile sig_atomic_t sigcnt, term;
static void sighandler1(int sig, siginfo_t *si, void *p)
{
(void)sig;
++sigcnt;
write(1, "[ALRM]\n", 7);
}
static void sighandler2(int sig, siginfo_t *si, void *p)
{
(void)sig;
term = 1;
write(1, "[TERM]\n", 7);
}
int main()
{
pid_t pid = fork();
if (pid == (pid_t)-1) {
perror("fork"); return 1;
}
if (!pid) {
struct sigaction sa1 = {0,};
struct sigaction sa2 = {0,};
sa1.sa_sigaction = sighandler1;
sa2.sa_sigaction = sighandler2;
sa1.sa_flags = SA_SIGINFO;
sa2.sa_flags = SA_SIGINFO;
sigaction(SIGALRM, &sa1, NULL);
sigaction(SIGTERM, &sa2, NULL);
while (!term)
sched_yield();
printf("%d: %d SIGALRM received, exit(42)\n", (int)getpid(), sigcnt);
fflush(stdout);
_exit(42);
}
printf("%d: fork()=%d\n", (int)getpid(), (int)pid);
sleep(1);
const int n = 10;
printf("SIGALRM x %d\n", n); fflush(stdout);
for (int i = 0; i < n; i++) {
sched_yield();
if (kill(pid, SIGALRM))
perror("SIGALRM");
}
printf("SIGTERM\n"); fflush(stdout);
if (kill(pid, SIGTERM))
perror("SIGTERM");
printf("waitpid()...\n"); fflush(stdout);
int status = -1;
int wp = waitpid(pid, &status, 0);
printf("waidpid()=%d, status=0x%04x\n", wp, status);
return 0;
}
--
Takashi Yano <takashi.y...@nifty.ne.jp>
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 18a566c45..bd44222b5 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1670,6 +1670,8 @@ _cygtls::call_signal_handler ()
break;
}
+ InterlockedIncrement(&ctx_cnt);
+
/* Pop the stack if the next "return address" is sigdelayed, since
this function is doing what sigdelayed would have done anyway. */
if (retaddr () == (__tlsstack_t) sigdelayed)
@@ -1697,10 +1699,10 @@ _cygtls::call_signal_handler ()
/* Only make a context for SA_SIGINFO handlers */
if (this_sa_flags & SA_SIGINFO)
{
- context.uc_link = 0;
- context.uc_flags = 0;
+ context[ctx_cnt - 1].uc_link = 0;
+ context[ctx_cnt - 1].uc_flags = 0;
if (thissi.si_cyg)
- memcpy (&context.uc_mcontext,
+ memcpy (&context[ctx_cnt - 1].uc_mcontext,
((cygwin_exception *) thissi.si_cyg)->context (),
sizeof (CONTEXT));
else
@@ -1710,13 +1712,13 @@ _cygtls::call_signal_handler ()
from sigdelayed, fix the instruction pointer accordingly. */
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
- RtlCaptureContext ((PCONTEXT) &context.uc_mcontext);
+ RtlCaptureContext ((PCONTEXT) &context[ctx_cnt - 1].uc_mcontext);
#pragma GCC diagnostic pop
- __unwind_single_frame ((PCONTEXT) &context.uc_mcontext);
+ __unwind_single_frame ((PCONTEXT) &context[ctx_cnt -
1].uc_mcontext);
if (stackptr > stack)
{
#ifdef __x86_64__
- context.uc_mcontext.rip = retaddr ();
+ context[ctx_cnt - 1].uc_mcontext.rip = retaddr ();
#else
#error unimplemented for this target
#endif
@@ -1727,30 +1729,30 @@ _cygtls::call_signal_handler ()
&& !_my_tls.altstack.ss_flags
&& _my_tls.altstack.ss_sp)
{
- context.uc_stack = _my_tls.altstack;
- context.uc_stack.ss_flags = SS_ONSTACK;
+ context[ctx_cnt - 1].uc_stack = _my_tls.altstack;
+ context[ctx_cnt - 1].uc_stack.ss_flags = SS_ONSTACK;
}
else
{
- context.uc_stack.ss_sp = NtCurrentTeb ()->Tib.StackBase;
- context.uc_stack.ss_flags = 0;
+ context[ctx_cnt - 1].uc_stack.ss_sp = NtCurrentTeb
()->Tib.StackBase;
+ context[ctx_cnt - 1].uc_stack.ss_flags = 0;
if (!NtCurrentTeb ()->DeallocationStack)
- context.uc_stack.ss_size
+ context[ctx_cnt - 1].uc_stack.ss_size
= (uintptr_t) NtCurrentTeb ()->Tib.StackLimit
- (uintptr_t) NtCurrentTeb ()->Tib.StackBase;
else
- context.uc_stack.ss_size
+ context[ctx_cnt - 1].uc_stack.ss_size
= (uintptr_t) NtCurrentTeb ()->DeallocationStack
- (uintptr_t) NtCurrentTeb ()->Tib.StackBase;
}
- context.uc_sigmask = context.uc_mcontext.oldmask = this_oldmask;
+ context[ctx_cnt - 1].uc_sigmask = context[ctx_cnt -
1].uc_mcontext.oldmask = this_oldmask;
- context.uc_mcontext.cr2 = (thissi.si_signo == SIGSEGV
+ context[ctx_cnt - 1].uc_mcontext.cr2 = (thissi.si_signo == SIGSEGV
|| thissi.si_signo == SIGBUS)
? (uintptr_t) thissi.si_addr : 0;
- thiscontext = &context;
- context_copy = context;
+ thiscontext = &context[ctx_cnt - 1];
+ context_copy = context[ctx_cnt - 1];
}
int this_errno = saved_errno;
@@ -1836,9 +1838,11 @@ _cygtls::call_signal_handler ()
incyg = true;
set_signal_mask (_my_tls.sigmask, (this_sa_flags & SA_SIGINFO)
- ? context.uc_sigmask : this_oldmask);
+ ? context[ctx_cnt - 1].uc_sigmask :
this_oldmask);
if (this_errno >= 0)
set_errno (this_errno);
+
+ InterlockedDecrement(&ctx_cnt);
}
/* FIXME: Since 2011 this return statement always returned 1 (meaning
@@ -1863,12 +1867,12 @@ _cygtls::signal_debugger (siginfo_t& si)
{
if (incyg)
c._CX_instPtr = retaddr ();
- memcpy (&context.uc_mcontext, &c, sizeof (CONTEXT));
+ memcpy (&context[ctx_cnt].uc_mcontext, &c, sizeof (CONTEXT));
/* Enough space for 64 bit addresses */
char sigmsg[2 * sizeof (_CYGWIN_SIGNAL_STRING
" ffffffff ffffffffffffffff")];
__small_sprintf (sigmsg, _CYGWIN_SIGNAL_STRING " %d %y %p",
- si.si_signo, thread_id, &context.uc_mcontext);
+ si.si_signo, thread_id,
&context[ctx_cnt].uc_mcontext);
OutputDebugString (sigmsg);
}
ResumeThread (th);
diff --git a/winsup/cygwin/local_includes/cygtls.h
b/winsup/cygwin/local_includes/cygtls.h
index dfd319843..4e251fcc9 100644
--- a/winsup/cygwin/local_includes/cygtls.h
+++ b/winsup/cygwin/local_includes/cygtls.h
@@ -188,7 +188,8 @@ public: /* Do NOT remove this public: line, it's a marker
for gentls_offsets. */
If you prepend cygtls members here, make sure context stays 16 byte
aligned. The gentls_offsets script checks for that now and fails
if the alignment is wrong. */
- ucontext_t context;
+ ucontext_t context[4];
+ LONG ctx_cnt;
DWORD thread_id;
siginfo_t infodata;
struct pthread *tid;
