[Bug-tar] [PATCH] two patches for --atime-preserve races and other problems

[Paul Eggert]

>From 2ddc18f266de4bde505860c954e654bcb172a17a Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Thu, 16 Sep 2010 10:16:47 -0700
Subject: [PATCH] tar: --atime-preserve fixes for races etc.

This patch fixes a race condition in the --atime-preserve=replace
option, which might cause tar to improperly follow a symbolic link.

It also drops the use of the _FIOSATIME ioctl of Solaris 2.x
and later, which loses resolution on time stamps.  Modern Solaris
systems support full-resolution time stamps in the kernel, and
it's not worth the hassle of testing this call, useful only in
no-longer-supported Solaris variants.

Also, it undoes a change I recently introduced to the --compare
option, which caused it to not follow symbolic links unless the
--dereference option was also used.  Quite possibly this change is
a good idea, but the old behavior was documented and the change
should not have been installed casually.

* configure.ac: Don't check for stropts.h and sys/filio.h.
* gnulib.modules: Add futimens, utimensat.  Remove futimens.
* src/common.h (fd_utimensat): New decl.
* src/compare.c (diff_file, diff_multivol):
Don't use open_read_flags: those are for --create only.
* src/create.c (dump_file0): Adjust to set_file_atime changes.
Pass fstatat_flags to set_file_atime, so that symbolic links are
not followed inadvertantly.
* src/extract.c: Don't include utimens.h.
(set_stat): Use fd_utimensat ant UTIME_NOW rather than fdutimens.
* src/misc.c: Don't include utimens.h, stropts.h, sys/filio.h.
(fd_utimensat): New function.
(set_file_atime): Use it.  New arg atflag, controlling symlink
handling.  All callers changed.
---
 configure.ac   |    4 ++--
 gnulib.modules |    3 ++-
 src/common.h   |    6 ++++--
 src/compare.c  |   24 ++++++++++++++++++------
 src/create.c   |    7 +++----
 src/extract.c  |   14 ++++----------
 src/misc.c     |   38 ++++++++++++++++++--------------------
 7 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/configure.ac b/configure.ac
index 62df25f..97a0869 100644
--- a/configure.ac
+++ b/configure.ac
@@ -40,8 +40,8 @@ AC_ISC_POSIX
 AC_C_INLINE
 
 AC_CHECK_HEADERS_ONCE(fcntl.h linux/fd.h memory.h net/errno.h \
-  sgtty.h string.h stropts.h \
-  sys/param.h sys/device.h sys/filio.h sys/gentape.h \
+  sgtty.h string.h \
+  sys/param.h sys/device.h sys/gentape.h \
   sys/inet.h sys/io/trioctl.h \
   sys/mtio.h sys/time.h sys/tprintf.h sys/tape.h \
   unistd.h locale.h)
diff --git a/gnulib.modules b/gnulib.modules
index a595d90..c6ded15 100644
--- a/gnulib.modules
+++ b/gnulib.modules
@@ -18,6 +18,7 @@ fnmatch-gnu
 fseeko
 ftruncate
 full-write
+futimens
 getdate
 getline
 getopt-gnu
@@ -56,7 +57,7 @@ strtoul
 timespec
 unlinkdir
 unlocked-io
-utimens
+utimensat
 version-etc-fsf
 xalloc
 xalloc-die
diff --git a/src/common.h b/src/common.h
index e908854..8bd8640 100644
--- a/src/common.h
+++ b/src/common.h
@@ -610,6 +610,8 @@ bool maybe_backup_file (const char *file_name, bool 
this_is_the_archive);
 void undo_last_backup (void);
 
 int deref_stat (bool deref, char const *name, struct stat *buf);
+int fd_utimensat (int fd, int parentfd, char const *file,
+                 struct timespec const ts[2], int atflag);
 
 extern int chdir_current;
 int chdir_arg (char const *dir);
@@ -636,8 +638,8 @@ pid_t xfork (void);
 void xpipe (int fd[2]);
 
 void *page_aligned_alloc (void **ptr, size_t size);
-int set_file_atime (int fd, char const *file,
-                   struct timespec const timespec[2]);
+int set_file_atime (int fd, char const *file, struct timespec atime,
+                   int atflag);
 
 /* Module names.c.  */
 
diff --git a/src/compare.c b/src/compare.c
index b74793f..6a873d7 100644
--- a/src/compare.c
+++ b/src/compare.c
@@ -217,7 +217,14 @@ diff_file (void)
        }
       else
        {
-         diff_handle = open (file_name, open_read_flags);
+         int atime_flag =
+           (atime_preserve_option == system_atime_preserve
+            ? O_NOATIME
+            : 0);
+
+         diff_handle = open (file_name,
+                             (O_RDONLY | O_BINARY | O_CLOEXEC | O_NOCTTY
+                              | O_NONBLOCK | atime_flag));
 
          if (diff_handle < 0)
            {
@@ -236,10 +243,8 @@ diff_file (void)
 
              if (atime_preserve_option == replace_atime_preserve)
                {
-                 struct timespec ts[2];
-                 ts[0] = get_stat_atime (&stat_data);
-                 ts[1] = get_stat_mtime (&stat_data);
-                 if (set_file_atime (diff_handle, file_name, ts) != 0)
+                 struct timespec atime = get_stat_atime (&stat_data);
+                 if (set_file_atime (diff_handle, file_name, atime, 0) != 0)
                    utime_error (file_name);
                }
 
@@ -391,6 +396,10 @@ diff_multivol (void)
   struct stat stat_data;
   int fd, status;
   off_t offset;
+  int atime_flag =
+    (atime_preserve_option == system_atime_preserve
+     ? O_NOATIME
+     : 0);
 
   if (current_stat_info.had_trailing_slash)
     {
@@ -416,7 +425,10 @@ diff_multivol (void)
       return;
     }
 
-  fd = open (current_stat_info.file_name, open_read_flags);
+
+  fd = open (current_stat_info.file_name,
+            (O_RDONLY | O_BINARY | O_CLOEXEC | O_NOCTTY | O_NONBLOCK
+             | atime_flag));
 
   if (fd < 0)
     {
diff --git a/src/create.c b/src/create.c
index 9dc928d..6eedb2e 100644
--- a/src/create.c
+++ b/src/create.c
@@ -1610,7 +1610,6 @@ dump_file0 (struct tar_stat_info *st, char const *name, 
char const *p)
   char type;
   off_t original_size;
   struct timespec original_ctime;
-  struct timespec restore_times[2];
   off_t block_ordinal = -1;
   int fd = 0;
   bool is_dir;
@@ -1654,8 +1653,8 @@ dump_file0 (struct tar_stat_info *st, char const *name, 
char const *p)
     }
 
   st->archive_file_size = original_size = st->stat.st_size;
-  st->atime = restore_times[0] = get_stat_atime (&st->stat);
-  st->mtime = restore_times[1] = get_stat_mtime (&st->stat);
+  st->atime = get_stat_atime (&st->stat);
+  st->mtime = get_stat_mtime (&st->stat);
   st->ctime = original_ctime = get_stat_ctime (&st->stat);
 
 #ifdef S_ISHIDDEN
@@ -1794,7 +1793,7 @@ dump_file0 (struct tar_stat_info *st, char const *name, 
char const *p)
              set_exit_status (TAREXIT_DIFFERS);
            }
          else if (atime_preserve_option == replace_atime_preserve
-                  && set_file_atime (fd, p, restore_times) != 0)
+                  && set_file_atime (fd, p, st->atime, fstatat_flags) != 0)
            utime_error (p);
        }
 
diff --git a/src/extract.c b/src/extract.c
index 4bf2791..2296066 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -21,7 +21,6 @@
 
 #include <system.h>
 #include <quotearg.h>
-#include <utimens.h>
 #include <errno.h>
 #include <priv-set.h>
 
@@ -304,24 +303,19 @@ set_stat (char const *file_name,
 
       if (! touch_option && permstatus != INTERDIR_PERMSTATUS)
        {
-         /* We set the accessed time to `now', which is really the time we
-            started extracting files, unless incremental_option is used, in
-            which case .st_atime is used.  */
-
-         /* FIXME: incremental_option should set ctime too, but how?  */
-
          struct timespec ts[2];
          if (incremental_option)
            ts[0] = st->atime;
          else
-           ts[0] = start_time;
+           ts[0].tv_nsec = UTIME_NOW;
          ts[1] = st->mtime;
 
-         if (fdutimens (file_name, fd, ts) != 0)
+         if (fd_utimensat (fd, AT_FDCWD, file_name, ts, 0) != 0)
            utime_error (file_name);
          else
            {
-             check_time (file_name, ts[0]);
+             if (incremental_option)
+               check_time (file_name, ts[0]);
              check_time (file_name, ts[1]);
            }
        }
diff --git a/src/misc.c b/src/misc.c
index 6f67887..1cf0c3b 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -24,14 +24,6 @@
 #include <save-cwd.h>
 #include <xgetcwd.h>
 #include <unlinkdir.h>
-#include <utimens.h>
-
-#if HAVE_STROPTS_H
-# include <stropts.h>
-#endif
-#if HAVE_SYS_FILIO_H
-# include <sys/filio.h>
-#endif
 
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT 0
@@ -621,24 +613,30 @@ deref_stat (bool deref, char const *name, struct stat 
*buf)
   return deref ? stat (name, buf) : lstat (name, buf);
 }
 
-/* Set FD's (i.e., FILE's) access time to TIMESPEC[0].  If that's not
-   possible to do by itself, set its access and data modification
-   times to TIMESPEC[0] and TIMESPEC[1], respectively.  */
+/* Use futimens if possible, utimensat otherwise.  */
 int
-set_file_atime (int fd, char const *file, struct timespec const timespec[2])
+fd_utimensat (int fd, int parentfd, char const *file,
+             struct timespec const ts[2], int atflag)
 {
-#ifdef _FIOSATIME
   if (0 <= fd)
     {
-      struct timeval timeval;
-      timeval.tv_sec = timespec[0].tv_sec;
-      timeval.tv_usec = timespec[0].tv_nsec / 1000;
-      if (ioctl (fd, _FIOSATIME, &timeval) == 0)
-       return 0;
+      int result = futimens (fd, ts);
+      if (! (result < 0 && errno == ENOSYS))
+       return result;
     }
-#endif
 
-  return gl_futimens (fd, file, timespec);
+  return utimensat (parentfd, file, ts, atflag);
+}
+
+/* Set FD's (i.e., FILE's) access time to ATIME.
+   ATFLAG controls symbolic-link following, in the style of openat.  */
+int
+set_file_atime (int fd, char const *file, struct timespec atime, int atflag)
+{
+  struct timespec ts[2];
+  ts[0] = atime;
+  ts[1].tv_nsec = UTIME_OMIT;
+  return fd_utimensat (fd, AT_FDCWD, file, ts, atflag);
 }
 
 /* A description of a working directory.  */
-- 
1.7.2

>From d7be0dad18e6f61a5c6ca9a262abd5da60378206 Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Thu, 16 Sep 2010 10:46:27 -0700
Subject: [PATCH] tar: another --atime-preserve race fix

* src/common.h (set_file_atime): Add parentfd arg.
* src/compare.c (diff_file): Use it.
* src/create.c (dump_file0): Likewise.  This closes yet another
race condition with symbolic links.
* src/misc.c (set_file_atime): Add parentfd arg.
---
 src/common.h  |    4 ++--
 src/compare.c |    4 +++-
 src/create.c  |    4 +++-
 src/misc.c    |   10 ++++++----
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/common.h b/src/common.h
index 8bd8640..e0d8eb7 100644
--- a/src/common.h
+++ b/src/common.h
@@ -638,8 +638,8 @@ pid_t xfork (void);
 void xpipe (int fd[2]);
 
 void *page_aligned_alloc (void **ptr, size_t size);
-int set_file_atime (int fd, char const *file, struct timespec atime,
-                   int atflag);
+int set_file_atime (int fd, int parentfd, char const *file,
+                   struct timespec atime, int atflag);
 
 /* Module names.c.  */
 
diff --git a/src/compare.c b/src/compare.c
index 6a873d7..204c5dc 100644
--- a/src/compare.c
+++ b/src/compare.c
@@ -244,7 +244,9 @@ diff_file (void)
              if (atime_preserve_option == replace_atime_preserve)
                {
                  struct timespec atime = get_stat_atime (&stat_data);
-                 if (set_file_atime (diff_handle, file_name, atime, 0) != 0)
+                 if (set_file_atime (diff_handle, AT_FDCWD, file_name,
+                                     atime, 0)
+                     != 0)
                    utime_error (file_name);
                }
 
diff --git a/src/create.c b/src/create.c
index 6eedb2e..0d22e96 100644
--- a/src/create.c
+++ b/src/create.c
@@ -1793,7 +1793,9 @@ dump_file0 (struct tar_stat_info *st, char const *name, 
char const *p)
              set_exit_status (TAREXIT_DIFFERS);
            }
          else if (atime_preserve_option == replace_atime_preserve
-                  && set_file_atime (fd, p, st->atime, fstatat_flags) != 0)
+                  && (set_file_atime (fd, parentfd, name,
+                                      st->atime, fstatat_flags)
+                      != 0))
            utime_error (p);
        }
 
diff --git a/src/misc.c b/src/misc.c
index 1cf0c3b..64b1b2b 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -628,15 +628,17 @@ fd_utimensat (int fd, int parentfd, char const *file,
   return utimensat (parentfd, file, ts, atflag);
 }
 
-/* Set FD's (i.e., FILE's) access time to ATIME.
-   ATFLAG controls symbolic-link following, in the style of openat.  */
+/* Set FD's (i.e., assuming the working directory is PARENTFD, FILE's)
+   access time to ATIME.  ATFLAG controls symbolic-link following, in
+   the style of openat.  */
 int
-set_file_atime (int fd, char const *file, struct timespec atime, int atflag)
+set_file_atime (int fd, int parentfd, char const *file, struct timespec atime,
+               int atflag)
 {
   struct timespec ts[2];
   ts[0] = atime;
   ts[1].tv_nsec = UTIME_OMIT;
-  return fd_utimensat (fd, AT_FDCWD, file, ts, atflag);
+  return fd_utimensat (fd, parentfd, file, ts, atflag);
 }
 
 /* A description of a working directory.  */
-- 
1.7.2