Subject: Re: bug#32375: Bug Gzip v1.9

From: Paul Eggert <[email-masked]>
Date: Mon, 6 Aug 2018 14:47:43 -0700

Thanks for reporting the bug. Although I doubt whether it can occur on any 
practical platform, we should fix it even if it's just theoretical. I installed 
into gzip master on savannah the attached patch, which I hope fixes the problem; 
please give it a try.

The SYMBIOSYS tool did not discover a related theoretical bug with the 
'caught_signals' variable, which also is used inside a signal handler despite 
not being volatile, so you might want to look into that. The attached patch 
should fix this related bug too.

I am CC'ing this to Bdale Garbee, since much of this patch brings in a Gnulib 
replacement for sigaction that I expect nowadays is used only on mingw, and 
Bdale is my mingw guru (see Bug#32305).
From f9c96538a4b3458893f48ea73a1b45fd393805fd Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Mon, 6 Aug 2018 14:44:41 -0700
Subject: [PATCH] Fix some theoretical races in signal handling

Problem reported by Johannes Przybilla (Bug#32375).
* NEWS: Mention this.
* bootstrap.conf (gnulib_modules): Add sigaction.
* gzip.c (SA_NOCLDSTOP, sigprocmask, sigset_t)
(siginterrupt) [!SA_NOCLDSTOP]: Remove; Gnulib not supplies these.
(remove_ofname): New var.
(volatile_strcpy): New function.
(create_outfile): Use it.
(install_signal_handlers, abort_gzip_signal): Assume sigaction.
(remove_output_file): New arg SIGNALS_ALREADY_BLOCKED.
All uses changed.  Avoid unnecessary (and racy) call
to sigprocmask if this new arg is set.
(abort_gzip_signal): Assume C89 or better for signal handler type.
* gzip.h (RETSIGTYPE): Remove.
* lib/.gitignore, m4/.gitignore:
Add files brought in by Gnulib sigaction module.  Sort.
---
 NEWS           |  6 +++++
 bootstrap.conf |  1 +
 gzip.c         | 63 ++++++++++++++++++++------------------------------
 gzip.h         |  4 ----
 lib/.gitignore |  9 +++++++-
 m4/.gitignore  | 10 +++++---
 6 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/NEWS b/NEWS
index 49c2e9b..c3113ed 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,12 @@ GNU gzip NEWS                                    -*- outline -*-
   regular files will use null timestamps after the year 2106, due to a
   limitation in the gzip format.)
 
+** Bug fixes
+
+  A few theoretical race conditions in signal handers have been fixed.
+  These bugs most likely do not happen on practical platforms.
+  [bugs present since the beginning]
+
 
 * Noteworthy changes in release 1.9 (2018-01-07) [stable]
 
diff --git a/bootstrap.conf b/bootstrap.conf
index 8c4b075..76690fc 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -52,6 +52,7 @@ printf-posix
 readme-release
 realloc-gnu
 savedir
+sigaction
 stat-time
 stdnoreturn
 sys_stat
diff --git a/gzip.c b/gzip.c
index b26dd14..1c9bc6c 100644
--- a/gzip.c
+++ b/gzip.c
@@ -119,17 +119,6 @@ static char const *const license_msg[] = {
 # define OFF_T_MAX TYPE_MAXIMUM (off_t)
 #endif
 
-/* Use SA_NOCLDSTOP as a proxy for whether the sigaction machinery is
-   present.  */
-#ifndef SA_NOCLDSTOP
-# define SA_NOCLDSTOP 0
-# define sigprocmask(how, set, oset) /* empty */
-# define sigset_t int
-# if ! HAVE_SIGINTERRUPT
-#  define siginterrupt(sig, flag) /* empty */
-# endif
-#endif
-
 #ifndef HAVE_WORKING_O_NOFOLLOW
 # define HAVE_WORKING_O_NOFOLLOW 0
 #endif
@@ -211,8 +200,10 @@ static sigset_t caught_signals;
    suppresses a "Broken Pipe" message with some shells.  */
 static int volatile exiting_signal;
 
-/* If nonnegative, close this file descriptor and unlink ofname on error.  */
+/* If nonnegative, close this file descriptor and unlink remove_ofname
+   on error.  */
 static int volatile remove_ofname_fd = -1;
+static char volatile remove_ofname[MAX_PATH_LEN];
 
 static bool stdin_was_read;
 
@@ -323,8 +314,8 @@ local void do_list      (int ifd, int method);
 local int  check_ofname (void);
 local void copy_stat    (struct stat *ifstat);
 local void install_signal_handlers (void);
-local void remove_output_file (void);
-local RETSIGTYPE abort_gzip_signal (int);
+static void remove_output_file (bool);
+static void abort_gzip_signal (int);
 local noreturn void do_exit (int exitcode);
 static void finish_out (void);
       int main          (int argc, char **argv);
@@ -1062,7 +1053,7 @@ local void treat_file(iname)
 
     if (method == -1) {
         if (!to_stdout)
-          remove_output_file ();
+          remove_output_file (false);
         return;
     }
 
@@ -1082,6 +1073,13 @@ local void treat_file(iname)
     }
 }
 
+static void
+volatile_strcpy (char volatile *dst, char const *src)
+{
+  while ((*dst++ = *src++))
+    continue;
+}
+
 /* ========================================================================
  * Create the output file. Return OK or ERROR.
  * Try several times if necessary to avoid truncating the z_suffix. For
@@ -1115,6 +1113,8 @@ local int create_outfile()
       int open_errno;
       sigset_t oldset;
 
+      volatile_strcpy (remove_ofname, ofname);
+
       sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
       remove_ofname_fd = ofd = openat (atfd, base, flags, S_IRUSR | S_IWUSR);
       open_errno = errno;
@@ -2061,8 +2061,6 @@ install_signal_handlers ()
 {
   int nsigs = sizeof handled_sig / sizeof handled_sig[0];
   int i;
-
-#if SA_NOCLDSTOP
   struct sigaction act;
 
   sigemptyset (&caught_signals);
@@ -2084,16 +2082,6 @@ install_signal_handlers ()
           foreground = 1;
         sigaction (handled_sig[i], &act, NULL);
       }
-#else
-  for (i = 0; i < nsigs; i++)
-    if (signal (handled_sig[i], SIG_IGN) != SIG_IGN)
-      {
-        if (i == 0)
-          foreground = 1;
-        signal (handled_sig[i], abort_gzip_signal);
-        siginterrupt (handled_sig[i], 1);
-      }
-#endif
 }
 
 /* ========================================================================
@@ -2133,12 +2121,13 @@ finish_out (void)
  * Close and unlink the output file.
  */
 static void
-remove_output_file ()
+remove_output_file (bool signals_already_blocked)
 {
   int fd;
   sigset_t oldset;
 
-  sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+  if (!signals_already_blocked)
+    sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
   fd = remove_ofname_fd;
   if (0 <= fd)
     {
@@ -2146,29 +2135,27 @@ remove_output_file ()
       close (fd);
       xunlink (ofname);
     }
-  sigprocmask (SIG_SETMASK, &oldset, NULL);
+  if (!signals_already_blocked)
+    sigprocmask (SIG_SETMASK, &oldset, NULL);
 }
 
 /* ========================================================================
  * Error handler.
  */
 void
-abort_gzip ()
+abort_gzip (void)
 {
-   remove_output_file ();
+   remove_output_file (false);
    do_exit(ERROR);
 }
 
 /* ========================================================================
  * Signal handler.
  */
-static RETSIGTYPE
-abort_gzip_signal (sig)
-     int sig;
+static void
+abort_gzip_signal (int sig)
 {
-  if (! SA_NOCLDSTOP)
-    signal (sig, SIG_IGN);
-   remove_output_file ();
+   remove_output_file (true);
    if (sig == exiting_signal)
      _exit (WARNING);
    signal (sig, SIG_DFL);
diff --git a/gzip.h b/gzip.h
index 2499337..329c9a5 100644
--- a/gzip.h
+++ b/gzip.h
@@ -41,10 +41,6 @@
 #include <stdnoreturn.h>
 #define memzero(s, n) memset ((voidp)(s), 0, (n))
 
-#ifndef RETSIGTYPE
-#  define RETSIGTYPE void
-#endif
-
 #define local static
 
 typedef unsigned char  uch;
diff --git a/lib/.gitignore b/lib/.gitignore
index cd11296..6a21391 100644
--- a/lib/.gitignore
+++ b/lib/.gitignore
@@ -146,6 +146,7 @@
 /printf-parse.c
 /printf-parse.h
 /printf.c
+/raise.c
 /readdir.c
 /realloc.c
 /rmdir.c
@@ -153,9 +154,15 @@
 /save-cwd.h
 /savedir.c
 /savedir.h
+/sig-handler.c
+/sig-handler.h
+/sigaction.c
+/signal.h
+/signal.in.h
 /signbitd.c
 /signbitf.c
 /signbitl.c
+/sigprocmask.c
 /size_max.h
 /stamp-h1
 /stat-time.c
@@ -175,6 +182,7 @@
 /stdio.in.h
 /stdlib.h
 /stdlib.in.h
+/stdnoreturn.in.h
 /stpcpy.c
 /strdup.c
 /strerror-override.c
@@ -218,4 +226,3 @@
 /xsize.h
 /yesno.c
 /yesno.h
-/stdnoreturn.in.h
diff --git a/m4/.gitignore b/m4/.gitignore
index 5ce3776..7f5c059 100644
--- a/m4/.gitignore
+++ b/m4/.gitignore
@@ -58,6 +58,7 @@
 /gnulib-common.m4
 /gnulib-comp.m4
 /gnulib-tool.m4
+/host-cpu-c-abi.m4
 /include_next.m4
 /intmax_t.m4
 /inttypes_h.m4
@@ -102,11 +103,15 @@
 /printf-posix-rpl.m4
 /printf.m4
 /pthread_rwlock_rdlock.m4
+/raise.m4
 /readdir.m4
 /realloc.m4
 /rmdir.m4
 /save-cwd.m4
 /savedir.m4
+/sigaction.m4
+/signal_h.m4
+/signalblocking.m4
 /signbit.m4
 /size_max.m4
 /ssize_t.m4
@@ -118,6 +123,7 @@
 /stdint_h.m4
 /stdio_h.m4
 /stdlib_h.m4
+/stdnoreturn.m4
 /stpcpy.m4
 /strdup.m4
 /strerror.m4
@@ -147,7 +153,5 @@
 /wint_t.m4
 /xalloc.m4
 /xsize.m4
-/yesno.m4
-/host-cpu-c-abi.m4
 /year2038.m4
-/stdnoreturn.m4
+/yesno.m4
-- 
2.17.1