[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: RFC: sigaction module
From: |
Bruno Haible |
Subject: |
Re: RFC: sigaction module |
Date: |
Wed, 18 Jun 2008 03:17:50 +0200 |
User-agent: |
KMail/1.5.4 |
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
- RFC: sigaction module, Eric Blake, 2008/06/16
- Re: RFC: sigaction module, Paul Eggert, 2008/06/16
- Re: RFC: sigaction module,
Bruno Haible <=
- Re: RFC: sigaction module, Eric Blake, 2008/06/17
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Bruno Haible, 2008/06/17
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Eric Blake, 2008/06/17
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Bruno Haible, 2008/06/18
- RE: sigaction, SA_SIGINFO, and SIG_IGN, Jason Zions, 2008/06/18
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Bruno Haible, 2008/06/19
- Re: Interix, Bruno Haible, 2008/06/19
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Paul Eggert, 2008/06/19
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Bruno Haible, 2008/06/19
- Re: sigaction, SA_SIGINFO, and SIG_IGN, Paul Eggert, 2008/06/20