Subject: bug#36007: closed (Re: bug#36007: double close() of of=file if receiving SIGINT during fdatasync())

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 30 May 2019 20:57:04 +0000

Your bug report

#36007: double close() of of=file if receiving SIGINT during fdatasync()

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
36007: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=36007
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#36007: double close() of of=file if receiving SIGINT during fdatasync()
From: Paul Eggert <[email-masked]>
To: Hans Henrik Bergan <[email-masked]>, [email-masked]
Date: Thu, 30 May 2019 13:56:04 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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-masked]>
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

Subject: double close() of of=file if receiving SIGINT during fdatasync()
From: Hans Henrik Bergan <[email-masked]>
To: [email-masked]
Date: Thu, 30 May 2019 17:38:16 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

i think there is a double-close() bug that causes dd to try to close input
file twice if it receives a SIGINT during fdatasync(),

it seems easy to reproduce on a slow block device (the issue was noticed on
a 5400RPM harddrive), but i have not been able to reproduce it on a SSD
(likely because fdatasync() finishes too fast, i didn't try very hard.
like, i suppose i could have hooked fdatasync and made it slow on purpose,
but i didn't spend that much time/energy on trying),

anyway, the bug seems to boil down to somehow cleanup() is being called
twice

command issued:

time strace dd if=/dev/zero of=/dev/sda4 bs=10M count=10 iflag=fullblock
conv=fdatasync >stdout 2>stderr

dd version:
root@1:/dev# dd --version
dd (coreutils) 8.30
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <
https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Paul Rubin, David MacKenzie, and Stuart Kemp.

script log (corrupted when pasted, strictly speaking, but i don't think
anything of value was lost. if you want an actual 100% intact log, i can
send you a base64 copy if you want, but again i don't think anything of
value was lost)

Script started on 2019-05-30 09:39:45-04:00 [TERM="xterm" TTY="/dev/pts/1"
COLUMNS="194" LINES="41"]
root@1:/dev# exit pastebinit foo.log
[C [C [C [C [C [C [C [C [C [C [C [C [C [5Pscript foo.log [3Prm foo.log
[2@nano [C [C [C [C [C [C [C [C [C [1Pcat [C [C [C [C [C [C [C [C [C script
foo.log ls [K cat stderr out time strace dd if=/dev/zero of=/dev/sda4
bs=10M count=10 iflag=fullblock conv=fdatasync >stdout 2>stderr
execve("/usr/bin/dd", ["dd", "if=/dev/zero", "of=/dev/sda4", "bs=10M",
"count=10", "iflag=fullblock", "conv=fdatasync"], 0x7fffffffe690 /* 18 vars
*/) = 0
brk(NULL)                               = 0x555555568000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or
directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=20223, ...}) = 0
mmap(NULL, 20223, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7ffff7fcb000
close(3)                                = 0
openat(AT_FDCWD, "/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3,
"\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\260A\2\0\0\0\0\0"..., 832)
= 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1824496, ...}) = 0
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7ffff7fc9000
mmap(NULL, 1837056, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) =
0x7ffff7e08000
mprotect(0x7ffff7e2a000, 1658880, PROT_NONE) = 0
mmap(0x7ffff7e2a000, 1343488, PROT_READ|PROT_EXEC,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x22000) = 0x7ffff7e2a000
mmap(0x7ffff7f72000, 311296, PROT_READ,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x16a000) = 0x7ffff7f72000
mmap(0x7ffff7fbf000, 24576, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1b6000) = 0x7ffff7fbf000
mmap(0x7ffff7fc5000, 14336, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7ffff7fc5000
close(3)                                = 0
arch_prctl(ARCH_SET_FS, 0x7ffff7fca580) = 0
mprotect(0x7ffff7fbf000, 16384, PROT_READ) = 0
mprotect(0x555555566000, 4096, PROT_READ) = 0
mprotect(0x7ffff7ffc000, 4096, PROT_READ) = 0
munmap(0x7ffff7fcb000, 20223)           = 0
rt_sigaction(SIGINT, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8)
= 0
rt_sigaction(SIGUSR1, {sa_handler=0x555555558430, sa_mask=[INT USR1],
sa_flags=SA_RESTORER, sa_restorer=0x7ffff7e3f840}, NULL, 8) = 0
rt_sigaction(SIGINT, {sa_handler=0x555555558420, sa_mask=[INT USR1],
sa_flags=SA_RESTORER|SA_NODEFER|SA_RESETHAND, sa_restorer=0x7ffff7e3f840},
NULL, 8) = 0
brk(NULL)                               = 0x555555568000
brk(0x555555589000)                     = 0x555555589000
openat(AT_FDCWD, "/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=3031616, ...}) = 0
mmap(NULL, 3031616, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7ffff7b23000
close(3)                                = 0
openat(AT_FDCWD, "/dev/zero", O_RDONLY) = 3
dup2(3, 0)                              = 0
close(3)                                = 0
lseek(0, 0, SEEK_CUR)                   = 0
openat(AT_FDCWD, "/dev/sda4", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3
dup2(3, 1)                              = 1
close(3)                                = 0
mmap(NULL, 10498048, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7ffff7120000
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
read(0,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
write(1,
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
10485760) = 10485760
fdatasync(1)                            = 0
--- SIGINT {si_signo=SIGINT, si_code=SI_USER, si_pid=1399, si_uid=0} ---
rt_sigreturn({mask=[]})                 = 0
close(0)                                = 0
close(1)                                = 0
openat(AT_FDCWD, "/usr/share/locale/locale.alias", O_RDONLY|O_CLOEXEC) = 0
fstat(0, {st_mode=S_IFREG|0644, st_size=2995, ...}) = 0
read(0, "# Locale name alias data base.\n#"..., 4096) = 2995
read(0, "", 4096)                       = 0
close(0)                                = 0
openat(AT_FDCWD, "/usr/share/locale/en_CA/LC_MESSAGES/coreutils.mo",
O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY)
= -1 ENOENT (No such file or directory)
write(2, "10+0 records in\n10+0 records out"..., 3310+0 records in
10+0 records out
) = 33
write(2, "104857600 bytes (105 MB, 100 MiB"..., 62104857600 bytes (105 MB,
100 MiB) copied, 1.94798 s, 53.8 MB/s) = 62
write(2, "\n", 1
)                       = 1
rt_sigprocmask(SIG_BLOCK, [INT USR1], [], 8) = 0
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
close(0)                                = -1 EBADF (Bad file descriptor)
write(2, "dd: ", 4dd: )                     = 4
write(2, "closing input file '/dev/zero'", 30closing input file
'/dev/zero') = 30
openat(AT_FDCWD, "/usr/share/locale/en_CA/LC_MESSAGES/libc.mo", O_RDONLY) =
-1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/libc.mo", O_RDONLY) = -1
ENOENT (No such file or directory)
write(2, ": Bad file descriptor", 21: Bad file descriptor)   = 21
write(2, "\n", 1
)                       = 1
close(2)                                = 0
exit_group(1)                           = ?
+++ exited with 1 +++

real 0m1.955s
user 0m0.001s
sys 0m0.201s
root@1:/dev# exit
exit

Script done on 2019-05-30 09:40:05-04:00 [COMMAND_EXIT_CODE="1"]