From f9c96538a4b3458893f48ea73a1b45fd393805fd Mon Sep 17 00:00:00 2001 From: Paul Eggert
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