[PATCH] Cygwin: signa: Redesign signal queue handling

[Takashi Yano]

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.Franke@t-online.de>
> > > > > Reviewed-by:
> > > > > Signed-off-by: Takashi Yano <takashi.yano@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.yano@nifty.ne.jp>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: context.patch
URL: <https://cygwin.com/pipermail/cygwin-patches/attachments/20250311/98f3ca84/attachment.ksh>