bug#71477: 30.0.50; Lock files are not deleted on Windows 98

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

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

With lock files enabled, type C-x C-f C:/WINDOWS/Application Data/.emacs
RET, modify the file, and type C-x C-f, whereupon such a warning will be
displayed:

Warning (unlock-file): Invalid argument, `~/.emacs', ignored

I don't recall whether it was because lock files were disabled on the
same machine that this issue wasn't present in Emacs 28.1, or it was
because the issue was introduced in a subsequent release.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> Date: Mon, 10 Jun 2024 23:07:38 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> With lock files enabled, type C-x C-f C:/WINDOWS/Application Data/.emacs
> RET, modify the file, and type C-x C-f, whereupon such a warning will be
> displayed:
> 
> Warning (unlock-file): Invalid argument, `~/.emacs', ignored

I don't have access to Windows 9X anymore, and there's no such
directory on the Windows system to which I do have access.  So either
you or someone will show a recipe that can be reproduced and debugged
on a more modern system, or you dig into the EINVAL on that system and
tell more details to understand what happens, or we just dismiss this
bug alone with "moreinfo" tag (after all, this is just a warning).

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 71477@debbugs.gnu.org
> Date: Tue, 11 Jun 2024 09:41:54 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't have access to Windows 9X anymore, and there's no such
> > directory on the Windows system to which I do have access.  So either
> > you or someone will show a recipe that can be reproduced and debugged
> > on a more modern system, or you dig into the EINVAL on that system and
> > tell more details to understand what happens, or we just dismiss this
> > bug alone with "moreinfo" tag (after all, this is just a warning).
> 
> I think I've arrived at the problem: Femacs_pid and getpid return a
> negative value, and once it is duly written to the lock file,
> current_lock_owner does not accept the sign character after the PID
> separator, consequently returning EINVAL.  Apparently, in times past
> file-locking routines cast all PIDs to unsigned long, but that behavior
> was lost in the midst of changes since installed for Unix systems, and
> as such I will attempt to restore the historical semantics on Windows
> systems.

Sorry, I don't understand the problems with negative PID values.
Where exactly in the code of filelock.c it gets in the way?

We had a similar problem on Cygwin, albeit with boot time, not PID,
and we fixed it very easily.  If filelock.c assumes PIDs are positive
somewhere, please point out that code.  In any case, please don't
install any changes in this area without posting them for discussion
first.

Thanks.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 71477@debbugs.gnu.org
> Date: Tue, 11 Jun 2024 15:42:58 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sorry, I don't understand the problems with negative PID values.
> > Where exactly in the code of filelock.c it gets in the way?
> 
> Here:
> 
>   /* The PID is everything from the last '.' to the ':' or equivalent.  */
>   if (! c_isdigit (dot[1])) <--------------
>     return EINVAL;
>   errno = 0;
> 
> The first character of the number after the period is `-' on Windows 98.

But that is easy to fix without any significant effect on the rest of
the code.  For example:

  if (! (c_isdigit (dot[1])
         || (dot[1] == '-'  && c_isdigit (dot[2]))))
    return EINVAL;

Are there any problems with the above fix?

Please note: I don't want to make any significant changes in this
area, certainly not for the benefit of Windows 9X.  So if the above is
not sufficient, please tell the details, and let's discuss how to
solve what's left.

P.S. I've for now reverted the changes you made to use unsigned values
because I don't think that TRT (pid must support negative values), and
this whole area of code is fragile enough for us to discuss changes
before installing them.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 71477@debbugs.gnu.org
> Date: Tue, 11 Jun 2024 16:15:08 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: 71477@debbugs.gnu.org
> >> Date: Tue, 11 Jun 2024 15:42:58 +0800
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > Sorry, I don't understand the problems with negative PID values.
> >> > Where exactly in the code of filelock.c it gets in the way?
> >> 
> >> Here:
> >> 
> >>   /* The PID is everything from the last '.' to the ':' or equivalent.  */
> >>   if (! c_isdigit (dot[1])) <--------------
> >>     return EINVAL;
> >>   errno = 0;
> >> 
> >> The first character of the number after the period is `-' on Windows 98.
> >
> > But that is easy to fix without any significant effect on the rest of
> > the code.  For example:
> >
> >   if (! (c_isdigit (dot[1])
> >          || (dot[1] == '-'  && c_isdigit (dot[2]))))
> >     return EINVAL;
> >
> > Are there any problems with the above fix?
> 
> No, but won't leaving the format of the lock file string inconsistent
> with Unix create difficulties elsewhere, as, for example, on a Samba
> share to which Unix systems are also connected?

What inconsistencies, specifically?

The only possible issue I see with allowing a negative PID is that the
code checks for "pid > 0" or "pid < 0" somewhere; if that is the case,
we should replace those with comparisons with -1 instead.

Can you test the above on Windows 9X when you have a chance?  Then we
could install it.

> P.S. is debbugs.gnu.org offline or some such?

It has connectivity problems.  It was already reported.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 71477@debbugs.gnu.org
> Date: Tue, 11 Jun 2024 16:43:06 +0800
> 
> > The only possible issue I see with allowing a negative PID is that the
> > code checks for "pid > 0" or "pid < 0" somewhere; if that is the case,
> > we should replace those with comparisons with -1 instead.
> >
> > Can you test the above on Windows 9X when you have a chance?  Then we
> > could install it.
> 
> If it doesn't produce any adverse effect on modern Windows, and what I
> raised is not important, let's install it now, and I will test it as
> soon as may be, or it might fall by the wayside.

OK, but could you provide some additional details, so I could
understand the issue better?  What kind of negative values do you get
from getpid on Windows 98, and what does the system show as the PID of
that process?  Is the value really such a large positive number that
its MSB is set?

According to my records, _getpid just calls GetCurrentProcessId and
returns the value as an int.  So for _getpid to return a negative
value, GetCurrentProcessId should return a very large positive value,
I think.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

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

Eli Zaretskii <eliz@gnu.org> writes:

> OK, but could you provide some additional details, so I could
> understand the issue better?  What kind of negative values do you get
> from getpid on Windows 98, and what does the system show as the PID of
> that process?  Is the value really such a large positive number that
> its MSB is set?

The value of getpid was -1859765, but I did not attempt to read the PID
manually with GetCurrentProcessID or cross-check it against the OS's
equivalent of ps, if there exists one at all.

> According to my records, _getpid just calls GetCurrentProcessId and
> returns the value as an int.  So for _getpid to return a negative
> value, GetCurrentProcessId should return a very large positive value,
> I think.

There's a screenshot on this forum that, if it is to be trusted,
demonstrates that PIDs are indeed of this scale on Windows 9X:

  https://www.vbforums.com/showthread.php?308830-Task-Manager-For-Windows-98

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 71477@debbugs.gnu.org
> Date: Tue, 11 Jun 2024 16:43:06 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What inconsistencies, specifically?
> 
> Any older Emacs binary that encounters a lock file produced under
> Windows 9X will report an "Invalid argument" error until the user
> intervenes to delete this lock file.

I don't see how we can fix such problems retroactively.

> > The only possible issue I see with allowing a negative PID is that the
> > code checks for "pid > 0" or "pid < 0" somewhere; if that is the case,
> > we should replace those with comparisons with -1 instead.
> >
> > Can you test the above on Windows 9X when you have a chance?  Then we
> > could install it.
> 
> If it doesn't produce any adverse effect on modern Windows, and what I
> raised is not important, let's install it now, and I will test it as
> soon as may be, or it might fall by the wayside.

OK.  (It turns out we already knew about this issue, see the comments
in w32proc.c, search for "Hack for Windows 95".)

Paul, do you see any problems with the change below?  It worked for me
in some limited testing.  I intend to install it on the master branch
unless there are objections.

diff --git a/src/filelock.c b/src/filelock.c
index 050cac5..59fb47e 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -393,7 +393,9 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
     return EINVAL;
 
   /* The PID is everything from the last '.' to the ':' or equivalent.  */
-  if (! c_isdigit (dot[1]))
+  if (! (c_isdigit (dot[1])
+	 /* Windows 9X report negative PID values.  */
+	 || (dot[1] == '-' && c_isdigit (dot[2]))))
     return EINVAL;
   errno = 0;
   pid = strtoimax (dot + 1, &owner->colon, 10);
@@ -451,7 +453,7 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
     {
       if (pid == getpid ())
         return I_OWN_IT;
-      else if (0 < pid && pid <= TYPE_MAXIMUM (pid_t)
+      else if (pid != -1 && pid <= TYPE_MAXIMUM (pid_t)
                && (kill (pid, 0) >= 0 || errno == EPERM)
 	       && (boot_time == 0
 		   || (boot_time <= TYPE_MAXIMUM (time_t)

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Paul Eggert]

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

On 2024-06-12 01:25, Eli Zaretskii wrote:

> -  if (! c_isdigit (dot[1]))
> +  if (! (c_isdigit (dot[1])
> +	 /* Windows 9X report negative PID values.  */
> +	 || (dot[1] == '-' && c_isdigit (dot[2]))))

Faster is "if (! c_isdigit[(dot[1] == '-') + 1])", as it avoids a 
conditional branch on most platforms.


> -      else if (0 < pid && pid <= TYPE_MAXIMUM (pid_t)
> +      else if (pid != -1 && pid <= TYPE_MAXIMUM (pid_t)
>                  && (kill (pid, 0) >= 0 || errno == EPERM)

This looks dubious for most systems, where 'kill' has special behavior 
when pid < -1 or pid == 0; it tests a process group. That's not the test 
we want here, since we want to check whether Emacs can be sent a signal, 
not whether any process in its process group can be sent a signal (this 
can be valid even after Emacs has exited). The code should use calls 
like kill (-2, 0) and kill (0, 0) only on platforms where we know the 
calls do not test a process group.

Even on MS Windows 98 we should check that TYPE_MINIMUM (pid_t) <= pid. 
Also, is there a special meaning for kill (0, 0) on MS Windows 98? If 
so, we should also check that pid != 0.

Do any MS-Windows platforms support process groups, i.e., kill (-2, 0) 
operates on process group 2 rather than on an individual process with 
process ID -2? If so, these platforms should be careful too, and should 
not use kill (-2, 0) or kill (0, 0).

How about the attached patch instead? You can adjust the 
Microsoft-specific .h files to define VALID_PROCESS_ID appropriately for 
MS Windows 98, and for any other MS platform where kill (-2, 0) is known 
to check for just the individual process -2.

[-- Attachment #2: 0001-Start-of-a-fix-for-bug-71477.patch --]
[-- Type: text/x-patch, Size: 2525 bytes --]

From f5dcecb833c716995d38f90abc6d120d6f5e3064 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 12 Jun 2024 08:42:24 -0700
Subject: [PATCH] Start of a fix for bug#71477

* src/filelock.c (integer_prefixed): New static function.
(VALID_PROCESS_ID): New macro.
(current_lock_owner): Use them to allow negative process IDs
on some Microsoft platforms.
---
 src/filelock.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/filelock.c b/src/filelock.c
index 050cac565c9..d4a4747955a 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -342,6 +342,22 @@ read_lock_data (char *lfname, char lfinfo[MAX_LFINFO + 1])
   return nbytes;
 }
 
+/* Whether the string S starts with a decimal integer, optionally
+   negative.  */
+static bool
+integer_prefixed (char const *s)
+{
+  /* Doing it this way avoids a conditional branch on most platforms.  */
+  return c_isdigit (s[s[0] == '-']);
+}
+
+/* Whether the integer P could identify an individual process.  On most
+   platforms this simply checks for positive pid_t, but on some
+   Microsoft ports our headers #define it to to some other test.  */
+#ifndef VALID_PROCESS_ID
+# define VALID_PROCESS_ID(p) (0 < (p) && (p) <= TYPE_MAXIMUM (pid_t))
+#endif
+
 /* 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 };
@@ -393,7 +409,7 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
     return EINVAL;
 
   /* The PID is everything from the last '.' to the ':' or equivalent.  */
-  if (! c_isdigit (dot[1]))
+  if (! integer_prefixed (dot + 1))
     return EINVAL;
   errno = 0;
   pid = strtoimax (dot + 1, &owner->colon, 10);
@@ -419,9 +435,7 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
       boot += 2;
       FALLTHROUGH;
     case ':':
-      if (!(c_isdigit (boot[0])
-	    /* A negative number.  */
-	    || (boot[0] == '-' && c_isdigit (boot[1]))))
+      if (! integer_prefixed (boot))
 	return EINVAL;
       boot_time = strtoimax (boot, &lfinfo_end, 10);
       break;
@@ -451,7 +465,7 @@ current_lock_owner (lock_info_type *owner, Lisp_Object lfname)
     {
       if (pid == getpid ())
         return I_OWN_IT;
-      else if (0 < pid && pid <= TYPE_MAXIMUM (pid_t)
+      else if (VALID_PROCESS_ID (pid)
                && (kill (pid, 0) >= 0 || errno == EPERM)
 	       && (boot_time == 0
 		   || (boot_time <= TYPE_MAXIMUM (time_t)
-- 
2.43.0

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> Date: Wed, 12 Jun 2024 09:07:52 -0700
> Cc: 71477@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> 
> On 2024-06-12 01:25, Eli Zaretskii wrote:
> 
> > -  if (! c_isdigit (dot[1]))
> > +  if (! (c_isdigit (dot[1])
> > +	 /* Windows 9X report negative PID values.  */
> > +	 || (dot[1] == '-' && c_isdigit (dot[2]))))
> 
> Faster is "if (! c_isdigit[(dot[1] == '-') + 1])", as it avoids a 
> conditional branch on most platforms.

OK.

> > -      else if (0 < pid && pid <= TYPE_MAXIMUM (pid_t)
> > +      else if (pid != -1 && pid <= TYPE_MAXIMUM (pid_t)
> >                  && (kill (pid, 0) >= 0 || errno == EPERM)
> 
> This looks dubious for most systems, where 'kill' has special behavior 
> when pid < -1 or pid == 0; it tests a process group. That's not the test 
> we want here, since we want to check whether Emacs can be sent a signal, 
> not whether any process in its process group can be sent a signal (this 
> can be valid even after Emacs has exited). The code should use calls 
> like kill (-2, 0) and kill (0, 0) only on platforms where we know the 
> calls do not test a process group.

But on all platforms except Windows 9X we shouldn't see a negative PID
here, so what you say is purely theoretical, no?

> Even on MS Windows 98 we should check that TYPE_MINIMUM (pid_t) <= pid. 

Since pid_t is typedefed as 'int', that's always true, no?

> Also, is there a special meaning for kill (0, 0) on MS Windows 98?

No.  And our emulation of 'kill' fails with EPERM when called witgh
both arguments zero.

> If so, we should also check that pid != 0.

There are no processes on Windows whose PID is zero, so getting zero
here is impossible.

> Do any MS-Windows platforms support process groups, i.e., kill (-2, 0) 
> operates on process group 2 rather than on an individual process with 
> process ID -2? If so, these platforms should be careful too, and should 
> not use kill (-2, 0) or kill (0, 0).

Windows does support process groups, but our emulation of 'kill'
pretends that each process is its own group.

> How about the attached patch instead? You can adjust the 
> Microsoft-specific .h files to define VALID_PROCESS_ID appropriately for 
> MS Windows 98, and for any other MS platform where kill (-2, 0) is known 
> to check for just the individual process -2.

Fine with me, please install and I will followup.

Thanks.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Paul Eggert]

On 2024-06-12 10:10, Eli Zaretskii wrote:

> But on all platforms except Windows 9X we shouldn't see a negative PID
> here, so what you say is purely theoretical, no?

No, because that value doesn't come from a pid_t that the system gave us 
as a process ID. It comes from the file system, and so could be invalid 
as a process ID. That's why the code already checks that pid <= 
TYPE_MAXIMUM (pid_t). Such a check wouldn't be needed if the pid were a 
valid process ID.


>> Even on MS Windows 98 we should check that TYPE_MINIMUM (pid_t) <= pid.
> 
> Since pid_t is typedefed as 'int', that's always true, no?

No, because the code is checking 'pid', which is of type intmax_t not 
pid_t. (And anyway pid_t need not be 'int'.)


> No.  And our emulation of 'kill' fails with EPERM when called witgh
> both arguments zero.

In that case there's no need to worry about pid == 0 here.


> Fine with me, please install and I will followup.

OK, installed.

bug#71477: 30.0.50; Lock files are not deleted on Windows 98

[Eli Zaretskii]

> Date: Wed, 12 Jun 2024 10:57:10 -0700
> Cc: luangruo@yahoo.com, 71477@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> > Fine with me, please install and I will followup.
> 
> OK, installed.

Thanks, I've now installed the followup bits.  Po Lu, please test if
the result works on Windows 9X when you have time.

I'm closing the bug for now; please reopen with new information if the
new code still doesn't work.