bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Vivien Mallet]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

Dear Emacs developers,

In Emacs 23 and Emacs 24 (built sometime ago from Git repository),
"(verify-visited-file-modtime (current-buffer))" sometimes returns 'nil'
instead of 't'. This happens often, even if the file has not been
modified by another program.

One reason is that stat can return an EINTR error. I am running
Linux. According to the manpages, stat is not supposed to fail with
EINTR under a Posix system (contrary to SVr4), but this is what I
observe. I checked with a call to printf right after the error, and I
got errno == 4 which is EINTR here. After exposing this on IRC #emacs, I
was told it could be due to the NFS mount (with option "intr") as the
files are on a network NFS volume. Hence I modified fileio.c to call
stat again after an EINTR error is raised. I attach the patch. If it is
confirmed that stat can return EINTR, there might be other calls to stat
in Emacs that need the correction.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fileio.patch --]
[-- Type: text/x-diff, Size: 808 bytes --]

diff --git a/src/fileio.c b/src/fileio.c
index 5d33fb9..611ccff 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4956,6 +4956,7 @@ See Info node `(elisp)Modification Time' for more details.  */)
   struct stat st;
   Lisp_Object handler;
   Lisp_Object filename;
+  int stat_status;
 
   if (NILP (buf))
     b = current_buffer;
@@ -4977,7 +4978,10 @@ See Info node `(elisp)Modification Time' for more details.  */)
 
   filename = ENCODE_FILE (BVAR (b, filename));
 
-  if (stat (SSDATA (filename), &st) < 0)
+  while ((stat_status = stat (SSDATA (filename), &st)) < 0 && errno == EINTR);
+
+  /* if (stat (SSDATA (filename), &st) < 0) */
+  if (stat_status < 0)
     {
       /* If the file doesn't exist now and didn't exist before,
 	 we say that it isn't modified, provided the error is a tame one.  */

[-- Attachment #3: Type: text/plain, Size: 1124 bytes --]


This solved only part of the problem. I still get a lot of false 'nil'
from "(verify-visited-file-modtime (current-buffer))" because stat
returns ENOENT. One may argue this comes from my network, but the
problem appears only inside Emacs. I wrote a small program to call stat
a (huge) number of times on a regular basis (on a file previously opened
by the program), but I did not get a single error. Maybe the way Emacs
opens the files has something to do with that? I would be grateful to
you if you could suggest a few tests to find out why Emacs may
experience problems with stat while other programs do not seem to face
the issue at all (no EINTR, no ENOENT).

Note that, on this network, the problem is met by all Emacs users I
know, whatever their Emacs configuration. The computers are running
Linux 2.6.32-31-generic x86_64, under Ubuntu 10.10. The involved Emacs
versions are 23.1.1 (Ubuntu package) and 24 (git commit
78cda4761357774eb9daa19546a078f24aa1ee66 from 19 March 2011). I was told
the problem does not appear in Emacs 22, but I did not check it myself.

Thank you for the great work.

Best regards,
VM.

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Lars Ingebrigtsen]

Vivien Mallet <Vivien.Mallet@inria.fr> writes:

> One reason is that stat can return an EINTR error. I am running
> Linux. According to the manpages, stat is not supposed to fail with
> EINTR under a Posix system (contrary to SVr4), but this is what I
> observe. I checked with a call to printf right after the error, and I
> got errno == 4 which is EINTR here. After exposing this on IRC #emacs, I
> was told it could be due to the NFS mount (with option "intr") as the
> files are on a network NFS volume.

(There was unfortunately no response to this bug report when it was
reported eight years ago.)

Hm.  I'm not sure you can expect things to work with a setup like that,
really: I'd expect things to bug out pretty regularly across the board,
since you'd have to check for EINTR in every single call to a bunch of
system calls, and basically do what you do here everywhere:

>    filename = ENCODE_FILE (BVAR (b, filename));
 
> -  if (stat (SSDATA (filename), &st) < 0)
> +  while ((stat_status = stat (SSDATA (filename), &st)) < 0 && errno == EINTR);
> +
> +  /* if (stat (SSDATA (filename), &st) < 0) */
> +  if (stat_status < 0)

> This solved only part of the problem. I still get a lot of false 'nil'
> from "(verify-visited-file-modtime (current-buffer))" because stat
> returns ENOENT. One may argue this comes from my network, but the
> problem appears only inside Emacs.

Indeed.  Does anybody have an opinion on whether this is a configuration
Emacs supports?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

> I'd expect things to bug out pretty regularly across the board,
> since you'd have to check for EINTR in every single call to a bunch of
> system calls

Yes in theory. However, Emacs already does the EINTR check for open, read and 
write even on regular files where POSIX says it can't happen (but it does happen 
with NFS). If you've recently dealt with an NFS file then it'll be cached on the 
client and you won't get EINTR, so in practice the issue comes up only for 
syscalls that are applied to a file that hasn't been looked at lately. stat is 
one of these calls (hence the bug report) so we might as well do the EINTR check 
for it as well. I installed the attached patch to do that for stat and similar 
calls, and also for openat (which I think was overlooked when 'open' was done).

The other part of this bug report (with ENOENT) is not something Emacs can work 
around and it's surely a bug in the Linux NFS client that was most likely fixed 
a while ago anyway <https://bugzilla.kernel.org/show_bug.cgi?id=14541>.

As I think both issues in the bug report have been addressed, I'm boldly closing it.

[-- Attachment #2: 0001-Work-better-if-stat-etc.-are-interrupted.patch --]
[-- Type: text/x-patch, Size: 12103 bytes --]

From b3ad638a60845f17938ff812efcf2b2edfbd8c57 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 20 Jan 2020 01:08:42 -0800
Subject: [PATCH] Work better if stat etc. are interrupted

Quit or retry if fstat, lstat, stat or openat fail with EINTR.
This should fix some bugs on platforms where accessing files via
NFS can fail that way (Bug#9256).
* src/dired.c (file_attributes):
* src/fileio.c (file_directory_p) [O_PATH]:
Use emacs_openat instead of openat.
* src/dired.c (file_attributes): Use emacs_fstatat instead of fstatat.
* src/fileio.c (barf_or_query_if_file_exists, Frename_file):
* src/filelock.c (rename_lock_file):
Use emacs_fstatat instead of lstat.
* src/fileio.c (file_directory_p, Ffile_regular_p, Ffile_modes)
(Ffile_newer_than_file_p, Fverify_visited_file_modtime)
(Fset_visited_file_modtime, auto_save_1):
* src/lread.c (Fload):
* src/sysdep.c (get_current_dir_name_or_unreachable):
Use emacs_fstatat instead of stat.
* src/sysdep.c (emacs_fstatat, emacs_openat): New functions.
(emacs_open): Redo in terms of emacs_open.
---
 src/dired.c    |  4 ++--
 src/fileio.c   | 39 ++++++++++++++++++++++++---------------
 src/filelock.c |  3 ++-
 src/lisp.h     |  2 ++
 src/lread.c    |  4 ++--
 src/sysdep.c   | 36 +++++++++++++++++++++++++++++++-----
 6 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/src/dired.c b/src/dired.c
index 611477aa4e..f013a4cea0 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -937,7 +937,7 @@ file_attributes (int fd, char const *name,
   int err = EINVAL;
 
 #if defined O_PATH && !defined HAVE_CYGWIN_O_PATH_BUG
-  int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
+  int namefd = emacs_openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW, 0);
   if (namefd < 0)
     err = errno;
   else
@@ -970,7 +970,7 @@ file_attributes (int fd, char const *name,
 	 information to be accurate.  */
       w32_stat_get_owner_group = 1;
 #endif
-      err = fstatat (fd, name, &s, AT_SYMLINK_NOFOLLOW) == 0 ? 0 : errno;
+      err = emacs_fstatat (fd, name, &s, AT_SYMLINK_NOFOLLOW) == 0 ? 0 : errno;
 #ifdef WINDOWSNT
       w32_stat_get_owner_group = 0;
 #endif
diff --git a/src/fileio.c b/src/fileio.c
index 34934dd6df..87a17eab42 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -1952,7 +1952,10 @@ barf_or_query_if_file_exists (Lisp_Object absname, bool known_to_exist,
 
   encoded_filename = ENCODE_FILE (absname);
 
-  if (! known_to_exist && lstat (SSDATA (encoded_filename), &statbuf) == 0)
+  if (! known_to_exist
+      && (emacs_fstatat (AT_FDCWD, SSDATA (encoded_filename),
+			 &statbuf, AT_SYMLINK_NOFOLLOW)
+	  == 0))
     {
       if (S_ISDIR (statbuf.st_mode))
 	xsignal2 (Qfile_error,
@@ -2555,7 +2558,9 @@ This is what happens in interactive use with M-x.  */)
   bool dirp = !NILP (Fdirectory_name_p (file));
   if (!dirp)
     {
-      if (lstat (SSDATA (encoded_file), &file_st) != 0)
+      if (emacs_fstatat (AT_FDCWD, SSDATA (encoded_file),
+			 &file_st, AT_SYMLINK_NOFOLLOW)
+	  != 0)
 	report_file_error ("Renaming", list2 (file, newname));
       dirp = S_ISDIR (file_st.st_mode) != 0;
     }
@@ -2928,7 +2933,8 @@ file_directory_p (Lisp_Object file)
 #else
 # ifdef O_PATH
   /* Use O_PATH if available, as it avoids races and EOVERFLOW issues.  */
-  int fd = openat (AT_FDCWD, SSDATA (file), O_PATH | O_CLOEXEC | O_DIRECTORY);
+  int fd = emacs_openat (AT_FDCWD, SSDATA (file),
+			 O_PATH | O_CLOEXEC | O_DIRECTORY, 0);
   if (0 <= fd)
     {
       emacs_close (fd);
@@ -2939,9 +2945,9 @@ file_directory_p (Lisp_Object file)
   /* O_PATH is defined but evidently this Linux kernel predates 2.6.39.
      Fall back on generic POSIX code.  */
 # endif
-  /* Use file_accessible_directory_p, as it avoids stat EOVERFLOW
+  /* Use file_accessible_directory_p, as it avoids fstatat EOVERFLOW
      problems and could be cheaper.  However, if it fails because FILE
-     is inaccessible, fall back on stat; if the latter fails with
+     is inaccessible, fall back on fstatat; if the latter fails with
      EOVERFLOW then FILE must have been a directory unless a race
      condition occurred (a problem hard to work around portably).  */
   if (file_accessible_directory_p (file))
@@ -2949,7 +2955,7 @@ file_directory_p (Lisp_Object file)
   if (errno != EACCES)
     return false;
   struct stat st;
-  if (stat (SSDATA (file), &st) != 0)
+  if (emacs_fstatat (AT_FDCWD, SSDATA (file), &st, 0) != 0)
     return errno == EOVERFLOW;
   if (S_ISDIR (st.st_mode))
     return true;
@@ -3080,7 +3086,7 @@ See `file-symlink-p' to distinguish symlinks.  */)
   Vw32_get_true_file_attributes = Qt;
 #endif
 
-  int stat_result = stat (SSDATA (absname), &st);
+  int stat_result = emacs_fstatat (AT_FDCWD, SSDATA (absname), &st, 0);
 
 #ifdef WINDOWSNT
   Vw32_get_true_file_attributes = true_attributes;
@@ -3340,7 +3346,7 @@ Return nil if FILENAME does not exist.  */)
   if (!NILP (handler))
     return call2 (handler, Qfile_modes, absname);
 
-  if (stat (SSDATA (ENCODE_FILE (absname)), &st) != 0)
+  if (emacs_fstatat (AT_FDCWD, SSDATA (ENCODE_FILE (absname)), &st, 0) != 0)
     return file_attribute_errno (absname, errno);
   return make_fixnum (st.st_mode & 07777);
 }
@@ -3486,7 +3492,7 @@ otherwise, if FILE2 does not exist, the answer is t.  */)
     return call3 (handler, Qfile_newer_than_file_p, absname1, absname2);
 
   int err1;
-  if (stat (SSDATA (ENCODE_FILE (absname1)), &st1) == 0)
+  if (emacs_fstatat (AT_FDCWD, SSDATA (ENCODE_FILE (absname1)), &st1, 0) == 0)
     err1 = 0;
   else
     {
@@ -3494,7 +3500,7 @@ otherwise, if FILE2 does not exist, the answer is t.  */)
       if (err1 != EOVERFLOW)
 	return file_attribute_errno (absname1, err1);
     }
-  if (stat (SSDATA (ENCODE_FILE (absname2)), &st2) != 0)
+  if (emacs_fstatat (AT_FDCWD, SSDATA (ENCODE_FILE (absname2)), &st2, 0) != 0)
     {
       file_attribute_errno (absname2, errno);
       return Qt;
@@ -3880,7 +3886,7 @@ by calling `format-decode', which see.  */)
 	  if (end_offset < 0)
 	    buffer_overflow ();
 
-	  /* The file size returned from stat may be zero, but data
+	  /* The file size returned from fstat may be zero, but data
 	     may be readable nonetheless, for example when this is a
 	     file in the /proc filesystem.  */
 	  if (end_offset == 0)
@@ -5625,7 +5631,7 @@ See Info node `(elisp)Modification Time' for more details.  */)
 
   filename = ENCODE_FILE (BVAR (b, filename));
 
-  mtime = (stat (SSDATA (filename), &st) == 0
+  mtime = (emacs_fstatat (AT_FDCWD, SSDATA (filename), &st, 0) == 0
 	   ? get_stat_mtime (&st)
 	   : time_error_value (errno));
   if (timespec_cmp (mtime, b->modtime) == 0
@@ -5689,7 +5695,8 @@ in `current-time' or an integer flag as returned by `visited-file-modtime'.  */)
 	/* The handler can find the file name the same way we did.  */
 	return call2 (handler, Qset_visited_file_modtime, Qnil);
 
-      if (stat (SSDATA (ENCODE_FILE (filename)), &st) == 0)
+      if (emacs_fstatat (AT_FDCWD, SSDATA (ENCODE_FILE (filename)), &st, 0)
+	  == 0)
         {
 	  current_buffer->modtime = get_stat_mtime (&st);
           current_buffer->modtime_size = st.st_size;
@@ -5728,12 +5735,14 @@ auto_save_1 (void)
   /* Get visited file's mode to become the auto save file's mode.  */
   if (! NILP (BVAR (current_buffer, filename)))
     {
-      if (stat (SSDATA (BVAR (current_buffer, filename)), &st) >= 0)
+      if (emacs_fstatat (AT_FDCWD, SSDATA (BVAR (current_buffer, filename)),
+			 &st, 0)
+	  == 0)
 	/* But make sure we can overwrite it later!  */
 	auto_save_mode_bits = (st.st_mode | 0600) & 0777;
       else if (modes = Ffile_modes (BVAR (current_buffer, filename)),
 	       FIXNUMP (modes))
-	/* Remote files don't cooperate with stat.  */
+	/* Remote files don't cooperate with fstatat.  */
 	auto_save_mode_bits = (XFIXNUM (modes) | 0600) & 0777;
     }
 
diff --git a/src/filelock.c b/src/filelock.c
index b28f16e9b5..73202f0b2c 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -347,7 +347,8 @@ rename_lock_file (char const *old, char const *new, bool force)
 	 potential race condition since some other process may create
 	 NEW immediately after the existence check, but it's the best
 	 we can portably do here.  */
-      if (lstat (new, &st) == 0 || errno == EOVERFLOW)
+      if (emacs_fstatat (AT_FDCWD, new, &st, AT_SYMLINK_NOFOLLOW) == 0
+	  || errno == EOVERFLOW)
 	{
 	  errno = EEXIST;
 	  return -1;
diff --git a/src/lisp.h b/src/lisp.h
index 4bcd122844..0bd375658e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4605,6 +4605,8 @@ extern void seed_random (void *, ptrdiff_t);
 extern void init_random (void);
 extern void emacs_backtrace (int);
 extern AVOID emacs_abort (void) NO_INLINE;
+extern int emacs_fstatat (int, char const *, void *, int);
+extern int emacs_openat (int, char const *, int, int);
 extern int emacs_open (const char *, int, int);
 extern int emacs_pipe (int[2]);
 extern int emacs_close (int);
diff --git a/src/lread.c b/src/lread.c
index 4e9860d5dc..69dd73912b 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1353,11 +1353,11 @@ Return t if the file exists and loads successfully.  */)
              ignores suffix order due to load_prefer_newer.  */
           if (!load_prefer_newer && is_elc)
             {
-              result = stat (SSDATA (efound), &s1);
+	      result = emacs_fstatat (AT_FDCWD, SSDATA (efound), &s1, 0);
               if (result == 0)
                 {
                   SSET (efound, SBYTES (efound) - 1, 0);
-                  result = stat (SSDATA (efound), &s2);
+		  result = emacs_fstatat (AT_FDCWD, SSDATA (efound), &s2, 0);
                   SSET (efound, SBYTES (efound) - 1, 'c');
                 }
 
diff --git a/src/sysdep.c b/src/sysdep.c
index c6344d8cec..e8e8bbfb50 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -312,8 +312,8 @@ get_current_dir_name_or_unreachable (void)
   if (pwd
       && (pwdlen = strnlen (pwd, bufsize_max)) < bufsize_max
       && IS_DIRECTORY_SEP (pwd[pwdlen && IS_DEVICE_SEP (pwd[1]) ? 2 : 0])
-      && stat (pwd, &pwdstat) == 0
-      && stat (".", &dotstat) == 0
+      && emacs_fstatat (AT_FDCWD, pwd, &pwdstat, 0) == 0
+      && emacs_fstatat (AT_FDCWD, ".", &dotstat, 0) == 0
       && dotstat.st_ino == pwdstat.st_ino
       && dotstat.st_dev == pwdstat.st_dev)
     {
@@ -2449,7 +2449,27 @@ emacs_abort (void)
 }
 #endif
 
-/* Open FILE for Emacs use, using open flags OFLAG and mode MODE.
+/* Assuming the directory DIRFD, store information about FILENAME into *ST,
+   using FLAGS to control how the status is obtained.
+   Do not fail merely because fetching info was interrupted by a signal.
+   Allow the user to quit.
+
+   The type of ST is void * instead of struct stat * because the
+   latter type would be problematic in lisp.h.  Some platforms may
+   play tricks like "#define stat stat64" in <sys/stat.h>, and lisp.h
+   does not include <sys/stat.h>.  */
+
+int
+emacs_fstatat (int dirfd, char const *filename, void *st, int flags)
+{
+  int r;
+  while ((r = fstatat (dirfd, filename, st, flags)) != 0 && errno == EINTR)
+    maybe_quit ();
+  return r;
+}
+
+/* Assuming the directory DIRFD, open FILE for Emacs use,
+   using open flags OFLAGS and mode MODE.
    Use binary I/O on systems that care about text vs binary I/O.
    Arrange for subprograms to not inherit the file descriptor.
    Prefer a method that is multithread-safe, if available.
@@ -2457,17 +2477,23 @@ emacs_abort (void)
    Allow the user to quit.  */
 
 int
-emacs_open (const char *file, int oflags, int mode)
+emacs_openat (int dirfd, char const *file, int oflags, int mode)
 {
   int fd;
   if (! (oflags & O_TEXT))
     oflags |= O_BINARY;
   oflags |= O_CLOEXEC;
-  while ((fd = open (file, oflags, mode)) < 0 && errno == EINTR)
+  while ((fd = openat (dirfd, file, oflags, mode)) < 0 && errno == EINTR)
     maybe_quit ();
   return fd;
 }
 
+int
+emacs_open (char const *file, int oflags, int mode)
+{
+  return emacs_openat (AT_FDCWD, file, oflags, mode);
+}
+
 /* Open FILE as a stream for Emacs use, with mode MODE.
    Act like emacs_open with respect to threads, signals, and quits.  */
 
-- 
2.17.1

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Andy Moreton]

On Mon 20 Jan 2020, Paul Eggert wrote:

>> I'd expect things to bug out pretty regularly across the board,
>> since you'd have to check for EINTR in every single call to a bunch of
>> system calls
>
> Yes in theory. However, Emacs already does the EINTR check for open, read and
> write even on regular files where POSIX says it can't happen (but it does
> happen with NFS). If you've recently dealt with an NFS file then it'll be
> cached on the client and you won't get EINTR, so in practice the issue comes
> up only for syscalls that are applied to a file that hasn't been looked at
> lately. stat is one of these calls (hence the bug report) so we might as well
> do the EINTR check for it as well. I installed the attached patch to do that
> for stat and similar calls, and also for openat (which I think was overlooked
> when 'open' was done).
>
> The other part of this bug report (with ENOENT) is not something Emacs can
> work around and it's surely a bug in the Linux NFS client that was most likely
> fixed a while ago anyway <https://bugzilla.kernel.org/show_bug.cgi?id=14541>.
>
> As I think both issues in the bug report have been addressed, I'm boldly closing it.

This patch breaks the build for Windows, as it appears that MinGW does
not have openat:

  CCLD     temacs.exe
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/9.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: sysdep.o: in function `emacs_openat':
C:/emacs/git/emacs/master/src/sysdep.c:2486: undefined reference to `openat'
C:/emacs/git/emacs/master/src/sysdep.c:2486:(.text+0x1359): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `openat'
collect2.exe: error: ld returned 1 exit status

Presumably this will need updates to w32.c to add the necessary support.

    AndyM

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 20 Jan 2020 15:36:53 +0000
> 
> > As I think both issues in the bug report have been addressed, I'm boldly closing it.
> 
> This patch breaks the build for Windows, as it appears that MinGW does
> not have openat:

It does now.

bug#9256: Erroneous output from "verify-visited-file-modtime" (fileio.c)

[Andy Moreton]

On Mon 20 Jan 2020, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Mon, 20 Jan 2020 15:36:53 +0000
>> 
>> > As I think both issues in the bug report have been addressed, I'm boldly closing it.
>> 
>> This patch breaks the build for Windows, as it appears that MinGW does
>> not have openat:
>
> It does now.

Thanks Eli, confirmed fixed.

    AndyM