Defer signals by hand with sigatomic_t

Refactor by using a sig_atomic_t variable instead of a sigprocmask
call to defer signals.  This should be good enough for a
single-thread app where we write all the code that needs critical
sections, and where the signal handler merely cleans up and exits.
The resulting code should have the same behavior (including
signal-handling races!) as the original.
* bootstrap.conf (gnulib_modules): Remove sigaction, sigprocmask.
Instead, use sigaction only if it’s supported natively,
as the Gnulib emulation of sigaction drags in code we no longer need.
* configure.ac: Check for sigaction, sigfillset.
* src/patch.c (fatal_cleanup): New async-signal-safe function,
which does the cleanup that the old fatal_exit (SIG) did when SIG
was nonzero.
(fatal_exit): Do what the old fatal_exit (SIG) did when SIG was zero.
Omit SIG arg.  All callers changed.  This function is no longer
called from signal handlers, and so no longer needs to be
async-signal-safe under some circumstances.  However, it now
defers signals.
* src/util.c (signal_received): New static var.
(signal_deferring_level): Now sig_atomic_t.
(fatal_cleanup_and_terminate, handle_signal): New functions.
(defer_signals, undefer_signals): Reimplement by
using sigatomic_t volatile vars, not by using sigprocmask.
(init_signals): Don’t assume SIGPIPE since we don’t use the
Gnulib sigpipe module.  Use simple sigfillset signal mask
so that we needn’t use sigprocmask to inquire about the
current signal mask.  Have a fallback for old platforms
that lack sigaction and sigfillset, since we no longer use
Gnulib’s sigaction module.
(exit_with_signal): Remove; no longer needed.

bootstrap.conf

@@ -70,9 +70,7 @@ readlinkat
 realloc-gnu
 renameat
 setenv
-sigaction
 signal-h
-sigprocmask
 ssize_t
 stat-time
 stdbool

configure.ac

@@ -111,7 +111,7 @@ AC_TYPE_OFF_T
 
 gl_FUNC_XATTR
 
-AC_CHECK_FUNCS_ONCE([geteuid getuid])
+AC_CHECK_FUNCS_ONCE([geteuid getuid sigaction sigfillset])
 AC_FUNC_SETMODE_DOS
 
 AC_PATH_PROG([ED], [ed], [ed])

src/common.h

@@ -117,7 +117,8 @@ extern enum diff diff_type;
 
 extern char *revision;			/* prerequisite revision, if any */
 
-_Noreturn void fatal_exit (int);
+void fatal_cleanup (void);
+_Noreturn void fatal_exit (void);
 
 #if ! (HAVE_GETEUID || defined geteuid)
 # if ! (HAVE_GETUID || defined getuid)

src/patch.c

@@ -541,7 +541,7 @@ main (int argc, char **argv)
       }
 
       /* If not a dry run, defer signals.
-	 Otherwise, fatal_exit would misbehave when called from a
+	 Otherwise, fatal_cleanup would misbehave when called from a
 	 signal handler invoked while the following code was being run.
 	 FIXME: The following code does an unbounded amount of work
 	 while signals are deferred, which is a bad thing.  */
@@ -2005,20 +2005,23 @@ output_files (struct stat const *st, int exiting)
     }
 }
 
-/* Fatal exit with cleanup.  If SIG, this is in response to the signal SIG.  */
+/* Clean up temp files after a signal.  This function is async-signal-safe.  */
 
 void
-fatal_exit (int sig)
+fatal_cleanup (void)
 {
-  /* Defer signals until program exit.  However, there is no need to
-     defer in a signal handler, as it has already blocked signals.  */
-  if (!sig)
-    defer_signals ();
-
   cleanup ();
-  output_files (nullptr, sig ? -1 : 1);
-  if (sig)
-    exit_with_signal (sig);
+  output_files (nullptr, -1);
+}
+
+/* Clean up temp files and exit after a fatal error.  */
+
+void
+fatal_exit (void)
+{
+  defer_signals ();
+  cleanup ();
+  output_files (nullptr, 1);
   exit (EXIT_TROUBLE);
 }
 

src/util.c

@@ -354,7 +354,7 @@ set_file_attributes (char *to, int tofd, enum file_attributes attr,
     if (copy_attr (from, fromfd, to, tofd) < 0
 	&& errno != ENOSYS && errno != ENOTSUP
 	&& lacks_appropriate_privileges (errno, tofd))
-      fatal_exit (0);
+      fatal_exit ();
   if (attr & FA_MODE)
     {
       /* The "diff --git" format does not store the file permissions of
@@ -982,7 +982,7 @@ fatal (char const *format, ...)
   va_end (args);
   fputc ('\n', stderr);
   fflush (stderr);
-  fatal_exit (0);
+  fatal_exit ();
 }
 
 void
@@ -1030,7 +1030,7 @@ pfatal (char const *format, ...)
   va_end (args);
   fprintf (stderr, " : %s\n", strerror (errnum));
   fflush (stderr);
-  fatal_exit (0);
+  fatal_exit ();
 }
 
 /* Tell the user something.  */
@@ -1169,34 +1169,69 @@ ok_to_reverse (char const *format, ...)
 
 /* Critical sections and signals.  */
 
-/* How to handle signals.  fatal_act.sa_mask lists signals to be
-   blocked when handling signals or in a critical section.  */
-static struct sigaction fatal_act = { .sa_handler = fatal_exit };
+/* If nonzero, the corresponding signal was the first one received.
+   Otherwise, no signal has been received.  */
+static sig_atomic_t volatile signal_received;
 
 /* Whether signals should be deferred, and if so how deep the
    nesting level of deferring is.  */
-static intmax_t signal_deferring_level;
+static sig_atomic_t signal_deferring_level;
+
+/* Signal SIG has arrived and can be acted on now.
+   Clean up and arrange to terminate.
+   This function is async-signal-safe.  */
+static void
+fatal_cleanup_and_terminate (int sig)
+{
+  fatal_cleanup ();
+
+  /* Cause the program to terminate with signal SIG.  Termination occurs now
+     if not in a signal handler, otherwise when the signal handler returns.  */
+  signal (sig, SIG_DFL);
+  raise (sig);
+}
+
+/* Handle a delivered signal.  While this function is running,
+   all signals that 'patch' cares about are blocked.
+   This function is async-signal-safe.  */
+static void
+handle_signal (int sig)
+{
+#if ! (HAVE_SIGACTION && HAVE_SIGFILLSET)
+  /* Cater to non-POSIX systems that lack sigaction and reset to SIG_DFL.  */
+  signal (sig, SIG_IGN);
+#endif
+
+  /* If multiple signals arrive, handle just the first one.
+     That's good enough, as 'patch' is about to exit.  */
+  if (signal_received)
+    return;
+  signal_received = sig;
+
+  if (!signal_deferring_level)
+    {
+      signal_deferring_level = 1;	/* Avoid infinite recursion.  */
+      fatal_cleanup_and_terminate (sig);
+    }
+}
 
 /* Defer incoming signals until later.  */
 void
 defer_signals (void)
 {
-  intmax_t s = signal_deferring_level;
-  if (!s)
-    sigprocmask (SIG_BLOCK, &fatal_act.sa_mask, nullptr);
-  signal_deferring_level = s + 1;
+  signal_deferring_level++;
 }
 
 /* Handle any signals that have arrived, and stop deferring them.  */
 void
 undefer_signals (void)
 {
-  signal_deferring_level--;
-  if (!signal_deferring_level)
+  assert (0 < signal_deferring_level);
+  if (!--signal_deferring_level)
     {
-      int e = errno;
-      sigprocmask (SIG_UNBLOCK, &fatal_act.sa_mask, nullptr);
-      errno = e;
+      int sig = signal_received;
+      if (sig)
+	fatal_cleanup_and_terminate (sig);
     }
 }
 
@@ -1207,7 +1242,9 @@ init_signals (void)
     {
       SIGHUP,
       SIGINT,
+#ifdef SIGPIPE
       SIGPIPE,
+#endif
 #ifdef SIGTERM
       SIGTERM,
 #endif
@@ -1223,35 +1260,21 @@ init_signals (void)
   /* System V fork+wait does not work if SIGCHLD is ignored.  */
   signal (SIGCHLD, SIG_DFL);
 
-  sigset_t initial_signal_mask;
-  if (sigprocmask (SIG_BLOCK, nullptr, &initial_signal_mask) < 0)
-    return;
-
-  sigemptyset (&fatal_act.sa_mask);
+#if HAVE_SIGACTION && HAVE_SIGFILLSET
+  struct sigaction fatal_act = { .sa_handler = handle_signal };
+  sigfillset (&fatal_act.sa_mask);
   for (int i = 0; i < NUM_SIGS; i++)
     {
       struct sigaction initial_act;
-      if (!sigismember (&initial_signal_mask, sigs[i])
-	  && sigaction (sigs[i], nullptr, &initial_act) == 0
+      if (sigaction (sigs[i], nullptr, &initial_act) == 0
 	  && initial_act.sa_handler != SIG_IGN)
-	sigaddset (&fatal_act.sa_mask, sigs[i]);
+	sigaction (sigs[i], &fatal_act, nullptr);
     }
-
+#else
   for (int i = 0; i < NUM_SIGS; i++)
-    if (sigismember (&fatal_act.sa_mask, sigs[i]))
-      sigaction (sigs[i], &fatal_act, nullptr);
-}
-
-void
-exit_with_signal (int sig)
-{
-  sigset_t s;
-  signal (sig, SIG_DFL);
-  sigemptyset (&s);
-  sigaddset (&s, sig);
-  sigprocmask (SIG_UNBLOCK, &s, nullptr);
-  raise (sig);
-  exit (EXIT_TROUBLE);
+    if (signal (sigs[i], SIG_IGN) != SIG_IGN)
+      signal (sigs[i], handle_signal);
+#endif
 }
 
 int

src/util.h

@@ -89,7 +89,6 @@ void copy_file (char *, struct stat const *, struct outfile *, struct stat *,
 		int, mode_t, enum file_attributes, bool);
 void append_to_file (char *, char *);
 idx_t quote_system_arg (char *, char const *);
-_Noreturn void exit_with_signal (int);
 void init_signals (void);
 void defer_signals (void);
 void undefer_signals (void);