bug#53136: 28.0.90; segfault in lock_file

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

This write in `lock_file':

      memcpy (dot, replacement, replacementlen);

Ends up writing one byte outside dot.

I could not find the problem, but someone else may want to take a look.
Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> Resent-From: Po Lu <luangruo@yahoo.com>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> Date: Sun, 09 Jan 2022 14:04:23 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This write in `lock_file':
> 
>       memcpy (dot, replacement, replacementlen);
> 
> Ends up writing one byte outside dot.
> 
> I could not find the problem, but someone else may want to take a look.

Details, please: the values of all the relevant variables at that
point, including the contents of all the string variables.  And what
is your value of MAX_LFINFO?  I don't see how this can be efficiently
investigated without this data.  In particular, 'dot' has no storage
of its own, it's just a pointer into the lock_info.user[] array.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> This write in `lock_file':
>> 
>>       memcpy (dot, replacement, replacementlen);
>> 
>> Ends up writing one byte outside dot.
>> 
>> I could not find the problem, but someone else may want to take a look.

> Details, please: the values of all the relevant variables at that
> point, including the contents of all the string variables.

gdb says dot is "optimized out" (but valgrind, which I happened to be
running that Emacs under in an attempt to catch the GC bug I reported a
month ago, disagrees).  pidlen is 6.

I will try to get more information from the valgrind core file.

> And what is your value of MAX_LFINFO?

MAX_LFINFO is 8192.

> I don't see how this can be efficiently investigated without this
> data.  In particular, 'dot' has no storage of its own, it's just a
> pointer into the lock_info.user[] array.

Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53136@debbugs.gnu.org
> Date: Sun, 09 Jan 2022 16:10:42 +0800
> 
> >>       memcpy (dot, replacement, replacementlen);
> >> 
> >> Ends up writing one byte outside dot.
> >> 
> >> I could not find the problem, but someone else may want to take a look.
> 
> > Details, please: the values of all the relevant variables at that
> > point, including the contents of all the string variables.
> 
> gdb says dot is "optimized out" (but valgrind, which I happened to be
> running that Emacs under in an attempt to catch the GC bug I reported a
> month ago, disagrees).  pidlen is 6.
> 
> I will try to get more information from the valgrind core file.

Thanks.

> > And what is your value of MAX_LFINFO?
> 
> MAX_LFINFO is 8192.

So we somehow wrote more than 8192 bytes by that memcpy line?  I find
this hard to believe.

What exactly did valgrind say?

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> gdb says dot is "optimized out" (but valgrind, which I happened to be
>> running that Emacs under in an attempt to catch the GC bug I reported a
>> month ago, disagrees).  pidlen is 6.
>> 
>> I will try to get more information from the valgrind core file.

A superficial examination didn't reveal any string that made sense in
the (stack) frame of lock_file.  I will dig deeper later.

> So we somehow wrote more than 8192 bytes by that memcpy line?  I find
> this hard to believe.

Same here, I couldn't find how that happened.

> What exactly did valgrind say?

It complained about an invalid write of size 1 at one byte outside the
current stack frame.

Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> So we somehow wrote more than 8192 bytes by that memcpy line?  I find
> this hard to believe.

Manual inspection of the core dump seems to reveal something very
different from what the debugger said (and I asked for a second opinion
on this as well):

 - lock_info.dot and lock_info.colon are NULL.
 - lock_filename is a Lisp string, the data is
   "/home/oldosfan/Mail/archive/sent/2022-01".
 - handler, subject_buf are NULL
 - dot is NULL
 - pidlen is -1 (long int)
 - replacementlen is 6

Any ideas?  Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53136@debbugs.gnu.org
> Date: Sun, 09 Jan 2022 19:43:47 +0800
> 
>  - lock_info.dot and lock_info.colon are NULL.
>  - lock_filename is a Lisp string, the data is
>    "/home/oldosfan/Mail/archive/sent/2022-01".
>  - handler, subject_buf are NULL
>  - dot is NULL

Maybe that's the problem: it means current_lock_owner returned early
due to some error, and we are dereferencing a NULL pointer.

Paul, could you please take a look?

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53136@debbugs.gnu.org
> Date: Sun, 09 Jan 2022 19:43:47 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So we somehow wrote more than 8192 bytes by that memcpy line?  I find
> > this hard to believe.
> 
> Manual inspection of the core dump seems to reveal something very
> different from what the debugger said (and I asked for a second opinion
> on this as well):
> 
>  - lock_info.dot and lock_info.colon are NULL.
>  - lock_filename is a Lisp string, the data is
>    "/home/oldosfan/Mail/archive/sent/2022-01".
>  - handler, subject_buf are NULL
>  - dot is NULL
>  - pidlen is -1 (long int)
>  - replacementlen is 6

On second thought, these values are strange.  Is PC really at the
memcpy line?  I don't see how dot could be NULL at that point: if
lock_if_free returns a negative value, lock_info.dot cannot be NULL,
according to my reading of the code.

What is the contents of lock_info.user upto the first null byte?

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> What is the contents of lock_info.user upto the first null byte?

It's "oldosfan" (which is correct).

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53136@debbugs.gnu.org
> Date: Sun, 09 Jan 2022 21:00:53 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What is the contents of lock_info.user upto the first null byte?
> 
> It's "oldosfan" (which is correct).

No, it's not: it should be of the form USER@HOST.PID[:BOOT-TIME].  See
the commentary around line 288 in filelock.c.  The string is generated
in lock_file_1.  maybe you can step into it and see what happens
there.

What is "oldosfan"? is it the user or the host name?

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> No, it's not: it should be of the form USER@HOST.PID[:BOOT-TIME].  See
> the commentary around line 288 in filelock.c.  The string is generated
> in lock_file_1.  maybe you can step into it and see what happens
> there.

Thanks, I'll look into it.

> What is "oldosfan"? is it the user or the host name?

That is my username.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Po Lu <luangruo@yahoo.com> writes:

> Thanks, I'll look into it.

Perhaps the problem is in valgrind: when running under Valgrind, trying
to lock that file makes lock_if_free return -2147459069.

That shouldn't happen, right?  (BTW, while investigating that value, I
noticed a portability problem to Haiku, which I'll fix on the master
branch.)

Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 53136@debbugs.gnu.org
> Date: Sun, 09 Jan 2022 21:23:14 +0800
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > Thanks, I'll look into it.
> 
> Perhaps the problem is in valgrind: when running under Valgrind, trying
> to lock that file makes lock_if_free return -2147459069.
> 
> That shouldn't happen, right?

It's a strange value, no matter what.

bug#53136: 28.0.90; segfault in lock_file

[Andreas Schwab]

On Jan 09 2022, Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

> That shouldn't happen, right?  (BTW, while investigating that value, I
> noticed a portability problem to Haiku, which I'll fix on the master
> branch.)

This means Haiku is not POSIX compliant, since error numbers must be
positive.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andreas Schwab <schwab@linux-m68k.org> writes:

> This means Haiku is not POSIX compliant, since error numbers must be
> positive.

This bug isn't about Haiku, but yes, it's not entirely POSIX compliant.

bug#53136: 28.0.90; segfault in lock_file

[Paul Eggert]

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

On 1/9/22 04:03, Eli Zaretskii wrote:

> Paul, could you please take a look?

The problem (as Po deduced) seems to be that Haiku errno values are 
negative by default.

However, I see some problems with the recently installed fix.

1. It uses BE_USE_POSITIVE_POSIX_ERRORS but Haiku spells it 
B_USE_POSITIVE_POSIX_ERRORS.

2. There's no need to replace "if (err < 0)" with "if (err == -1 || err 
== -2)", as this replacement is not needed given the "#if !defined HAIKU 
..." stuff.

3. More importantly, I'm sure there are other places where Emacs assumes 
that errno values are positive. I doubt whether it's reasonable to 
expect Emacs developers to remember and work around this Haiku 
incompatibility, every time they call a function that sets errno or 
returns an errno value. Instead, we should arrange for Haiku builds to 
use positive errno values, they way errno behaves on GNU and other 
POSIX-compatible hosts. This should be a much more maintainable solution.

Proposed patch attached.

[-- Attachment #2: 0001-Improve-port-to-Haiku-errno-values.patch --]
[-- Type: text/x-patch, Size: 1746 bytes --]

From 026aef868ad242c3ccaf0d542ab81b621a26e0a9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 10 Jan 2022 15:07:51 -0800
Subject: [PATCH] Improve port to Haiku errno values

* configure.ac (B_USE_POSITIVE_POSIX_ERRORS): Define on Haiku.
* src/filelock.c (lock_if_free): Revert recent changes, since
defining B_USE_POSITIVE_POSIX_ERRORS should fix the general problem.
---
 configure.ac   | 2 ++
 src/filelock.c | 9 +--------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index baf8c8018e..c5d97890c7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5672,6 +5672,8 @@ AC_DEFUN
 
   haiku)
     AC_DEFINE(HAIKU, [], [Define if the system is Haiku.])
+    AC_DEFINE([B_USE_POSITIVE_POSIX_ERRORS], [],
+      [Define to cause Haiku to use POSIX-compatible errno values.])
     ;;
 esac
 
diff --git a/src/filelock.c b/src/filelock.c
index c3927f58fa..a213c2b3ca 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -608,7 +608,7 @@ lock_if_free (lock_info_type *clasher, char *lfname)
       err = current_lock_owner (clasher, lfname);
       if (err != 0)
 	{
-	  if (err == -1 || err == -2)
+	  if (err < 0)
 	    return -2 - err; /* We locked it, or someone else has it.  */
 	  break; /* current_lock_owner returned strange error.  */
 	}
@@ -616,14 +616,7 @@ lock_if_free (lock_info_type *clasher, char *lfname)
       /* We deleted a stale lock; try again to lock the file.  */
     }
 
-#if !defined HAIKU \
-  || defined BE_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
-- 
2.32.0

bug#53136: 28.0.90; segfault in lock_file

[Paul Eggert]

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

One little thing I noticed in the Haiku port. I assume the lisp.h 
complication for pD was needed on 32-bit Haiku to avoid compiler 
warnings when passing long vs int (same size, either would work, but 
compiler warning). I installed the attached to simplify the ifdefs here. 
If my diagnosis of the issue is wrong and this patch doesn't work for 
Haiku, please let me know or feel free to revert and we can find a 
better way.

At some point we can get rid of pD by replacing pD with "t" everywhere 
in Emacs, but one step at a time.

[-- Attachment #2: 0001-Assume-C99-when-defining-pD.patch --]
[-- Type: text/x-patch, Size: 1517 bytes --]

From c4cadadea6f03308eb2c25c757431c19b9108e58 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 10 Jan 2022 15:21:08 -0800
Subject: [PATCH] Assume C99 when defining pD

* src/lisp.h (pD): Assume C99 or later, to simplify the ifdefs.
---
 src/lisp.h | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index f8fe2a6906..c5e63110c7 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -137,25 +137,9 @@ verify (BITS_WORD_MAX >> (BITS_PER_BITS_WORD - 1) == 1);
 /* Use pD to format ptrdiff_t values, which suffice for indexes into
    buffers and strings.  Emacs never allocates objects larger than
    PTRDIFF_MAX bytes, as they cause problems with pointer subtraction.
-   In C99, pD can always be "t"; configure it here for the sake of
-   pre-C99 libraries such as glibc 2.0 and Solaris 8.
-
-   On Haiku, the size of ptrdiff_t is inconsistent with the value of
-   PTRDIFF_MAX.  In that case, "t" should be sufficient. */
-
-#ifndef HAIKU
-#if PTRDIFF_MAX == INT_MAX
-# define pD ""
-#elif PTRDIFF_MAX == LONG_MAX
-# define pD "l"
-#elif PTRDIFF_MAX == LLONG_MAX
-# define pD "ll"
-#else
-# define pD "t"
-#endif
-#else
-# define pD "t"
-#endif
+   In C99, pD can always be "t", as we no longer need to worry about
+   pre-C99 libraries such as glibc 2.0 (1997) and Solaris 8 (2000).  */
+#define pD "t"
 
 /* Convenience macro for rarely-used functions that do not return.  */
 #define AVOID _Noreturn ATTRIBUTE_COLD void
-- 
2.32.0

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 1/9/22 04:03, Eli Zaretskii wrote:
>
>> Paul, could you please take a look?
>
> The problem (as Po deduced) seems to be that Haiku errno values are
> negative by default.

That was an unrelated problem.  This bug is on GNU/Linux, when running
under Valgrind.  (I was trying to find the GC bug from last month.)

> 1. It uses BE_USE_POSITIVE_POSIX_ERRORS but Haiku spells it
> B_USE_POSITIVE_POSIX_ERRORS.

I'll fix that.

> 2. There's no need to replace "if (err < 0)" with "if (err == -1 ||
> err == -2)", as this replacement is not needed given the "#if !defined
> HAIKU ..." stuff.

> 3. More importantly, I'm sure there are other places where Emacs
> assumes that errno values are positive. I doubt whether it's
> reasonable to expect Emacs developers to remember and work around this
> Haiku incompatibility, every time they call a function that sets errno
> or returns an errno value. Instead, we should arrange for Haiku builds
> to use positive errno values, they way errno behaves on GNU and other
> POSIX-compatible hosts. This should be a much more maintainable
> solution.

I would prefer for us to find and fix those instances manually, because
B_USE_POSITIVE_POSIX_ERRORS is some kind of stopgap measure for porting
POSIX utilities like sshfs, and prevents some other functionality from
working correctly (i.e. apparently it's an error to -ltracker with it
defined, so file dialogs won't work.)

Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Paul Eggert <eggert@cs.ucla.edu> writes:

> At some point we can get rid of pD by replacing pD with "t" everywhere
> in Emacs, but one step at a time.

It works here, thanks.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Paul Eggert <eggert@cs.ucla.edu> writes:

> +    AC_DEFINE([B_USE_POSITIVE_POSIX_ERRORS], [],
> +      [Define to cause Haiku to use POSIX-compatible errno values.])

Also, for this to have a chance of working in purely tty Haiku builds,
you have to give the linker `-lposix_error_mapper'.

Maybe doing that in tty builds would be a good idea.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Po Lu <luangruo@yahoo.com> writes:

> That was an unrelated problem.  This bug is on GNU/Linux, when running
> under Valgrind.  (I was trying to find the GC bug from last month.)

It happens with valgrind-3.18.1-7.fc35, but not valgrind-3.17.0-13.fc35.

Paul, any idea why this may be?  Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> Date: Mon, 10 Jan 2022 15:11:04 -0800
> Cc: 53136@debbugs.gnu.org, Po Lu <luangruo@yahoo.com>
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> 3. More importantly, I'm sure there are other places where Emacs assumes 
> that errno values are positive. I doubt whether it's reasonable to 
> expect Emacs developers to remember and work around this Haiku 
> incompatibility, every time they call a function that sets errno or 
> returns an errno value. Instead, we should arrange for Haiku builds to 
> use positive errno values, they way errno behaves on GNU and other 
> POSIX-compatible hosts. This should be a much more maintainable solution.

I agree.

bug#53136: 28.0.90; segfault in lock_file

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  53136@debbugs.gnu.org
> Date: Tue, 11 Jan 2022 08:51:25 +0800
> 
> > 3. More importantly, I'm sure there are other places where Emacs
> > assumes that errno values are positive. I doubt whether it's
> > reasonable to expect Emacs developers to remember and work around this
> > Haiku incompatibility, every time they call a function that sets errno
> > or returns an errno value. Instead, we should arrange for Haiku builds
> > to use positive errno values, they way errno behaves on GNU and other
> > POSIX-compatible hosts. This should be a much more maintainable
> > solution.
> 
> I would prefer for us to find and fix those instances manually, because
> B_USE_POSITIVE_POSIX_ERRORS is some kind of stopgap measure for porting
> POSIX utilities like sshfs, and prevents some other functionality from
> working correctly (i.e. apparently it's an error to -ltracker with it
> defined, so file dialogs won't work.)

Ugh!

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.

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

 [Sorry for the webmail, but Yahoo SMTP is wedged; I hope Reply All
worked and this reaches the bug tracker]

> Ugh!

I agree completely.

> 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.
  

[-- Attachment #2: Type: text/html, Size: 666 bytes --]

bug#53136: 28.0.90; segfault in lock_file

[Paul Eggert]

[-- 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

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Paul Eggert <eggert@cs.ucla.edu> writes:

> 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?

I tried yesterday and got many mysterious assertation failures in the
system library code, but it won't hurt to take another crack at that.

Thanks.

bug#53136: 28.0.90; segfault in lock_file

[Paul Eggert]

On 1/10/22 17:05, Po Lu wrote:
> Po Lu <luangruo@yahoo.com> writes:
> 
>> That was an unrelated problem.  This bug is on GNU/Linux, when running
>> under Valgrind.  (I was trying to find the GC bug from last month.)
> 
> It happens with valgrind-3.18.1-7.fc35, but not valgrind-3.17.0-13.fc35.
> 
> Paul, any idea why this may be?  Thanks.

Looking at the 3.18.1 release notes:

https://valgrind.org/docs/manual/dist.news.html

what jumps out at me are these entries:

  * glibc 2.34, which moved various functions from libpthread.so into
    libc.so, is now supported.
  407589  [Linux] Add support for C11 aligned_alloc() and GNU reallocarray()
  442061  very slow execution under Fedora 34 (readdwarf3)

It could be that these changes caused a bug in Valgrind. I think Emacs 
is using aligned_alloc and reallocarray.

I tried building Emacs master (savannah) on both Ubuntu 21.10 x86-64, 
which has valgrind-3.17.0, and on Fedora 35 x86-64, which has 
valgrind-3.18.1, and both instances worked for me. I was running the 
Fedora 35 instance on an AMD Phenom II X4 910e.

I merely stole a lock using an interactive Emacs as my test, though. 
What test are you using to reproduce the bug?

bug#53136: 28.0.90; segfault in lock_file

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 1/10/22 17:05, Po Lu wrote:
>> Po Lu <luangruo@yahoo.com> writes:
>> 
>>> That was an unrelated problem.  This bug is on GNU/Linux, when running
>>> under Valgrind.  (I was trying to find the GC bug from last month.)
>> It happens with valgrind-3.18.1-7.fc35, but not
>> valgrind-3.17.0-13.fc35.
>> Paul, any idea why this may be?  Thanks.
>
> Looking at the 3.18.1 release notes:
>
> https://valgrind.org/docs/manual/dist.news.html
>
> what jumps out at me are these entries:
>
>  * glibc 2.34, which moved various functions from libpthread.so into
>    libc.so, is now supported.
>  407589  [Linux] Add support for C11 aligned_alloc() and GNU reallocarray()
>  442061  very slow execution under Fedora 34 (readdwarf3)
>
> It could be that these changes caused a bug in Valgrind. I think Emacs
> is using aligned_alloc and reallocarray.
>
> I tried building Emacs master (savannah) on both Ubuntu 21.10 x86-64,
> which has valgrind-3.17.0, and on Fedora 35 x86-64, which has
> valgrind-3.18.1, and both instances worked for me. I was running the
> Fedora 35 instance on an AMD Phenom II X4 910e.

> I merely stole a lock using an interactive Emacs as my test,
> though. What test are you using to reproduce the bug?

(lock-file "~/any/nonexistent/file/with/a/very/long/path") will
consistently reproduce the bug.

bug#53136: 28.0.90; segfault in lock_file

[Paul Eggert]

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

On 1/11/22 19:04, Po Lu wrote:
> (lock-file "~/any/nonexistent/file/with/a/very/long/path")

Unfortunately I'm still not reproducing the problem on Fedora 35 x86-64. 
Here's how I tried to reproduce it, while in the src directory:

  valgrind --suppressions=valgrind.supp ./emacs -Q -batch -eval '
    (let ((f "/d"))
      (dotimes (i 13) (setq f (concat f f)))
      (lock-file f))
  '

I configured Emacs this way:

./configure --enable-checking

(in general this is needed if you want to use Valgrind), and used the 
attached valgrind.supp.

[-- Attachment #2: valgrind.supp --]
[-- Type: text/plain, Size: 1669 bytes --]

# valgrind suppression file
# Usage:
#    valgrind --suppressions=valgrind.supp ./temacs

# Conservative garbage collection inherently looks at uninitialized values,
# and garbage_collect and its callees all depend on this.
# It's hard to separate out exactly which callees need to be listed here,
# since the C compiler can inline them.  Also, valgrind doesn't care
# about the use of uninitialized variables directly, only when their values
# are eventually used.  So just list garbage_collect and its callees.
{
   garbage_collect Cond - conservative garbage collection
   Memcheck:Cond
   ...
   fun:garbage_collect
}
{
   garbage_collect Value8 - conservative garbage collection
   Memcheck:Value8
   ...
   fun:garbage_collect
}
# valgrind only looks at the last few callees on the stack, but
# mark_object can call itself recursively and deeply.  So list
# it too, in case garbage_collect is a long way from the stack top.
{
   mark_object Cond - conservative garbage collection
   Memcheck:Cond
   ...
   fun:mark_object
}
{
   mark_object Value8 - conservative garbage collection
   Memcheck:Value8
   ...
   fun:mark_object
}
# valgrind gets confused about mark_stack and loses its caller
{
   mark_stack Value8 - conservative garbage collection
   Memcheck:Value8
   ...
   fun:mark_stack
}

# On a circa-2017 x86-64 GNU/Linux platform, memmove falsely warns
# about overlapping moves.
{
   memmove false alarm about overlap
   Memcheck:Overlap
   ...
   fun:memmove
}

# On one circa-2011 x86-64 GNU/Linux platform, strlen is inlined to
# something that loads 4 bytes at a time.
#{
#   init_buffer optimized strlen
#   Memcheck:Addr4
#   fun:init_buffer
#}