>From 2c9f67e21aadbcf455ba8f9467202cc6ae8ad99d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 30 May 2019 13:53:54 -0700 Subject: [PATCH] dd: be more careful about signal handling Problem reported by Hans Henrik Bergan (Bug#36007). * NEWS: Mention this. * src/dd.c (iclose, ifdatasync, ifstat, ifsync): New functions, which are more careful about SIGINT. (cleanup): Use iclose instead of close. (finish_up): Process signals first. (skip, dd_copy, main): Use ifstat instead of fstat. (dd_copy): Use ifdatasync and ifsync instead of fdatasync and fsync. --- NEWS | 6 +++ src/dd.c | 113 ++++++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 96 insertions(+), 23 deletions(-) diff --git a/NEWS b/NEWS index 5db95b1c4..593c69b00 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,12 @@ GNU coreutils NEWS -*- outline -*- it is a character-special file whose minor device number is N. [bug introduced in fileutils-4.1.6] + dd conv=fdatasync no longer reports a "Bad file descriptor" error + when fdatasync is interrupted, and dd now retries interrupted calls + to close, fdatasync, fstat and fsync instead of incorrectly + reporting an "Interrupted system call" error. + [bugs introduced in coreutils-6.0] + df now correctly parses the /proc/self/mountinfo file for unusual entries like ones with '\r' in a field value ("mount -t tmpfs tmpfs /foo$'\r'bar"), when the source field is empty ('mount -t tmpfs "" /mnt'), and when the diff --git a/src/dd.c b/src/dd.c index ef0d07ac3..edb096a2f 100644 --- a/src/dd.c +++ b/src/dd.c @@ -529,7 +529,7 @@ maybe_close_stdout (void) _exit (EXIT_FAILURE); } -/* Like error() but handle any pending newline. */ +/* Like the 'error' function but handle any pending newline. */ static void _GL_ATTRIBUTE_FORMAT ((__printf__, 3, 4)) nl_error (int status, int errnum, const char *fmt, ...) @@ -914,8 +914,8 @@ install_signal_handlers (void) { act.sa_handler = siginfo_handler; /* Note we don't use SA_RESTART here and instead - handle EINTR explicitly in iftruncate() etc. - to avoid blocking on noncommitted read()/write() calls. */ + handle EINTR explicitly in iftruncate etc. + to avoid blocking on noncommitted read/write calls. */ act.sa_flags = 0; sigaction (SIGINFO, &act, NULL); } @@ -942,16 +942,33 @@ install_signal_handlers (void) #endif } +/* Close FD. Return 0 if successful, -1 (setting errno) otherwise. + If close fails with errno == EINTR, POSIX says the file descriptor + is in an unspecified state, so keep trying to close FD but do not + consider EBADF to be an error. Do not process signals. This all + differs somewhat from functions like ifdatasync and ifsync. */ +static int +iclose (int fd) +{ + if (close (fd) != 0) + do + if (errno != EINTR) + return -1; + while (close (fd) != 0 && errno != EBADF); + + return 0; +} + static void cleanup (void) { - if (close (STDIN_FILENO) < 0) + if (iclose (STDIN_FILENO) != 0) die (EXIT_FAILURE, errno, _("closing input file %s"), quoteaf (input_file)); /* Don't remove this call to close, even though close_stdout closes standard output. This close is necessary when cleanup - is called as part of a signal handler. */ - if (close (STDOUT_FILENO) < 0) + is called as a consequence of signal handling. */ + if (iclose (STDOUT_FILENO) != 0) die (EXIT_FAILURE, errno, _("closing output file %s"), quoteaf (output_file)); } @@ -992,9 +1009,10 @@ process_signals (void) static void finish_up (void) { + /* Process signals first, so that cleanup is called at most once. */ + process_signals (); cleanup (); print_stats (); - process_signals (); } static void ATTRIBUTE_NORETURN @@ -1192,7 +1210,7 @@ iwrite (int fd, char const *buf, size_t size) /* Since we have just turned off O_DIRECT for the final write, we try to preserve some of its semantics. */ - /* Call invalidate_cache() to setup the appropriate offsets + /* Call invalidate_cache to setup the appropriate offsets for subsequent calls. */ o_nocache_eof = true; invalidate_cache (STDOUT_FILENO, 0); @@ -1201,7 +1219,7 @@ iwrite (int fd, char const *buf, size_t size) to disk as quickly as possible. */ conversions_mask |= C_FSYNC; - /* After the subsequent fsync() we'll call invalidate_cache() + /* After the subsequent fsync we'll call invalidate_cache to attempt to clear all data from the page cache. */ } @@ -1271,7 +1289,24 @@ write_output (void) oc = 0; } -/* Restart on EINTR from fd_reopen(). */ +/* Restart on EINTR from fdatasync. */ + +static int +ifdatasync (int fd) +{ + int ret; + + do + { + process_signals (); + ret = fdatasync (fd); + } + while (ret < 0 && errno == EINTR); + + return ret; +} + +/* Restart on EINTR from fd_reopen. */ static int ifd_reopen (int desired_fd, char const *file, int flag, mode_t mode) @@ -1288,7 +1323,41 @@ ifd_reopen (int desired_fd, char const *file, int flag, mode_t mode) return ret; } -/* Restart on EINTR from ftruncate(). */ +/* Restart on EINTR from fstat. */ + +static int +ifstat (int fd, struct stat *st) +{ + int ret; + + do + { + process_signals (); + ret = fstat (fd, st); + } + while (ret < 0 && errno == EINTR); + + return ret; +} + +/* Restart on EINTR from fsync. */ + +static int +ifsync (int fd) +{ + int ret; + + do + { + process_signals (); + ret = fsync (fd); + } + while (ret < 0 && errno == EINTR); + + return ret; +} + +/* Restart on EINTR from ftruncate. */ static int iftruncate (int fd, off_t length) @@ -1780,7 +1849,7 @@ skip (int fdesc, char const *file, uintmax_t records, size_t blocksize, if (fdesc == STDIN_FILENO) { struct stat st; - if (fstat (STDIN_FILENO, &st) != 0) + if (ifstat (STDIN_FILENO, &st) != 0) die (EXIT_FAILURE, errno, _("cannot fstat %s"), quoteaf (file)); if (usable_st_size (&st) && st.st_size < input_offset + offset) { @@ -2036,7 +2105,7 @@ set_fd_flags (int fd, int add_flags, char const *name) /* NEW_FLAGS contains at least one file creation flag that requires some checking of the open file descriptor. */ struct stat st; - if (fstat (fd, &st) != 0) + if (ifstat (fd, &st) != 0) ok = false; else if ((new_flags & O_DIRECTORY) && ! S_ISDIR (st.st_mode)) { @@ -2327,7 +2396,7 @@ dd_copy (void) if (final_op_was_seek) { struct stat stdout_stat; - if (fstat (STDOUT_FILENO, &stdout_stat) != 0) + if (ifstat (STDOUT_FILENO, &stdout_stat) != 0) { error (0, errno, _("cannot fstat %s"), quoteaf (output_file)); return EXIT_FAILURE; @@ -2349,7 +2418,7 @@ dd_copy (void) } } - if ((conversions_mask & C_FDATASYNC) && fdatasync (STDOUT_FILENO) != 0) + if ((conversions_mask & C_FDATASYNC) && ifdatasync (STDOUT_FILENO) != 0) { if (errno != ENOSYS && errno != EINVAL) { @@ -2359,13 +2428,11 @@ dd_copy (void) conversions_mask |= C_FSYNC; } - if (conversions_mask & C_FSYNC) - while (fsync (STDOUT_FILENO) != 0) - if (errno != EINTR) - { - error (0, errno, _("fsync failed for %s"), quoteaf (output_file)); - return EXIT_FAILURE; - } + if ((conversions_mask & C_FSYNC) && ifsync (STDOUT_FILENO) != 0) + { + error (0, errno, _("fsync failed for %s"), quoteaf (output_file)); + return EXIT_FAILURE; + } return exit_status; } @@ -2465,7 +2532,7 @@ main (int argc, char **argv) fails on /dev/fd0. */ int ftruncate_errno = errno; struct stat stdout_stat; - if (fstat (STDOUT_FILENO, &stdout_stat) != 0) + if (ifstat (STDOUT_FILENO, &stdout_stat) != 0) die (EXIT_FAILURE, errno, _("cannot fstat %s"), quoteaf (output_file)); if (S_ISREG (stdout_stat.st_mode) -- 2.21.0