[Emacs-diffs] master 2b7e009: Fix file-attributes race on GNU hosts

[Paul Eggert]

branch: master
commit 2b7e009257a40ef1dcad9845fe61764fea08cdea
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Fix file-attributes race on GNU hosts
    
    * doc/lispref/files.texi (File Attributes):
    Document file-attributes atomicity.
    * etc/NEWS: Document the fix.
    * src/dired.c (file_attributes): New args DIRNAME and FILENAME,
    for diagnostics.  All callers changed.  On platforms like
    GNU/Linux that support O_PATH, fix a race condition in
    file-attributes and similar functions, so that these functions do
    not return nonsense if a directory entry is replaced while getting
    its attributes.  On non-GNU platforms, do a better (though not
    perfect) job of detecting the race, and return nil if detected.
---
 doc/lispref/files.texi |  7 ++++
 etc/NEWS               |  6 ++++
 src/dired.c            | 86 +++++++++++++++++++++++++++++++++++++-------------
 src/kqueue.c           |  4 +--
 src/sysdep.c           |  2 +-
 5 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5a52765..36944e4 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1249,6 +1249,13 @@ the default, but we plan to change that, so you should 
specify a
 address@hidden value for @var{id-format} if you use the returned
 @acronym{UID} or @acronym{GID}.
 
+On GNU platforms when operating on a local file, this function is
+atomic: if the filesystem is simultaneously being changed by some
+other process, this function returns the file's attributes either
+before or after the change.  Otherwise this function is not atomic,
+and might return @code{nil} it detects the race condition, or might
+return a hodgepodge of the previous and current file attributes.
+
 Accessor functions are provided to access the elements in this list.
 The accessors are mentioned along with the descriptions of the
 elements below.
diff --git a/etc/NEWS b/etc/NEWS
index bf59749..02de66b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1428,6 +1428,12 @@ job of signaling list cycles instead of looping 
indefinitely.
 can be used for creation of temporary files of remote or mounted directories.
 
 +++
+** On GNU platforms when operating on a local file, 'file-attributes'
+no longer suffers from a race when called while another process is
+altering the filesystem.  On non-GNU platforms 'file-attributes'
+attempts to detect the race, and returns nil if it does so.
+
++++
 ** The new function 'file-local-name' can be used to specify arguments
 of remote processes.
 
diff --git a/src/dired.c b/src/dired.c
index 288ba6b..128493a 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -51,7 +51,8 @@ extern int is_slow_fs (const char *);
 #endif
 
 static ptrdiff_t scmp (const char *, const char *, ptrdiff_t);
-static Lisp_Object file_attributes (int, char const *, Lisp_Object);
+static Lisp_Object file_attributes (int, char const *, Lisp_Object,
+                                   Lisp_Object, Lisp_Object);
 
 /* Return the number of bytes in DP's name.  */
 static ptrdiff_t
@@ -161,7 +162,7 @@ read_dirent (DIR *dir, Lisp_Object dirname)
 /* Function shared by Fdirectory_files and Fdirectory_files_and_attributes.
    If not ATTRS, return a list of directory filenames;
    if ATTRS, return a list of directory filenames and their attributes.
-   In the latter case, ID_FORMAT is passed to Ffile_attributes.  */
+   In the latter case, pass ID_FORMAT to file_attributes.  */
 
 Lisp_Object
 directory_files_internal (Lisp_Object directory, Lisp_Object full,
@@ -225,7 +226,7 @@ directory_files_internal (Lisp_Object directory, 
Lisp_Object full,
   if (attrs)
     {
       /* Do this only once to avoid doing it (in w32.c:stat) for each
-        file in the directory, when we call Ffile_attributes below.  */
+        file in the directory, when we call file_attributes below.  */
       record_unwind_protect (directory_files_internal_w32_unwind,
                             Vw32_get_true_file_attributes);
       w32_save = Vw32_get_true_file_attributes;
@@ -304,7 +305,7 @@ directory_files_internal (Lisp_Object directory, 
Lisp_Object full,
          if (attrs)
            {
              Lisp_Object fileattrs
-               = file_attributes (fd, dp->d_name, id_format);
+               = file_attributes (fd, dp->d_name, directory, name, id_format);
              list = Fcons (Fcons (finalname, fileattrs), list);
            }
          else
@@ -351,7 +352,7 @@ If NOSORT is non-nil, the list is not sorted--its order is 
unpredictable.
     return call5 (handler, Qdirectory_files, directory,
                   full, match, nosort);
 
-  return directory_files_internal (directory, full, match, nosort, 0, Qnil);
+  return directory_files_internal (directory, full, match, nosort, false, 
Qnil);
 }
 
 DEFUN ("directory-files-and-attributes", Fdirectory_files_and_attributes,
@@ -379,7 +380,8 @@ which see.  */)
     return call6 (handler, Qdirectory_files_and_attributes,
                   directory, full, match, nosort, id_format);
 
-  return directory_files_internal (directory, full, match, nosort, 1, 
id_format);
+  return directory_files_internal (directory, full, match, nosort,
+                                  true, id_format);
 }
 
 
@@ -923,14 +925,17 @@ so last access time will always be midnight of that day.  
*/)
     }
 
   encoded = ENCODE_FILE (filename);
-  return file_attributes (AT_FDCWD, SSDATA (encoded), id_format);
+  return file_attributes (AT_FDCWD, SSDATA (encoded), Qnil, filename,
+                         id_format);
 }
 
 static Lisp_Object
-file_attributes (int fd, char const *name, Lisp_Object id_format)
+file_attributes (int fd, char const *name,
+                Lisp_Object dirname, Lisp_Object filename,
+                Lisp_Object id_format)
 {
+  ptrdiff_t count = SPECPDL_INDEX ();
   struct stat s;
-  int lstat_result;
 
   /* An array to hold the mode string generated by filemodestring,
      including its terminating space and null byte.  */
@@ -938,22 +943,60 @@ file_attributes (int fd, char const *name, Lisp_Object 
id_format)
 
   char *uname = NULL, *gname = NULL;
 
-#ifdef WINDOWSNT
-  /* We usually don't request accurate owner and group info, because
-     it can be very expensive on Windows to get that, and most callers
-     of 'lstat' don't need that.  But here we do want that information
-     to be accurate.  */
-  w32_stat_get_owner_group = 1;
-#endif
+  int err = EINVAL;
 
-  lstat_result = fstatat (fd, name, &s, AT_SYMLINK_NOFOLLOW);
+#ifdef O_PATH
+  int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
+  if (namefd < 0)
+    err = errno;
+  else
+    {
+      record_unwind_protect_int (close_file_unwind, namefd);
+      if (fstat (namefd, &s) != 0)
+       err = errno;
+      else
+       {
+         err = 0;
+         fd = namefd;
+         name = "";
+       }
+    }
+#endif
 
+  if (err == EINVAL)
+    {
+#ifdef WINDOWSNT
+      /* We usually don't request accurate owner and group info,
+        because it can be expensive on Windows to get that, and most
+        callers of 'lstat' don't need that.  But here we do want that
+        information to be accurate.  */
+      w32_stat_get_owner_group = 1;
+#endif
+      if (fstatat (fd, name, &s, AT_SYMLINK_NOFOLLOW) == 0)
+       err = 0;
 #ifdef WINDOWSNT
-  w32_stat_get_owner_group = 0;
+      w32_stat_get_owner_group = 0;
 #endif
+    }
 
-  if (lstat_result < 0)
-    return Qnil;
+  if (err != 0)
+    return unbind_to (count, Qnil);
+
+  Lisp_Object file_type;
+  if (S_ISLNK (s.st_mode))
+    {
+      /* On systems lacking O_PATH support there is a race if the
+        symlink is replaced between the call to fstatat and the call
+        to emacs_readlinkat.  Detect this race unless the replacement
+        is also a symlink.  */
+      file_type = emacs_readlinkat (fd, name);
+      if (NILP (file_type))
+       return unbind_to (count, Qnil);
+    }
+  else
+    file_type = S_ISDIR (s.st_mode) ? Qt : Qnil;
+
+  unbind_to (count, Qnil);
 
   if (!(NILP (id_format) || EQ (id_format, Qinteger)))
     {
@@ -964,8 +1007,7 @@ file_attributes (int fd, char const *name, Lisp_Object 
id_format)
   filemodestring (&s, modes);
 
   return CALLN (Flist,
-               (S_ISLNK (s.st_mode) ? emacs_readlinkat (fd, name)
-                : S_ISDIR (s.st_mode) ? Qt : Qnil),
+               file_type,
                make_number (s.st_nlink),
                (uname
                 ? DECODE_SYSTEM (build_unibyte_string (uname))
diff --git a/src/kqueue.c b/src/kqueue.c
index a8eb4cb..30922ef 100644
--- a/src/kqueue.c
+++ b/src/kqueue.c
@@ -130,7 +130,7 @@ kqueue_compare_dir_list (Lisp_Object watch_object)
     return;
   }
   new_directory_files =
-    directory_files_internal (dir, Qnil, Qnil, Qnil, 1, Qnil);
+    directory_files_internal (dir, Qnil, Qnil, Qnil, true, Qnil);
   new_dl = kqueue_directory_listing (new_directory_files);
 
   /* Parse through the old list.  */
@@ -453,7 +453,7 @@ only when the upper directory of the renamed file is 
watched.  */)
   if (NILP (Ffile_directory_p (file)))
     watch_object = list4 (watch_descriptor, file, flags, callback);
   else {
-    dir_list = directory_files_internal (file, Qnil, Qnil, Qnil, 1, Qnil);
+    dir_list = directory_files_internal (file, Qnil, Qnil, Qnil, true, Qnil);
     watch_object = list5 (watch_descriptor, file, flags, callback, dir_list);
   }
   watch_list = Fcons (watch_object, watch_list);
diff --git a/src/sysdep.c b/src/sysdep.c
index 12e9c83..b66a745 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2930,7 +2930,7 @@ list_system_processes (void)
      process.  */
   procdir = build_string ("/proc");
   match = build_string ("[0-9]+");
-  proclist = directory_files_internal (procdir, Qnil, match, Qt, 0, Qnil);
+  proclist = directory_files_internal (procdir, Qnil, match, Qt, false, Qnil);
 
   /* `proclist' gives process IDs as strings.  Destructively convert
      each string into a number.  */