Re: RFC: sigaction module

[Bruno Haible]

Hi Eric,

> I've gone ahead and done an reduced implementation of sigaction for mingw,
> getting the parts that were easy to implement and which were exercised within 
> other gnulib modules.

That's fully sufficient. Nice work!

> The implementation is NOT async-signal-safe, but since 
> it seems that mingw is the only platform lacking a native sigaction, this 
> does 
> not seem to be much of a loss

Remember that there are at least two async signals: Ctrl-C leading to SIGINT,
and also Ctrl-Break leading to SIGBREAK.

> mingw lacks sigprocmask.  There is a gnulib module replacement, but it too
> suffers from the race condition.

The sigprocmask replacement may, when a signal arrives just after it has
been unblocked, still execute the handler for this signal. This is more
benign than the race condition that you introduce here: When a signal
arrives between the
   signal (sig, SIG_DFL)
and
   signal (sig, sigaction_handler)
lines, the signal will undergo default handling. Executing either the old
or the new handler would be excusable, but not an open race like this.

> +Posix recommends using @code{sigaction} with SA_NODEFER instead.

s/Posix/POSIX/, no?

> +/* We assume that a platform without POSIX sigaction implements SysV
> +   semantics in signal() (ie. the handler is uninstalled before it is
> +   invoked).

Yes, this is how msvcrt behaves.

> +static struct sigaction action_array[NSIG] /* = 0 */;

This array needs to be marked volatile, because you access it from a
signal handler. (You must forbid the compiler to reorder statements in
the sigaction() function in a non-obvious way.)

> +/* Signal handler that is installed for signals.  */
> +static void
> +sigaction_handler (int sig)
> +{
> +  handler_t h;
> +  sigset_t oldmask;
> +  sigset_t mask;
> +  if (sig < 0 || NSIG <= sig || !action_array[sig].sa_handler)

If you need to access the action_array from within the handler, you need to
change the statements

          if (signal (sig, sigaction_handler) == SIG_ERR)
            return -1;
          /* Consider what happens if a signal arrives ---HERE--- */
          action_array[sig] = *act;

to

          action_array[sig] = *act;
          if (signal (sig, sigaction_handler) == SIG_ERR)
            return -1;

> +    {
> +      /* Unexpected situation; be careful to avoid recursive abort.  */
> +      if (sig == SIGABRT)
> +        exit (1);
> +      abort ();

I don't like exit(1) here, because it leaves no clue where to look when it
really occurs. Why not like this:

         if (sig == SIGABRT)
           signal (sig, SIG_DFL);
         abort ();

> +  /* Reinstall the signal handler when required.  Do this prior to
> +     blocking any signals, since sigprocmask replacement doesn't like
> +     changing a blocked signal.  */
> +  h = action_array[sig].sa_handler;
> +  if ((action_array[sig].sa_flags & SA_RESETHAND) == 0)
> +    signal (sig, sigaction_handler);
> +  else
> +    action_array[sig].sa_handler = NULL;
> +
> +  /* Block appropriate signals.  */
> +  mask = action_array[sig].sa_mask;
> +  if ((action_array[sig].sa_flags & SA_NODEFER) == 0)
> +    sigaddset (&mask, sig);
> +  sigprocmask (SIG_BLOCK, &mask, &oldmask);
> +
> +  /* Invoke the user's handler, then restore prior mask.  */
> +  h (sig);
> +  sigprocmask (SIG_SETMASK, &oldmask, NULL);
> +}

Looks good.

> +  if (oact)
> +    {
> +      if (action_array[sig].sa_handler)
> +        *oact = action_array[sig];
> +      else
> +        {
> +          /* This opens a slight window where an async signal can call
> +             wrong handler.  Oh well.  */
> +          oact->sa_handler = signal (sig, SIG_DFL);
> +          signal (sig, oact->sa_handler);
> +          if (oact->sa_handler == SIG_ERR)
> +            return -1;
> +          oact->sa_flags = SA_RESETHAND | SA_NODEFER;
> +          sigemptyset (&oact->sa_mask);
> +        }
> +    }
> +
> +  if (act)
> +    {
> +      if (act->sa_handler == SIG_DFL || act->sa_handler == SIG_IGN)
> +        {
> +          if (signal (sig, act->sa_handler) == SIG_ERR)
> +            return -1;
> +          action_array[sig].sa_handler = NULL;
> +        }
> +      else
> +        {
> +          if (signal (sig, sigaction_handler) == SIG_ERR)
> +            return -1;

Like Paul, I would like to see atomicity here. Can you merge the two signal()
calls, to guarantee atomicity of the change?

> +          action_array[sig] = *act;

This statement is not atomic, because action_array[sig] consists of
several memory words. Yet you access all three words from the signal handler.
What happens if a signal occurs right in the middle of this copying operation?

> +# if address@hidden@
> +/* Present to allow compilation, but unsupported by gnulib.  */
> +union sigval
> +{
> +  int sival_int;
> +  void *sival_ptr;
> +};
> +
> +struct siginfo_t
> +{
> +  int si_signo;
> +  int si_code;
> +  int si_errno;
> +  pid_t si_pid;
> +  uid_t si_uid;
> +  void *si_addr;
> +  int si_status;
> +  long si_band;
> +  union sigval si_value;
> +};

I think this requires a #include <sys/types.h>.

> --- a/lib/fatal-signal.c
> +++ b/lib/fatal-signal.c
> @@ -1,5 +1,5 @@
>  /* Emergency actions in case of a fatal signal.
> -   Copyright (C) 2003-2004, 2006-2007 Free Software Foundation, Inc.
> +   Copyright (C) 2003-2004, 2006-2008 Free Software Foundation, Inc.
>     Written by Bruno Haible <address@hidden>, 2003.
>  
>     This program is free software: you can redistribute it and/or modify
> @@ -30,6 +30,9 @@
>  
>  #define SIZEOF(a) (sizeof(a) / sizeof(a[0]))
>  
> +#ifndef SA_SIGINFO
> +# define SA_SIGINFO 0
> +#endif
>  
>  /* ========================================================================= 
> */
>  
> @@ -88,7 +91,6 @@ init_fatal_signals (void)
>    static bool fatal_signals_initialized = false;
>    if (!fatal_signals_initialized)
>      {
> -#if HAVE_SIGACTION
>        size_t i;
>  
>        for (i = 0; i < num_fatal_signals; i++)
> @@ -96,11 +98,10 @@ init_fatal_signals (void)
>         struct sigaction action;
>  
>         if (sigaction (fatal_signals[i], NULL, &action) >= 0
> +              && (action.sa_flags & SA_SIGINFO) == 0
>             && action.sa_handler == SIG_IGN)
>           fatal_signals[i] = -1;
>       }
> -#endif
> -

What does this SA_SIGINFO business mean? Why do you need to check whether
(sa_flags & SA_SIGINFO) == 0 when sa_handler is known to be SIG_IGN?

Bruno