shred: fix direct I/O failures for last write to file

Since direct I/O is now enabled with commit v8.21-139-gebaf961
we must handle the case where we write an odd size at the
end of a file (with --exact), or we specify an odd --size that
is larger than 64KiB, or in the very unlikely case of a device
with an odd size.  This issue was present since direct I/O
support was first added in v5.3.0, but latent since v6.0.
Theoretically this could have also been an issue after that on
systems which didn't have alignment constraints, but did have
size constraints for direct I/O.

* src/shred.c (dopass): On the first pass for a file, always
retry a write that fails with EINVAL, so we handle direct I/O
failure at either the start or end of the file.  Adjust the comment
as the original case is out of date and implicitly handled
by this more general fix.
* tests/misc/shred-exact.sh: Add a test case.
* NEWS: Add a "bug fix" entry for shred since there are
two related issues now fixed.

diff --git a/NEWS b/NEWS
index 6f685b4786d3c3b5e068661e60f77a0c2067dfcb..79665ac8b38efd385ad1053f19161d7fdd7de3e0 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   rm -I now prompts for confirmation before removing a write protected file.
   [Bug introduced in coreutils-6.8]
 
+  shred once again uses direct I/O on systems requiring aligned buffers.
+  Also direct I/O failures for odd sized writes at end of file are now handled.
+  [The "last write" bug was introduced in coreutils-5.3.0 but masked
+   by the alignment bug introduced in coreutils-6.0]
+
   tail --retry -f now waits for the files specified to appear.  Before, tail
   would immediately exit when such a file is inaccessible during the initial
   open.
@@ -92,9 +97,8 @@ GNU coreutils NEWS                                    -*- outline -*-
   Reservoir sampling is used to limit memory usage based on the number of
   outputs, rather than the number of inputs.
 
-  shred once again uses direct I/O where available, and increases write block
-  size from 12KiB to 64KiB when possible.
-  [Direct I/O regression introduced in coreutils-6.0]
+  shred increases write block size from 12KiB to 64KiB when possible,
+  to align with other utilities and reduce the system call overhead.
 
   split --line-bytes=SIZE, now only allocates memory as needed rather
   than allocating SIZE bytes at program start.

diff --git a/src/shred.c b/src/shred.c
index 9ff72386501f607a3fc1cf52eb2300d8f0fa367c..98dd8726ce4b25596d29b737441421853d92afd0 100644 (file)
--- a/src/shred.c
+++ b/src/shred.c
@@ -394,7 +394,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
   char pass_string[PASS_NAME_SIZE];    /* Name of current pass */
   bool write_error = false;
   bool other_error = false;
-  bool first_write = true;
+  bool tried_without_directio = false;
 
   /* Printable previous offset into the file */
   char previous_offset_buf[LONGEST_HUMAN_READABLE + 1];
@@ -443,7 +443,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
       if (type < 0)
         randread (s, pbuf, lim);
       /* Loop to retry partial writes. */
-      for (soff = 0; soff < lim; soff += ssize, first_write = false)
+      for (soff = 0; soff < lim; soff += ssize)
         {
           ssize = write (fd, pbuf + soff, lim - soff);
           if (ssize <= 0)
@@ -459,17 +459,15 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
                   int errnum = errno;
                   char buf[INT_BUFSIZE_BOUND (uintmax_t)];
 
-                  /* If the first write of the first pass for a given file
-                     has just failed with EINVAL, turn off direct mode I/O
-                     and try again.  This works around a bug in Linux kernel
-                     2.4 whereby opening with O_DIRECT would succeed for some
-                     file system types (e.g., ext3), but any attempt to
-                     access a file through the resulting descriptor would
-                     fail with EINVAL.  */
-                  if (k == 1 && first_write && errno == EINVAL)
+                  /* Retry without direct I/O since this may not be supported
+                     at all on some (file) systems, or with the current size.
+                     I.E. a specified --size that is not aligned, or when
+                     dealing with slop at the end of a file with --exact.  */
+                  if (k == 1 && !tried_without_directio && errno == EINVAL)
                     {
                       direct_mode (fd, false);
                       ssize = 0;
+                      tried_without_directio = true;
                       continue;
                     }
                   error (0, errnum, _("%s: error writing at offset %s"),
@@ -478,7 +476,8 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
                   /* 'shred' is often used on bad media, before throwing it
                      out.  Thus, it shouldn't give up on bad blocks.  This
                      code works because lim is always a multiple of
-                     SECTOR_SIZE, except at the end.  */
+                     SECTOR_SIZE, except at the end.  This size constraint
+                     also enables direct I/O on some (file) systems.  */
                   verify (PERIODIC_OUTPUT_SIZE % SECTOR_SIZE == 0);
                   verify (NONPERIODIC_OUTPUT_SIZE % SECTOR_SIZE == 0);
                   if (errnum == EIO && 0 <= size && (soff | SECTOR_MASK) < lim)

diff --git a/tests/misc/shred-exact.sh b/tests/misc/shred-exact.sh
index 0cdc91fc06feb3106dd6c6680023ed4103247f9f..eb30a7d695eca0caad879df7c0b93fd554dd3600 100755 (executable)
--- a/tests/misc/shred-exact.sh
+++ b/tests/misc/shred-exact.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# make sure that neither --exact nor --zero gobbles a command line argument
+# Test functionality of --exact
 
 # Copyright (C) 2000-2013 Free Software Foundation, Inc.
 
@@ -20,6 +20,7 @@
 print_ver_ shred
 
 
+# make sure that neither --exact nor --zero gobbles a command line argument
 for opt in --exact --zero; do
   echo a > a || fail=1
   echo bb > b || fail=1
@@ -33,4 +34,12 @@ for opt in --exact --zero; do
   test -f c && fail=1
 done
 
+
+# make sure direct I/O is handled appropriately at end of file
+# Create a 1MiB file as we'll probably not be using blocks larger than that
+# (i.e. we want to test failed writes not at the start).
+truncate -s1MiB file.slop || framework_failure_
+truncate -s+1 file.slop || framework_failure_
+shred --exact -n1 file.slop || fail=1
+
 Exit $fail