In shred, check for errors from fdatasync more carefully. If

fdatasync fails with errno==EINVAL, it means this implementation
does not support synchronized I/O for this file.  Do not report
this as an error, as (for example) AIX 5.2 fdatasync reports it
for raw disk devices.  Problem reported by Albert Chin in
<http://mail.gnu.org/archive/html/bug-gnu-utils/2004-05/msg00028.html>.

Check for write errors, though: the old code ignored them.
Improve error checking in a few other cases, too (e.g., close of a
directory).

Also, change several 'int' values to 'bool', so that the error
checking is a bit clearer.  Similarly, change unsigned values
to size_t where appropriate.

* src/shred.c: Include "dirname.h".
(datasync) [!HAVE_FDATASYNC]: Remove.
(dosync): New function.
(dopass): Use it.  Return 1 on write error, -1 on other error.
All callers changed.  Report write error if dosync does.
(do_wipefd, wipefd, wipename, wipefile): Return bool (true/false),
not int (0/-1).  All callers changed.  Return false if there's a
write error.
(incname): Return bool (true/false), not int (0/1).  Accept
size_t length, not unsigned.  All callers changed.  Do not
bother checking for non-digits; it can't happen.  Replace
recursion with iteration.
(wipename): Use dir_name, base_name, etc. instead of assuming
Unix file names.  Use size_t for length, not unsigned.
Report error if unlink or close fails.
(wipename, main): Use bool for booleans.

(names): Use only digits and uppercase letters, for greater
portability.

1 files changed, 147 insertions, 112 deletions

diff --git a/src/shred.c b/src/shred.c
index dd38bac8a..d6041ee35 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -104,6 +104,7 @@
 
 #include "system.h"
 #include "xstrtol.h"
+#include "dirname.h"
 #include "error.h"
 #include "getpagesize.h"
 #include "human.h"
@@ -218,10 +219,6 @@ to be recovered later.\n\
   exit (status);
 }
 
-#if ! HAVE_FDATASYNC
-# define fdatasync(fd) -1
-#endif
-
 /*
  * --------------------------------------------------------------------
  *     Bob Jenkins' cryptographic random number generator, ISAAC.
@@ -776,6 +773,42 @@ passname (unsigned char const *data, char name[PASS_NAME_SIZE])
     memcpy (name, "random", PASS_NAME_SIZE);
 }
 
+/* Request that all data for FD be transferred to the corresponding
+   storage device.  QNAME is the file name (quoted for colons), and
+   *ST its status.  Report any errors found.  Return 0 on success, -1
+   (setting errno) on failure.  It is not an error if fdatasync and/or
+   fsync is not supported for this file.  */
+static int
+dosync (int fd, char const *qname)
+{
+  int err;
+
+#if HAVE_FDATASYNC
+  if (fdatasync (fd) == 0)
+    return 0;
+  err = errno;
+  if (err != EINVAL)
+    {
+      error (0, err, "%s: fdatasync", qname);
+      errno = err;
+      return -1;
+    }
+#endif
+
+  if (fsync (fd) == 0)
+    return 0;
+  err = errno;
+  if (err != EINVAL)
+    {
+      error (0, err, "%s: fsync", qname);
+      errno = err;
+      return -1;
+    }
+
+  sync ();
+  return 0;
+}
+
 /*
  * Do pass number k of n, writing "size" bytes of the given pattern "type"
  * to the file descriptor fd.   Qname, k and n are passed in only for verbose
@@ -783,6 +816,8 @@ passname (unsigned char const *data, char name[PASS_NAME_SIZE])
  *
  * If *sizep == -1, the size is unknown, and it will be filled in as soon
  * as writing fails.
+ *
+ * Return 1 on write error, -1 on other error, 0 on success.
  */
 static int
 dopass (int fd, char const *qname, off_t *sizep, int type,
@@ -799,6 +834,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
   size_t rsize = 3 * MAX (ISAAC_WORDS, 1024) * sizeof *r; /* Fill size.  */
   size_t ralign = lcm (getpagesize (), sizeof *r); /* Fill alignment.  */
   char pass_string[PASS_NAME_SIZE];	/* Name of current pass */
+  bool write_error = false;
 
   /* Printable previous offset into the file */
   char previous_offset_buf[LONGEST_HUMAN_READABLE + 1];
@@ -885,6 +921,7 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
 			  != -1)
 			{
 			  soff += 512;
+			  write_error = true;
 			  continue;
 			}
 		      error (0, errno, "%s: lseek", qname);
@@ -953,22 +990,25 @@ dopass (int fd, char const *qname, off_t *sizep, int type,
 	       * It's a common problem with programs that do lots of writes,
 	       * like mkfs.
 	       */
-	      if (fdatasync (fd) < 0 && fsync (fd) < 0)
+	      if (dosync (fd, qname) != 0)
 		{
-		  error (0, errno, "%s: fsync", qname);
-		  return -1;
+		  if (errno != EIO)
+		    return -1;
+		  write_error = true;
 		}
 	    }
 	}
     }
 
   /* Force what we just wrote to hit the media. */
-  if (fdatasync (fd) < 0 && fsync (fd) < 0)
+  if (dosync (fd, qname) != 0)
     {
-      error (0, errno, "%s: fsync", qname);
-      return -1;
+      if (errno != EIO)
+	return -1;
+      write_error = true;
     }
-  return 0;
+
+  return write_error;
 }
 
 /*
@@ -1167,9 +1207,9 @@ genpattern (int *dest, size_t num, struct isaac_state *s)
 
 /*
  * The core routine to actually do the work.  This overwrites the first
- * size bytes of the given fd.  Returns -1 on error, 0 on success.
+ * size bytes of the given fd.  Return true if successful.
  */
-static int
+static bool
 do_wipefd (int fd, char const *qname, struct isaac_state *s,
 	   struct Options const *flags)
 {
@@ -1178,6 +1218,7 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
   off_t size;			/* Size to write, size to read */
   unsigned long n;		/* Number of passes for printing purposes */
   int *passarray;
+  bool ok = true;
 
   n = 0;		/* dopass takes n -- 0 to mean "don't print progress" */
   if (flags->verbose)
@@ -1186,7 +1227,7 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
   if (fstat (fd, &st))
     {
       error (0, errno, "%s: fstat", qname);
-      return -1;
+      return false;
     }
 
   /* If we know that we can't possibly shred the file, give up now.
@@ -1197,7 +1238,7 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
       || S_ISSOCK (st.st_mode))
     {
       error (0, 0, _("%s: invalid file type"), qname);
-      return -1;
+      return false;
     }
 
   /* Allocate pass array */
@@ -1214,7 +1255,7 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
 	  if (size < 0)
 	    {
 	      error (0, 0, _("%s: file has negative size"), qname);
-	      return -1;
+	      return false;
 	    }
 	}
       else
@@ -1245,11 +1286,16 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
   /* Do the work */
   for (i = 0; i < flags->n_iterations; i++)
     {
-      if (dopass (fd, qname, &size, passarray[i], s, i + 1, n) < 0)
+      int err = dopass (fd, qname, &size, passarray[i], s, i + 1, n);
+      if (err)
 	{
-	  memset (passarray, 0, flags->n_iterations * sizeof (int));
-	  free (passarray);
-	  return -1;
+	  if (err < 0)
+	    {
+	      memset (passarray, 0, flags->n_iterations * sizeof (int));
+	      free (passarray);
+	      return false;
+	    }
+	  ok = false;
 	}
     }
 
@@ -1257,8 +1303,15 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
   free (passarray);
 
   if (flags->zero_fill)
-    if (dopass (fd, qname, &size, 0, s, flags->n_iterations + 1, n) < 0)
-      return -1;
+    {
+      int err = dopass (fd, qname, &size, 0, s, flags->n_iterations + 1, n);
+      if (err)
+	{
+	  if (err < 0)
+	    return false;
+	  ok = false;
+	}
+    }
 
   /* Okay, now deallocate the data.  The effect of ftruncate on
      non-regular files is unspecified, so don't worry about any
@@ -1267,14 +1320,14 @@ do_wipefd (int fd, char const *qname, struct isaac_state *s,
       && S_ISREG (st.st_mode))
     {
       error (0, errno, _("%s: error truncating"), qname);
-      return -1;
+      return false;
     }
 
-  return 0;
+  return ok;
 }
 
 /* A wrapper with a little more checking for fds on the command line */
-static int
+static bool
 wipefd (int fd, char const *qname, struct isaac_state *s,
 	struct Options const *flags)
 {
@@ -1283,12 +1336,12 @@ wipefd (int fd, char const *qname, struct isaac_state *s,
   if (fd_flags < 0)
     {
       error (0, errno, "%s: fcntl", qname);
-      return -1;
+      return false;
     }
   if (fd_flags & O_APPEND)
     {
       error (0, 0, _("%s: cannot shred append-only file descriptor"), qname);
-      return -1;
+      return false;
     }
   return do_wipefd (fd, qname, s, flags);
 }
@@ -1296,43 +1349,32 @@ wipefd (int fd, char const *qname, struct isaac_state *s,
 /* --- Name-wiping code --- */
 
 /* Characters allowed in a file name - a safe universal set. */
-static char const nameset[] =
-"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_+=%@#.";
+static char const nameset[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
 
-/*
- * This increments the name, considering it as a big-endian base-N number
- * with the digits taken from nameset.  Characters not in the nameset
- * are considered to come before nameset[0].
- *
- * It's not obvious, but this will explode if name[0..len-1] contains
- * any 0 bytes.
- *
- * This returns the carry (1 on overflow).
- */
-static int
-incname (char *name, unsigned len)
+/* Increment NAME (with LEN bytes).  NAME must be a big-endian base N
+   number with the digits taken from nameset.  Return true if
+   successful if not (because NAME already has the greatest possible
+   value.  */
+
+static bool
+incname (char *name, size_t len)
 {
-  char const *p;
+  while (len--)
+    {
+      char const *p = strchr (nameset, name[--len]);
 
-  if (!len)
-    return 1;
+      /* If this character has a successor, use it.  */
+      if (p[1])
+	{
+	  name[len] = p[1];
+	  return true;
+	}
 
-  p = strchr (nameset, name[--len]);
-  /* If the character is not found, replace it with a 0 digit */
-  if (!p)
-    {
+      /* Otherwise, set this digit to 0 and increment the prefix.  */
       name[len] = nameset[0];
-      return 0;
     }
-  /* If this character has a successor, use it */
-  if (p[1])
-    {
-      name[len] = p[1];
-      return 0;
-    }
-  /* Otherwise, set this digit to 0 and increment the prefix */
-  name[len] = nameset[0];
-  return incname (name, len);
+
+  return false;
 }
 
 /*
@@ -1361,35 +1403,21 @@ incname (char *name, unsigned len)
  * This is fairly significantly Unix-specific.  Of course, on any
  * filesystem with synchronous metadata updates, this is unnecessary.
  */
-static int
+static bool
 wipename (char *oldname, char const *qoldname, struct Options const *flags)
 {
-  char *newname, *base;	  /* Base points to filename part of newname */
-  unsigned len;
-  int first = 1;
-  int err;
-  int dir_fd;			/* Try to open directory to sync *it* */
+  char *newname = xstrdup (oldname);
+  char *base = base_name (newname);
+  size_t len = base_len (base);
+  char *dir = dir_name (newname);
+  char *qdir = xstrdup (quotearg_colon (dir));
+  int dir_fd = open (dir, O_RDONLY | O_NOCTTY);
+  bool first = true;
+  bool ok = true;
 
-  newname = xstrdup (oldname);
   if (flags->verbose)
     error (0, 0, _("%s: removing"), qoldname);
 
-  /* Find the file name portion */
-  base = strrchr (newname, '/');
-  /* Temporary hackery to get a directory fd */
-  if (base)
-    {
-      *base = '\0';
-      dir_fd = open (newname, O_RDONLY | O_NOCTTY);
-      *base = '/';
-    }
-  else
-    {
-      dir_fd = open (".", O_RDONLY | O_NOCTTY);
-    }
-  base = base ? base + 1 : newname;
-  len = strlen (base);
-
   while (len)
     {
       memset (base, nameset[0], len);
@@ -1401,9 +1429,8 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
 	    {
 	      if (rename (oldname, newname) == 0)
 		{
-		  if (dir_fd < 0
-		      || (fdatasync (dir_fd) < 0 && fsync (dir_fd) < 0))
-		    sync ();	/* Force directory out */
+		  if (0 <= dir_fd && dosync (dir_fd, qdir) != 0)
+		    ok = false;
 		  if (flags->verbose)
 		    {
 		      /*
@@ -1414,7 +1441,7 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
 		       */
 		      char const *old = (first ? qoldname : oldname);
 		      error (0, 0, _("%s: renamed to %s"), old, newname);
-		      first = 0;
+		      first = false;
 		    }
 		  memcpy (oldname + (base - newname), base, len + 1);
 		  break;
@@ -1430,17 +1457,30 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
 	      /* newname exists, so increment BASE so we use another */
 	    }
 	}
-      while (!incname (base, len));
+      while (incname (base, len));
       len--;
     }
-  free (newname);
-  err = unlink (oldname);
-  if (dir_fd < 0 || (fdatasync (dir_fd) < 0 && fsync (dir_fd) < 0))
-    sync ();
-  close (dir_fd);
-  if (!err && flags->verbose)
+  if (unlink (oldname) != 0)
+    {
+      error (0, errno, "%s: remove", qoldname);
+      ok = false;
+    }
+  else if (flags->verbose)
     error (0, 0, _("%s: removed"), qoldname);
-  return err;
+  if (0 <= dir_fd)
+    {
+      if (dosync (dir_fd, qdir) != 0)
+	ok = false;
+      if (close (dir_fd) != 0)
+	{
+	  error (0, errno, "%s: close", qdir);
+	  ok = false;
+	}
+    }
+  free (newname);
+  free (dir);
+  free (qdir);
+  return ok;
 }
 
 /*
@@ -1455,11 +1495,12 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
  * again or EPERM, which both give similar error messages.
  * Does anyone disagree?
  */
-static int
+static bool
 wipefile (char *name, char const *qname,
 	  struct isaac_state *s, struct Options const *flags)
 {
-  int err, fd;
+  bool ok;
+  int fd;
 
   fd = open (name, O_WRONLY | O_NOCTTY);
   if (fd < 0)
@@ -1491,29 +1532,25 @@ wipefile (char *name, char const *qname,
   if (fd < 0)
     {
       error (0, errno, "%s", qname);
-      return -1;
+      return false;
     }
 
-  err = do_wipefd (fd, qname, s, flags);
+  ok = do_wipefd (fd, qname, s, flags);
   if (close (fd) != 0)
     {
-      error (0, 0, "%s: close", qname);
-      err = -1;
-    }
-  if (err == 0 && flags->remove_file)
-    {
-      err = wipename (name, qname, flags);
-      if (err < 0)
-	error (0, 0, _("%s: cannot remove"), qname);
+      error (0, errno, "%s: close", qname);
+      ok = false;
     }
-  return err;
+  if (ok && flags->remove_file)
+    ok = wipename (name, qname, flags);
+  return ok;
 }
 
 int
 main (int argc, char **argv)
 {
   struct isaac_state s;
-  int err = 0;
+  bool ok = true;
   struct Options flags;
   char **file;
   int n_files;
@@ -1612,14 +1649,12 @@ main (int argc, char **argv)
       char *qname = xstrdup (quotearg_colon (file[i]));
       if (STREQ (file[i], "-"))
 	{
-	  if (wipefd (STDOUT_FILENO, qname, &s, &flags) < 0)
-	    err = 1;
+	  ok &= wipefd (STDOUT_FILENO, qname, &s, &flags);
 	}
       else
 	{
 	  /* Plain filename - Note that this overwrites *argv! */
-	  if (wipefile (file[i], qname, &s, &flags) < 0)
-	    err = 1;
+	  ok &= wipefile (file[i], qname, &s, &flags);
 	}
       free (qname);
     }
@@ -1627,7 +1662,7 @@ main (int argc, char **argv)
   /* Just on general principles, wipe s. */
   memset (&s, 0, sizeof s);
 
-  exit (err == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
+  exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
 }
 /*
  * vim:sw=2:sts=2: