Re: [Bug-tar] opening files with O_NONBLOCK causes problems

[Paul Eggert]

On 01/06/12 06:32, Ron Kerry wrote:
> opening with O_NONBLOCK and then using fcntl() to clear the O_NONBLOCK flag 
> will work in a DMF context.

OK, I pushed the following patch into the GNU tar master.
Can you please give it a try on one of the affected hosts?
I don't have a hierarchical storage manager to play with.
Thanks.

>From 363d1bd2027833464d25b9519e7e892e3025fdfd Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Fri, 6 Jan 2012 12:38:55 -0800
Subject: [PATCH] tar: don't assume O_NONBLOCK is benign on regular files

On Data Migration Facility (DMF), High Peformance Storage System (HPSS),
and presumably other file systems based on hierarchical storage, opening
a regular file with O_NONBLOCK can cause later reads to fail with
errno == EAGAIN.  We need the O_NONBLOCK to avoid some security races.
Work around the problem by using fcntl to clear the O_NONBLOCK
flag if I/O fails with that errno value.
Problem reported by Vitezslav Cizek in
<http://lists.gnu.org/archive/html/bug-tar/2012-01/msg00000.html>.
* src/common.h (blocking_read, blocking_write): New decls.
* src/misc.c (blocking_read, blocking_write): New functions.
* src/compare.c (process_rawdata):
* src/create.c (dump_regular_file):
* src/extract.c (extract_file):
* src/sparse.c (sparse_scan_file, sparse_extract_region):
---
 src/common.h  |    3 +++
 src/compare.c |    8 ++++----
 src/create.c  |    2 +-
 src/extract.c |    4 ++--
 src/misc.c    |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/sparse.c  |    6 +++---
 6 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/src/common.h b/src/common.h
index 1429b22..c51ab11 100644
--- a/src/common.h
+++ b/src/common.h
@@ -621,6 +621,9 @@ void undo_last_backup (void);
 
 int deref_stat (char const *name, struct stat *buf);
 
+size_t blocking_read (int fd, void *buf, size_t count);
+size_t blocking_write (int fd, void const *buf, size_t count);
+
 extern int chdir_current;
 extern int chdir_fd;
 int chdir_arg (char const *dir);
diff --git a/src/compare.c b/src/compare.c
index 185a61a..639e935 100644
--- a/src/compare.c
+++ b/src/compare.c
@@ -80,7 +80,7 @@ process_noop (size_t size __attribute__ ((unused)),
 static int
 process_rawdata (size_t bytes, char *buffer)
 {
-  size_t status = safe_read (diff_handle, diff_buffer, bytes);
+  size_t status = blocking_read (diff_handle, diff_buffer, bytes);
 
   if (status != bytes)
     {
@@ -390,7 +390,7 @@ diff_dumpdir (struct tar_stat_info *dir)
          file_removed_diag (dir->orig_file_name, false, diag);
          return;
        }
-    }      
+    }
   dumpdir_buffer = directory_contents (scan_directory (dir));
 
   if (dumpdir_buffer)
@@ -462,7 +462,7 @@ diff_multivol (void)
 void
 diff_archive (void)
 {
-  
+
   set_next_block_after (current_header);
 
   /* Print the block from current_header and current_stat_info.  */
@@ -547,7 +547,7 @@ verify_volume (void)
          _("Verification may fail to locate original files.")));
 
   clear_directory_table ();
-  
+
   if (!diff_buffer)
     diff_init ();
 
diff --git a/src/create.c b/src/create.c
index 9839e1f..2e0bfc3 100644
--- a/src/create.c
+++ b/src/create.c
@@ -1051,7 +1051,7 @@ dump_regular_file (int fd, struct tar_stat_info *st)
            memset (blk->buffer + size_left, 0, BLOCKSIZE - count);
        }
 
-      count = (fd <= 0) ? bufsize : safe_read (fd, blk->buffer, bufsize);
+      count = (fd <= 0) ? bufsize : blocking_read (fd, blk->buffer, bufsize);
       if (count == SAFE_READ_ERROR)
        {
          read_diag_details (st->orig_file_name,
diff --git a/src/extract.c b/src/extract.c
index 6c38492..55f3eb8 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -649,7 +649,7 @@ maybe_recoverable (char *file_name, bool regular, bool 
*interdir_made)
 
        case KEEP_OLD_FILES:
          return RECOVER_NO;
-         
+
        case KEEP_NEWER_FILES:
          if (file_newer_p (file_name, stp, &current_stat_info))
            break;
@@ -998,7 +998,7 @@ extract_file (char *file_name, int typeflag)
        if (written > size)
          written = size;
        errno = 0;
-       count = full_write (fd, data_block->buffer, written);
+       count = blocking_write (fd, data_block->buffer, written);
        size -= written;
 
        set_next_block_after ((union block *)
diff --git a/src/misc.c b/src/misc.c
index b75f2ab..3add371 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -616,6 +616,57 @@ deref_stat (char const *name, struct stat *buf)
   return fstatat (chdir_fd, name, buf, fstatat_flags);
 }
 
+/* Read from FD into the buffer BUF with COUNT bytes.  Attempt to fill
+   BUF.  Wait until input is available; this matters because files are
+   opened O_NONBLOCK for security reasons, and on some file systems
+   this can cause read to fail with errno == EAGAIN.  Return the
+   actual number of bytes read, zero for EOF, or
+   SAFE_READ_ERROR upon error.  */
+size_t
+blocking_read (int fd, void *buf, size_t count)
+{
+  size_t bytes = safe_read (fd, buf, count);
+
+#if defined F_SETFL && O_NONBLOCK
+  if (bytes == SAFE_READ_ERROR && errno == EAGAIN)
+    {
+      int flags = fcntl (fd, F_GETFL);
+      if (0 <= flags && flags & O_NONBLOCK
+         && fcntl (fd, F_SETFL, flags & ~O_NONBLOCK) != -1)
+       bytes = safe_read (fd, buf, count);
+    }
+#endif
+
+  return bytes;
+}
+
+/* Write to FD from the buffer BUF with COUNT bytes.  Do a full write.
+   Wait until an output buffer is available; this matters because
+   files are opened O_NONBLOCK for security reasons, and on some file
+   systems this can cause write to fail with errno == EAGAIN.  Return
+   the actual number of bytes written, setting errno if that is less
+   than COUNT.  */
+size_t
+blocking_write (int fd, void const *buf, size_t count)
+{
+  size_t bytes = full_write (fd, buf, count);
+
+#if defined F_SETFL && O_NONBLOCK
+  if (bytes < count && errno == EAGAIN)
+    {
+      int flags = fcntl (fd, F_GETFL);
+      if (0 <= flags && flags & O_NONBLOCK
+         && fcntl (fd, F_SETFL, flags & ~O_NONBLOCK) != -1)
+       {
+         char const *buffer = buf;
+         bytes += full_write (fd, buffer + bytes, count - bytes);
+       }
+    }
+#endif
+
+  return bytes;
+}
+
 /* Set FD's (i.e., assuming the working directory is PARENTFD, FILE's)
    access time to ATIME.  */
 int
diff --git a/src/sparse.c b/src/sparse.c
index 4b2f982..8e5ad28 100644
--- a/src/sparse.c
+++ b/src/sparse.c
@@ -230,7 +230,7 @@ sparse_scan_file (struct tar_sparse_file *file)
       if (!tar_sparse_scan (file, scan_begin, NULL))
        return false;
 
-      while ((count = safe_read (fd, buffer, sizeof buffer)) != 0
+      while ((count = blocking_read (fd, buffer, sizeof buffer)) != 0
             && count != SAFE_READ_ERROR)
        {
          /* Analyze the block.  */
@@ -360,7 +360,7 @@ sparse_extract_region (struct tar_sparse_file *file, size_t 
i)
          return false;
        }
       set_next_block_after (blk);
-      count = full_write (file->fd, blk->buffer, wrbytes);
+      count = blocking_write (file->fd, blk->buffer, wrbytes);
       write_size -= count;
       file->dumped_size += count;
       mv_size_left (file->stat_info->archive_file_size - file->dumped_size);
@@ -991,7 +991,7 @@ pax_dump_header_1 (struct tar_sparse_file *file)
   off_t size = 0;
   struct sp_array *map = file->stat_info->sparse_map;
   char *save_file_name = file->stat_info->file_name;
-  
+
 #define COPY_STRING(b,dst,src) do                \
  {                                               \
    char *endp = b->buffer + BLOCKSIZE;           \
-- 
1.7.6.5