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 #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