dd: use more robust SIGUSR1 handling

* src/dd.c (ifd_reopen): A new wrapper to ensure we
don't exit upon receiving a SIGUSR1 in a blocking open()
on a fifo for example.
(iftruncate): Likewise for ftruncate().
(iread): Process signals also after a short read.
(install_signal_handlers): Install SIGINFO/SIGUSR1 handler
even if set to SIG_IGN, as this is what the parent can easily
set from a shell script that can send SIGUSR1 without the
possiblity of inadvertently killing the dd process.
* doc/coreutils.texi (dd invocation): Improve the example to
show robust usage wrt signal races and short reads.
* tests/dd/stats.sh: A new test for various signal races.
* tests/local.mk: Reference the new test.
* NEWS: Mention the fix.

5 files changed, 135 insertions, 32 deletions

diff --git a/NEWS b/NEWS
index 077c5fcb9..3e10ac4e5 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  dd supports more robust SIGINFO/SIGUSR1 handling for outputting statistics.
+  Previously those signals may have inadvertently terminated the process.
+
   cp no longer issues an incorrect warning about directory hardlinks when a
   source directory is specified multiple times.  Now, consistent with other
   file types, a warning is issued for source directories with duplicate names,
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 1519fcb3e..7d32af582 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9001,23 +9001,36 @@ occur on disk based devices):
 dd conv=noerror,sync iflag=fullblock </dev/sda1 > /mnt/rescue.img
 @end example
 
-Sending an @samp{INFO} signal to a running @command{dd}
-process makes it print I/O statistics to standard error
-and then resume copying.  In the example below,
-@command{dd} is run in the background to copy 10 million blocks.
+Sending an @samp{INFO} signal (or @samp{USR1} signal where that is unavailable)
+to a running @command{dd} process makes it print I/O statistics to
+standard error and then resume copying.  In the example below,
+@command{dd} is run in the background to copy 5GB of data.
 The @command{kill} command makes it output intermediate I/O statistics,
 and when @command{dd} completes normally or is killed by the
 @code{SIGINT} signal, it outputs the final statistics.
 
 @example
-$ dd if=/dev/zero of=/dev/null count=10MB & pid=$!
-$ kill -s INFO $pid; wait $pid
-3385223+0 records in
-3385223+0 records out
-1733234176 bytes (1.7 GB) copied, 6.42173 seconds, 270 MB/s
-10000000+0 records in
-10000000+0 records out
-5120000000 bytes (5.1 GB) copied, 18.913 seconds, 271 MB/s
+# Ignore the signal so we never inadvertently terminate the dd child.
+# Note this is not needed when SIGINFO is available.
+trap '' USR1
+
+# Run dd with the fullblock iflag to avoid short reads
+# which can be triggered by reception of signals.
+dd iflag=fullblock if=/dev/zero of=/dev/null count=5000000 bs=1000 & pid=$!
+
+# Output stats every half second
+until ! kill -s USR1 $pid 2>/dev/null; do sleep .5; done
+@end example
+
+The above script will output in the following format
+
+@example
+859+0 records in
+859+0 records out
+4295000000 bytes (4.3 GB) copied, 0.539934 s, 8.0 GB/s
+1000+0 records in
+1000+0 records out
+5000000000 bytes (5.0 GB) copied, 0.630785 s, 7.9 GB/s
 @end example
 
 @vindex POSIXLY_CORRECT
diff --git a/src/dd.c b/src/dd.c
index c17f7d8f3..d22ec592d 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -627,22 +627,14 @@ Each FLAG symbol may be:\n\
 "), stdout);
 
       {
-        char const *siginfo_name = (SIGINFO == SIGUSR1 ? "USR1" : "INFO");
         printf (_("\
 \n\
 Sending a %s signal to a running 'dd' process makes it\n\
 print I/O statistics to standard error and then resume copying.\n\
 \n\
-  $ dd if=/dev/zero of=/dev/null& pid=$!\n\
-  $ kill -%s $pid; sleep 1; kill $pid\n\
-  18335302+0 records in\n\
-  18335302+0 records out\n\
-  9387674624 bytes (9.4 GB) copied, 34.6279 seconds, 271 MB/s\n\
-\n\
 Options are:\n\
 \n\
-"),
-                siginfo_name, siginfo_name);
+"), SIGINFO == SIGUSR1 ? "USR1" : "INFO");
       }
 
       fputs (HELP_OPTION_DESCRIPTION, stdout);
@@ -830,11 +822,7 @@ install_signal_handlers (void)
   struct sigaction act;
   sigemptyset (&caught_signals);
   if (catch_siginfo)
-    {
-      sigaction (SIGINFO, NULL, &act);
-      if (act.sa_handler != SIG_IGN)
-        sigaddset (&caught_signals, SIGINFO);
-    }
+    sigaddset (&caught_signals, SIGINFO);
   sigaction (SIGINT, NULL, &act);
   if (act.sa_handler != SIG_IGN)
     sigaddset (&caught_signals, SIGINT);
@@ -843,6 +831,9 @@ install_signal_handlers (void)
   if (sigismember (&caught_signals, SIGINFO))
     {
       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.  */
       act.sa_flags = 0;
       sigaction (SIGINFO, &act, NULL);
     }
@@ -856,7 +847,7 @@ install_signal_handlers (void)
 
 #else
 
-  if (catch_siginfo && signal (SIGINFO, SIG_IGN) != SIG_IGN)
+  if (catch_siginfo)
     {
       signal (SIGINFO, siginfo_handler);
       siginterrupt (SIGINFO, 1);
@@ -1033,6 +1024,10 @@ iread (int fd, char *buf, size_t size)
     }
   while (nread < 0 && errno == EINTR);
 
+  /* Short read may be due to received signal.  */
+  if (0 < nread && nread < size)
+    process_signals ();
+
   if (0 < nread && warn_partial_read)
     {
       static ssize_t prev_nread;
@@ -1173,6 +1168,40 @@ write_output (void)
   oc = 0;
 }
 
+/* Restart on EINTR from fd_reopen().  */
+
+static int
+ifd_reopen (int desired_fd, char const *file, int flag, mode_t mode)
+{
+  int ret;
+
+  do
+    {
+      process_signals ();
+      ret = fd_reopen (desired_fd, file, flag, mode);
+    }
+  while (ret < 0 && errno == EINTR);
+
+  return ret;
+}
+
+/* Restart on EINTR from ftruncate().  */
+
+static int
+iftruncate (int fd, off_t length)
+{
+  int ret;
+
+  do
+    {
+      process_signals ();
+      ret = ftruncate (fd, length);
+    }
+  while (ret < 0 && errno == EINTR);
+
+  return ret;
+}
+
 /* Return true if STR is of the form "PATTERN" or "PATTERNDELIM...".  */
 
 static bool _GL_ATTRIBUTE_PURE
@@ -2172,7 +2201,7 @@ dd_copy (void)
           off_t output_offset = lseek (STDOUT_FILENO, 0, SEEK_CUR);
           if (output_offset > stdout_stat.st_size)
             {
-              if (ftruncate (STDOUT_FILENO, output_offset) != 0)
+              if (iftruncate (STDOUT_FILENO, output_offset) != 0)
                 {
                   error (0, errno,
                          _("failed to truncate to %" PRIdMAX " bytes"
@@ -2248,7 +2277,7 @@ main (int argc, char **argv)
     }
   else
     {
-      if (fd_reopen (STDIN_FILENO, input_file, O_RDONLY | input_flags, 0) < 0)
+      if (ifd_reopen (STDIN_FILENO, input_file, O_RDONLY | input_flags, 0) < 0)
         error (EXIT_FAILURE, errno, _("failed to open %s"), quote (input_file));
     }
 
@@ -2275,8 +2304,8 @@ main (int argc, char **argv)
          need to read to satisfy a 'seek=' request.  If we can't read
          the file, go ahead with write-only access; it might work.  */
       if ((! seek_records
-           || fd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0)
-          && (fd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms)
+           || ifd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0)
+          && (ifd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms)
               < 0))
         error (EXIT_FAILURE, errno, _("failed to open %s"),
                quote (output_file));
@@ -2293,7 +2322,7 @@ main (int argc, char **argv)
                      " (%lu-byte) blocks"),
                    seek_records, obs);
 
-          if (ftruncate (STDOUT_FILENO, size) != 0)
+          if (iftruncate (STDOUT_FILENO, size) != 0)
             {
               /* Complain only when ftruncate fails on a regular file, a
                  directory, or a shared memory object, as POSIX 1003.1-2004
diff --git a/tests/dd/stats.sh b/tests/dd/stats.sh
new file mode 100755
index 000000000..386752e77
--- /dev/null
+++ b/tests/dd/stats.sh
@@ -0,0 +1,57 @@
+#!/bin/sh
+# Check robust handling of SIG{INFO,USR1}
+
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ dd
+
+env kill -l | grep '^INFO$' && SIGINFO='INFO' || SIGINFO='USR1'
+
+# This to avoid races in the USR1 case
+# as the dd process will terminate by default until
+# it has its handler enabled.
+trap '' $SIGINFO
+
+mkfifo_or_skip_ fifo
+
+for open in '' '1'; do
+  # Run dd with the fullblock iflag to avoid short reads
+  # which can be triggered by reception of signals
+  dd iflag=fullblock if=/dev/zero of=fifo count=100 bs=5000000 2>err & pid=$!
+
+  # Note if we sleep here we give dd a chance to exec and block on open.
+  # Otherwise we're probably testing SIG_IGN in the forked shell or early dd.
+  test "$open" && sleep .1
+
+  # dd will block on open until fifo is opened for reading.
+  # Timeout in case dd goes away erroneously which we check for below.
+  timeout 10 sh -c 'wc -c < fifo > nwritten' &
+
+  # Send lots of signals immediately to ensure dd not killed due
+  # to race setting handler, or blocking on open of fifo.
+  # Many signals also check that short reads are handled.
+  until ! kill -s $SIGINFO $pid 2>/dev/null; do
+    sleep .01
+  done
+
+  wait
+
+  # Ensure all data processed and at least last status written
+  grep '500000000 bytes .* copied' err || { cat err; fail=1; }
+done
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 97bf5edda..8498acbbe 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -490,6 +490,7 @@ all_tests =					\
   tests/dd/stderr.sh				\
   tests/dd/unblock.pl				\
   tests/dd/unblock-sync.sh			\
+  tests/dd/stats.sh				\
   tests/df/total-verify.sh			\
   tests/du/2g.sh				\
   tests/du/8gb.sh				\