From 2b7e009257a40ef1dcad9845fe61764fea08cdea Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Fri, 25 Aug 2017 12:44:52 -0700 Subject: [PATCH] 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 5a52765131c..36944e4713d 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 non-@code{nil} 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 bf59749a62b..02de66b355f 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -1427,6 +1427,12 @@ job of signaling list cycles instead of looping indefinitely. ** The new functions 'make-nearby-temp-file' and 'temporary-file-directory' 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 288ba6b1038..128493aff28 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 a8eb4cb797c..30922ef28b1 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 12e9c83ee90..b66a7453172 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. */ -- 2.39.2