make signal handlers more reliable

make signal handlers more reliable

[Bruno Haible]

Here is a proposed patch to make signal handlers defined in and outside of
gnulib more reliable. Signal handlers are forbidden to call specific POSIX
functions (such as malloc(), strdup(), and sprintf()), and there are also
recommendations regarding 'volatile'.

To make things more reliable, this patch:
1) Defines a macro _GL_ASYNC_SAFE, to be used on all functions to which
   these restrictions apply, and states the rules.
2) Marks specific functions with _GL_ASYNC_SAFE, as required.
3) Fixes a bug in 'c-stack': It is currently using getprogname() in a
   signal handler. But getprogname() calls malloc(), strdup(), or sprintf()
   on various platforms.


2019-03-18  Bruno Haible  <bruno@clisp.org>

	Make signal handlers more reliable.
	* m4/gnulib-common.m4 (gl_COMMON_BODY): Emit definition of
	_GL_ASYNC_SAFE into config.h.
	* lib/nanosleep.c (sighandler): Mark as _GL_ASYNC_SAFE.
	* lib/fatal-signal.h (at_fatal_signal): Add _GL_ASYNC_SAFE marker to
	argument.
	* lib/fatal-signal.c (action_t, uninstall_handlers,
	fatal_signal_handler): Mark as _GL_ASYNC_SAFE.
	* lib/clean-temp.c (cleanup_action): Mark as _GL_ASYNC_SAFE.
	* lib/wait-process.c (cleanup_slaves, cleanup_slaves_action): Mark as
	_GL_ASYNC_SAFE.
	* lib/c-stack.h (c_stack_action): Add _GL_ASYNC_SAFE marker to argument.
	* lib/c-stack.c: Add _GL_ASYNC_SAFE markers.
	(progname): New variable.
	(die): Use it.
	(c_stack_action): Initialize it.

diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4
index 688a1e5..b0990f2 100644
--- a/m4/gnulib-common.m4
+++ b/m4/gnulib-common.m4
@@ -1,4 +1,4 @@
-# gnulib-common.m4 serial 42
+# gnulib-common.m4 serial 43
 dnl Copyright (C) 2007-2019 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -86,6 +86,27 @@ AC_DEFUN([gl_COMMON_BODY], [
 # define _GL_ATTRIBUTE_MALLOC /* empty */
 #endif
 ])
+  AH_VERBATIM([async_safe],
+[/* The _GL_ASYNC_SAFE marker should be attached to functions that are
+   signal handlers (for signals other than SIGABRT, SIGPIPE) or can be
+   invoked from such signal handlers.  Such functions have some restrictions:
+     * All functions that it calls should be marked _GL_ASYNC_SAFE as well,
+       or should be listed as async-signal-safe in POSIX
+       <http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04>
+       section 2.4.3.  Note that malloc() and fprintf(), in particular, are
+       NOT async-signal-safe.
+     * All memory locations (variables and struct fields) that these functions
+       access must be marked 'volatile'.  This holds for both read and write
+       accesses.  Otherwise the compiler might optimize away stores to and
+       reads from such locations that occur in the program, depending on its
+       data flow analysis.  For example, when the program contains a loop
+       that is intended to inspect a variable set from within a signal handler
+           while (!signal_occurred)
+             ;
+       the compiler is allowed to transform this into an endless loop if the
+       variable 'signal_occurred' is not declared 'volatile'.  */
+#define _GL_ASYNC_SAFE
+])
   dnl Preparation for running test programs:
   dnl Tell glibc to write diagnostics from -D_FORTIFY_SOURCE=2 to stderr, not
   dnl to /dev/tty, so they can be redirected to log files.  Such diagnostics
diff --git a/lib/nanosleep.c b/lib/nanosleep.c
index 6f6cc89..3150ff9 100644
--- a/lib/nanosleep.c
+++ b/lib/nanosleep.c
@@ -194,7 +194,7 @@ static sig_atomic_t volatile suspended;
 
 /* Handle SIGCONT. */
 
-static void
+static _GL_ASYNC_SAFE void
 sighandler (int sig)
 {
   suspended = 1;
diff --git a/lib/fatal-signal.h b/lib/fatal-signal.h
index c7d8e35..af1df23 100644
--- a/lib/fatal-signal.h
+++ b/lib/fatal-signal.h
@@ -51,7 +51,7 @@ extern "C" {
    The cleanup function is executed asynchronously.  It is unspecified
    whether during its execution the catchable fatal signals are blocked
    or not.  */
-extern void at_fatal_signal (void (*function) (int sig));
+extern void at_fatal_signal (_GL_ASYNC_SAFE void (*function) (int sig));
 
 
 /* Sometimes it is necessary to block the usually fatal signals while the
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 7f12f12..c9153f1 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -107,7 +107,7 @@ init_fatal_signals (void)
 /* ========================================================================= */
 
 
-typedef void (*action_t) (int sig);
+typedef _GL_ASYNC_SAFE void (*action_t) (int sig);
 
 /* Type of an entry in the actions array.
    The 'action' field is accessed from within the fatal_signal_handler(),
@@ -131,7 +131,7 @@ static struct sigaction saved_sigactions[64];
 
 
 /* Uninstall the handlers.  */
-static void
+static _GL_ASYNC_SAFE void
 uninstall_handlers (void)
 {
   size_t i;
@@ -148,7 +148,7 @@ uninstall_handlers (void)
 
 
 /* The signal handler.  It gets called asynchronously.  */
-static void
+static _GL_ASYNC_SAFE void
 fatal_signal_handler (int sig)
 {
   for (;;)
diff --git a/lib/clean-temp.c b/lib/clean-temp.c
index 0cb4a7e..d333f56 100644
--- a/lib/clean-temp.c
+++ b/lib/clean-temp.c
@@ -180,7 +180,7 @@ string_hash (const void *x)
 
 
 /* The signal handler.  It gets called asynchronously.  */
-static void
+static _GL_ASYNC_SAFE void
 cleanup_action (int sig _GL_UNUSED)
 {
   size_t i;
diff --git a/lib/wait-process.c b/lib/wait-process.c
index b51e244..7c69933 100644
--- a/lib/wait-process.c
+++ b/lib/wait-process.c
@@ -80,7 +80,7 @@ static size_t slaves_allocated = SIZEOF (static_slaves);
 #endif
 
 /* The cleanup action.  It gets called asynchronously.  */
-static void
+static _GL_ASYNC_SAFE void
 cleanup_slaves (void)
 {
   for (;;)
@@ -104,7 +104,7 @@ cleanup_slaves (void)
 
 /* The cleanup action, taking a signal argument.
    It gets called asynchronously.  */
-static void
+static _GL_ASYNC_SAFE void
 cleanup_slaves_action (int sig _GL_UNUSED)
 {
   cleanup_slaves ();
diff --git a/lib/c-stack.h b/lib/c-stack.h
index fcfe66f..36dd4ec 100644
--- a/lib/c-stack.h
+++ b/lib/c-stack.h
@@ -41,4 +41,4 @@
    This function may install a handler for the SIGSEGV signal or for the SIGBUS
    signal or exercise other system dependent exception handling APIs.  */
 
-extern int c_stack_action (void (* /*action*/) (int));
+extern int c_stack_action (_GL_ASYNC_SAFE void (* /*action*/) (int));
diff --git a/lib/c-stack.c b/lib/c-stack.c
index c62726b..c0018a4 100644
--- a/lib/c-stack.c
+++ b/lib/c-stack.c
@@ -91,7 +91,7 @@ typedef struct sigaltstack stack_t;
 
 /* The user-specified action to take when a SEGV-related program error
    or stack overflow occurs.  */
-static void (* volatile segv_action) (int);
+static _GL_ASYNC_SAFE void (* volatile segv_action) (int);
 
 /* Translated messages for program errors and stack overflow.  Do not
    translate them in the signal handler, since gettext is not
@@ -107,7 +107,9 @@ static char const * volatile stack_overflow_message;
    appears to have been a stack overflow, or with a core dump
    otherwise.  This function is async-signal-safe.  */
 
-static _Noreturn void
+static char const * volatile progname;
+
+static _GL_ASYNC_SAFE _Noreturn void
 die (int signo)
 {
   char const *message;
@@ -119,7 +121,7 @@ die (int signo)
 #endif /* !SIGINFO_WORKS && !HAVE_LIBSIGSEGV */
   segv_action (signo);
   message = signo ? program_error_message : stack_overflow_message;
-  ignore_value (write (STDERR_FILENO, getprogname (), strlen (getprogname ())));
+  ignore_value (write (STDERR_FILENO, progname, strlen (progname)));
   ignore_value (write (STDERR_FILENO, ": ", 2));
   ignore_value (write (STDERR_FILENO, message, strlen (message)));
   ignore_value (write (STDERR_FILENO, "\n", 1));
@@ -146,8 +148,8 @@ static union
   void *p;
 } alternate_signal_stack;
 
-static void
-null_action (int signo __attribute__ ((unused)))
+static _GL_ASYNC_SAFE void
+null_action (int signo _GL_UNUSED)
 {
 }
 
@@ -164,8 +166,8 @@ static volatile int segv_handler_missing;
 /* Handle a segmentation violation and exit if it cannot be stack
    overflow.  This function is async-signal-safe.  */
 
-static int segv_handler (void *address __attribute__ ((unused)),
-                         int serious)
+static _GL_ASYNC_SAFE int
+segv_handler (void *address _GL_UNUSED, int serious)
 {
 # if DEBUG
   {
@@ -185,9 +187,8 @@ static int segv_handler (void *address __attribute__ ((unused)),
 /* Handle a segmentation violation that is likely to be a stack
    overflow and exit.  This function is async-signal-safe.  */
 
-static _Noreturn void
-overflow_handler (int emergency,
-                  stackoverflow_context_t context __attribute__ ((unused)))
+static _GL_ASYNC_SAFE _Noreturn void
+overflow_handler (int emergency, stackoverflow_context_t context _GL_UNUSED)
 {
 # if DEBUG
   {
@@ -202,11 +203,12 @@ overflow_handler (int emergency,
 }
 
 int
-c_stack_action (void (*action) (int))
+c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
 {
   segv_action = action ? action : null_action;
   program_error_message = _("program error");
   stack_overflow_message = _("stack overflow");
+  progname = getprogname ();
 
   /* Always install the overflow handler.  */
   if (stackoverflow_install_handler (overflow_handler,
@@ -229,9 +231,8 @@ c_stack_action (void (*action) (int))
 /* Handle a segmentation violation and exit.  This function is
    async-signal-safe.  */
 
-static _Noreturn void
-segv_handler (int signo, siginfo_t *info,
-              void *context __attribute__ ((unused)))
+static _GL_ASYNC_SAFE _Noreturn void
+segv_handler (int signo, siginfo_t *info, void *context _GL_UNUSED)
 {
   /* Clear SIGNO if it seems to have been a stack overflow.  */
 #  if ! HAVE_XSI_STACK_OVERFLOW_HEURISTIC
@@ -278,7 +279,7 @@ segv_handler (int signo, siginfo_t *info,
 # endif
 
 int
-c_stack_action (void (*action) (int))
+c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
 {
   int r;
   stack_t st;
@@ -300,6 +301,7 @@ c_stack_action (void (*action) (int))
   segv_action = action ? action : null_action;
   program_error_message = _("program error");
   stack_overflow_message = _("stack overflow");
+  progname = getprogname ();
 
   sigemptyset (&act.sa_mask);
 
@@ -325,7 +327,7 @@ c_stack_action (void (*action) (int))
              && HAVE_STACK_OVERFLOW_HANDLING) || HAVE_LIBSIGSEGV) */
 
 int
-c_stack_action (void (*action) (int)  __attribute__ ((unused)))
+c_stack_action (_GL_ASYNC_SAFE void (*action) (int)  _GL_UNUSED)
 {
   errno = ENOTSUP;
   return -1;

Re: make signal handlers more reliable

[Paul Eggert]

Thanks, I like the idea of marking signal handlers, presumably for use
by static checkers later. And it's nice that you found the bug in c-stack.

Did you use static checking to find the bug? Is there some reasonable
way to cajole GCC into doing this checking?

Re: make signal handlers more reliable

[Bruno Haible]

Hi Paul,

> Thanks, I like the idea of marking signal handlers, presumably for use
> by static checkers later. And it's nice that you found the bug in c-stack.

Thanks.

> Did you use static checking to find the bug?

No. The signal handlers are so small in size that I could afford to review
their function calls and memory accesses line-by-line.

> Is there some reasonable way to cajole GCC into doing this checking?

A web search for "static analysis" and "signal handler" reveals a couple
of research papers, but I did not immediately find anything based on GCC
or clang.

Bruno

Re: make signal handlers more reliable

[Kamil Dudka]

On Tuesday, March 19, 2019 3:46:54 AM CET Paul Eggert wrote:
> Is there some reasonable way to cajole GCC into doing this checking?

There is a work-in-progress GCC plug-in that checks signal handlers
for uses of functions that are not async-signal-safe:

https://github.com/dkozovsk/handler_plug
https://copr.fedorainfracloud.org/coprs/dkozovsk/handler_plug

But the plug-in is going to be renamed soon.

Kamil

Re: make signal handlers more reliable

[Bruno Haible]

Hi Paul,

> it's nice that you found the bug in c-stack.

There was also another bug in c-stack: The SIGSEGV handler, in the case
where the stack overflow is not yet serious, clobbered errno before
returning.

Fixed both together like this:


2019-03-19  Bruno Haible  <bruno@clisp.org>

	c-stack: Make signal handlers more reliable.
	* lib/c-stack.c (progname): New variable.
	(die): Use it.
	(c_stack_action): Initialize it.
	(segv_handler): Save and restore errno.

diff --git a/lib/c-stack.c b/lib/c-stack.c
index 929bb3a..f50a4a5 100644
--- a/lib/c-stack.c
+++ b/lib/c-stack.c
@@ -107,6 +107,8 @@ static char const * volatile stack_overflow_message;
    appears to have been a stack overflow, or with a core dump
    otherwise.  This function is async-signal-safe.  */
 
+static char const * volatile progname;
+
 static _GL_ASYNC_SAFE _Noreturn void
 die (int signo)
 {
@@ -119,7 +121,7 @@ die (int signo)
 #endif /* !SIGINFO_WORKS && !HAVE_LIBSIGSEGV */
   segv_action (signo);
   message = signo ? program_error_message : stack_overflow_message;
-  ignore_value (write (STDERR_FILENO, getprogname (), strlen (getprogname ())));
+  ignore_value (write (STDERR_FILENO, progname, strlen (progname)));
   ignore_value (write (STDERR_FILENO, ": ", 2));
   ignore_value (write (STDERR_FILENO, message, strlen (message)));
   ignore_value (write (STDERR_FILENO, "\n", 1));
@@ -170,8 +172,10 @@ segv_handler (void *address _GL_UNUSED, int serious)
 # if DEBUG
   {
     char buf[1024];
+    int saved_errno = errno;
     sprintf (buf, "segv_handler serious=%d\n", serious);
     write (STDERR_FILENO, buf, strlen (buf));
+    errno = saved_errno;
   }
 # endif
 
@@ -206,6 +210,7 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   segv_action = action ? action : null_action;
   program_error_message = _("program error");
   stack_overflow_message = _("stack overflow");
+  progname = getprogname ();
 
   /* Always install the overflow handler.  */
   if (stackoverflow_install_handler (overflow_handler,
@@ -298,6 +303,7 @@ c_stack_action (_GL_ASYNC_SAFE void (*action) (int))
   segv_action = action ? action : null_action;
   program_error_message = _("program error");
   stack_overflow_message = _("stack overflow");
+  progname = getprogname ();
 
   sigemptyset (&act.sa_mask);