bug#53136: 28.0.90; segfault in lock_file

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

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

On 1/11/22 05:16, Po Lu wrote:

>  > Fixing each place individually is a time bomb: no one will remember
>  > that after enough time has passed, and we will add bugs.
> 
>  > The only alternative is to filter errno via some macro, which will do
>  > nothing on all platforms but Haiku, where it will map them to Posix
>  > values. Butt-ugly, but it's not our fault.
> 
> Hmm, perhaps gnulib can do something in this area? Paul, any ideas?
> Thanks in advance.

Here are three possibilities:

1. Carefully audit the many hundreds of use of errno values in Emacs and 
make sure they still work even on Haiku, updating Gnulib accordingly. I 
think we agree this is a big and continuing maintenance hassle. I just 
did a bit more of this sort of thing in filelock.c (please see attached) 
and would rather not do more.


2. Change Gnulib so that on Haiku Gnulib wraps errno-producing functions 
called by Emacs (and by Gnulib), so that they negate errno before 
returning. Gnulib would also wrap errno.h so that errno constants are 
positive. This would affect only calls from Emacs code; it wouldn't 
affect calls from Haiku libraries, so file dialogs would continue to work.

Unfortunately this would be a maintenance hassle too. I suppose we could 
adjust Gnulib to wrap only the errno-producing functions that Emacs 
cares about, either directly or indirectly via Gnulib. However, I expect 
there are some such functions not in the standard library, and I doubt 
whether we'd want to change Gnulib to wrap random functions in random 
libraries. For these functions, we'd need to modify Emacs much as we 
already modified filelock.c.


3. Compile Emacs code with B_USE_POSITIVE_POSIX_ERRORS, and use linker 
magic only on this code; do not use linker magic on library code (e.g., 
-ltracker) so that Haiku library code uses the original functions. That 
way, Emacs source code proper would need to worry about negative errno 
values only in haiku*.c files that call Haiku-specific libraries. If 
this is feasible, it should be much less work and more maintainable. 
Could you look into that?

[-- Attachment #2: 0001-Clean-up-filelock-code-related-to-errno.patch --]
[-- Type: text/x-patch, Size: 6194 bytes --]

From 80b054b66b778d374d11620b7949727596dc2c35 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 11 Jan 2022 08:58:18 -0800
Subject: [PATCH] Clean up filelock code related to errno

Reduce dependency on Haiku internals, by not assuming that
Haiku errno values (which are negative) are neither -1 nor -2.
This removes an #ifdef HAIKU while still maintaining
portability to Haiku.
* src/filelock.c (NEGATIVE_ERRNO, ANOTHER_OWNS_IT, I_OWN_IT):
New constants, which should work regardless of whether
we are on Haiku or B_USE_POSITIVE_POSIX_ERRORS is defined.
(current_lock_owner, lock_if_free, lock_file, unlock_file)
(Ffile_locked_p): Use them, without assuming anything about errno
value sign.
---
 src/filelock.c | 69 +++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 31 deletions(-)

diff --git a/src/filelock.c b/src/filelock.c
index 3555cfc251..eb8d9ab5e0 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -490,15 +490,29 @@ read_lock_data (char *lfname, char lfinfo[MAX_LFINFO + 1])
   return nbytes;
 }
 
+/* True if errno values are negative.  Although the C standard
+   requires them to be positive, they are negative in Haiku.  */
+enum { NEGATIVE_ERRNO = EDOM < 0 };
+
+/* Nonzero values that are not errno values.  */
+enum
+  {
+    /* Another process on this machine owns it.  */
+    ANOTHER_OWNS_IT = NEGATIVE_ERRNO ? 1 : -1,
+
+    /* This Emacs process owns it.  */
+    I_OWN_IT = 2 * ANOTHER_OWNS_IT
+  };
+
 /* Return 0 if nobody owns the lock file LFNAME or the lock is obsolete,
-   -1 if another process owns it (and set OWNER (if non-null) to info),
-   -2 if the current process owns it,
+   ANOTHER_OWNS_IT if another process owns it
+     (and set OWNER (if non-null) to info),
+   I_OWN_IT if the current process owns it,
    or an errno value if something is wrong with the locking mechanism.  */
 
 static int
 current_lock_owner (lock_info_type *owner, char *lfname)
 {
-  int ret;
   lock_info_type local_owner;
   ptrdiff_t lfinfolen;
   intmax_t pid, boot_time;
@@ -571,13 +585,13 @@ current_lock_owner (lock_info_type *owner, char *lfname)
       && memcmp (at + 1, SSDATA (system_name), SBYTES (system_name)) == 0)
     {
       if (pid == getpid ())
-        ret = -2; /* We own it.  */
+        return I_OWN_IT;
       else if (0 < pid && pid <= TYPE_MAXIMUM (pid_t)
                && (kill (pid, 0) >= 0 || errno == EPERM)
 	       && (boot_time == 0
 		   || (boot_time <= TYPE_MAXIMUM (time_t)
 		       && within_one_second (boot_time, get_boot_time ()))))
-        ret = -1; /* An existing process on this machine owns it.  */
+        return ANOTHER_OWNS_IT;
       /* The owner process is dead or has a strange pid, so try to
          zap the lockfile.  */
       else
@@ -586,18 +600,16 @@ current_lock_owner (lock_info_type *owner, char *lfname)
   else
     { /* If we wanted to support the check for stale locks on remote machines,
          here's where we'd do it.  */
-      ret = -1;
+      return ANOTHER_OWNS_IT;
     }
-
-  return ret;
 }
 
 \f
 /* Lock the lock named LFNAME if possible.
    Return 0 in that case.
-   Return negative if some other process owns the lock, and info about
+   Return ANOTHER_OWNS_IT if some other process owns the lock, and info about
      that process in CLASHER.
-   Return positive errno value if cannot lock for any other reason.  */
+   Return errno value if cannot lock for any other reason.  */
 
 static int
 lock_if_free (lock_info_type *clasher, char *lfname)
@@ -606,24 +618,17 @@ lock_if_free (lock_info_type *clasher, char *lfname)
   while ((err = lock_file_1 (lfname, 0)) == EEXIST)
     {
       err = current_lock_owner (clasher, lfname);
+
+      /* Return if we locked it, or another process owns it, or it is
+	 a strange error.  */
       if (err != 0)
-	{
-	  if (err == -1 || err == -2)
-	    return -2 - err; /* We locked it, or someone else has it.  */
-	  break; /* current_lock_owner returned strange error.  */
-	}
+	return err == I_OWN_IT ? 0 : err;
 
-      /* We deleted a stale lock; try again to lock the file.  */
+      /* We deleted a stale lock or some other process deleted the lock;
+	 try again to lock the file.  */
     }
 
-#if !defined HAIKU \
-  || defined B_USE_POSITIVE_POSIX_ERRORS
   return err;
-#else
-  /* On Haiku, POSIX errno values are negative by default, but this
-     code's callers assume that all errno values are positive.  */
-  return -err;
-#endif
 }
 
 static Lisp_Object
@@ -681,12 +686,12 @@ lock_file (Lisp_Object fn)
   if (!NILP (subject_buf)
       && NILP (Fverify_visited_file_modtime (subject_buf))
       && !NILP (Ffile_exists_p (fn))
-      && current_lock_owner (NULL, lfname) != -2)
+      && current_lock_owner (NULL, lfname) != I_OWN_IT)
     call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
 
   /* Try to lock the lock.  FIXME: This ignores errors when
-     lock_if_free returns a positive errno value.  */
-  if (lock_if_free (&lock_info, lfname) < 0)
+     lock_if_free returns an errno value.  */
+  if (lock_if_free (&lock_info, lfname) == ANOTHER_OWNS_IT)
     {
       /* Someone else has the lock.  Consider breaking it.  */
       Lisp_Object attack;
@@ -717,9 +722,9 @@ unlock_file (Lisp_Object fn)
   lfname = SSDATA (ENCODE_FILE (lock_filename));
 
   int err = current_lock_owner (0, lfname);
-  if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
-    err = errno;
-  if (0 < err)
+  if (! (err == 0 || err == ANOTHER_OWNS_IT
+	 || (err == I_OWN_IT
+	     && (unlink (lfname) == 0 || (err = errno) == ENOENT))))
     report_file_errno ("Unlocking file", fn, err);
 
   return Qnil;
@@ -865,8 +870,10 @@ DEFUN ("file-locked-p", Ffile_locked_p, Sfile_locked_p, 1, 1, 0,
   owner = current_lock_owner (&locker, lfname);
   switch (owner)
     {
-    case -2: ret = Qt; break;
-    case -1: ret = make_string (locker.user, locker.at - locker.user); break;
+    case I_OWN_IT: ret = Qt; break;
+    case ANOTHER_OWNS_IT:
+      ret = make_string (locker.user, locker.at - locker.user);
+      break;
     case  0: ret = Qnil; break;
     default: report_file_errno ("Testing file lock", filename, owner);
     }
-- 
2.32.0