bug#72641: 31.0.50; "Unlocking file: Invalid argument" when deleting lock file on network file system

[Paul Eggert <eggert@cs.ucla.edu>]

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

On 2024-08-15 17:59, Michal Nazarewicz wrote:

> However, if I save the file (be it by save-buffer or by killing the
> buffer and picking save option), the lock file remains....
> - unlock_file is called
>    - it calls current_lock_owner
>      - which return ENOENT for some reason
>    - the lock file is left alone

Obviously current_lock_owner should not return ENOENT if there is an 
existing lock file - that would defeat the purpose of having a lock 
file. We need to get to the bottom of why current_lock_owner returns ENOENT.

 From inspection, current_lock_owner returns ENOENT only if Emacs 
notices that the "lock" file is actually an empty regular file (or looks 
stale), and calls 'unlink' on it, and 'unlink' fails with errno == 
ENOENT. Is that what's actually happening? You can use a debugger or 
'strace' to confirm.

Come to think of it, if 'unlink' fails with errno == ENOENT, that means 
there's no lock file so current_lock_owner should return 0. This is true 
because of NFS and similar network file systems where unlink can fail 
even though it actually removed the file. I installed the attached patch 
to fix this; a similar problem exists elsewhere, so this patch fixes all 
the instances of it in Emacs master.

With this patch, current_lock_owner should never return ENOENT and we 
can move on to the next problem you observe, if there is one.


> In desperation I’ve tried attached patch (it adds `if (!lfinfolen)
> return I_OWN_IT`; big diff is because than `if (lfinfolen)` check can be
> removed and code dedented) and it worked.

Yes, that doesn't sound right; on non-buggy file systems the invalid 
lock file would cause races to occur, if I'm reading the code correctly.

Let's see what happens with current master before proceeding in any 
direction like that.

[-- Attachment #2: 0001-Port-better-to-NFS-unlink.patch --]
[-- Type: text/x-patch, Size: 2738 bytes --]

From 9aad7664813d77f4dab2649d56adbf12239b7505 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 15 Aug 2024 20:10:53 -0700
Subject: [PATCH] Port better to NFS unlink
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I found this problem while looking into Bug#72641.
* lib-src/etags.c (do_move_file):
* lib-src/update-game-score.c (unlock_file):
* src/androidvfs.c (android_hack_asset_fd_fallback):
* src/filelock.c (current_lock_owner):
Treat unlink as successful if it fails because the file wasn’t there.
This can happen with some NFS implementations, due to its
retrying over the network to get at-least-once semantics.
Although most of Emacs’s calls to unlink were already doing this,
a few instances were not.
---
 lib-src/etags.c             | 2 +-
 lib-src/update-game-score.c | 2 +-
 src/androidvfs.c            | 2 +-
 src/filelock.c              | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index 03bc55de03d..edadbc25901 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -7812,7 +7812,7 @@ do_move_file (const char *src_file, const char *dst_file)
   if (fclose (dst_f) == EOF)
     pfatal (dst_file);
 
-  if (unlink (src_file) == -1)
+  if (unlink (src_file) < 0 && errno != ENOENT)
     pfatal ("unlink error");
 
   return;
diff --git a/lib-src/update-game-score.c b/lib-src/update-game-score.c
index 4139073bcd7..e3b24ad7717 100644
--- a/lib-src/update-game-score.c
+++ b/lib-src/update-game-score.c
@@ -497,7 +497,7 @@ unlock_file (const char *filename, void *state)
   char *lockpath = (char *) state;
   int saved_errno = errno;
   int ret = unlink (lockpath);
-  if (0 <= ret)
+  if (! (ret < 0 && errno != ENOENT))
     errno = saved_errno;
   free (lockpath);
   return ret;
diff --git a/src/androidvfs.c b/src/androidvfs.c
index 14da8eed37e..ff81ef288f5 100644
--- a/src/androidvfs.c
+++ b/src/androidvfs.c
@@ -1323,7 +1323,7 @@ android_hack_asset_fd_fallback (AAsset *asset)
   if (fd < 0)
     return -1;
 
-  if (unlink (filename))
+  if (unlink (filename) && errno != ENOENT)
     goto fail;
 
   if (ftruncate (fd, size))
diff --git a/src/filelock.c b/src/filelock.c
index 1ae57dc7344..bc09fce69f8 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -501,7 +501,7 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
      the file system is buggy, e.g., <https://bugs.gnu.org/72641>.
      Emacs never creates empty lock files even temporarily, so removing
      an empty lock file should be harmless.  */
-  return emacs_unlink (SSDATA (lfname)) < 0 ? errno : 0;
+  return emacs_unlink (SSDATA (lfname)) < 0 && errno != ENOENT ? errno : 0;
 }
 
 \f
-- 
2.43.0