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 <egg...@cs.ucla.edu>
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
