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.

diff --git a/NEWS b/NEWS
index 5db95b1c41aa063b603fb439dd2b0d918e2c9300..593c69b00d5b8151b33766c8615112d69717cd08 100644 (file)
--- 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 ef0d07ac395faa0a2e606dd9b7dcc7ca6fc1e173..edb096a2fa6cc2af86ecc98bceffb59ab88f5694 100644 (file)
--- 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)