Thanks for reporting the bug. I installed the attached patch into the
development version on Savannah, to fix the bug you reported along with
a closely related bug that I found when looking into your problem.
Please give this patch a try at your convenience.
>From 2c9f67e21aadbcf455ba8f9467202cc6ae8ad99d Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
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
