Bug in filelock.c

Bug in filelock.c

[Eli Zaretskii]

The marked line from filelock.c seems to have such a glaring bug that
I'm questioning my own judgment:

  /* Shift the nondirectory part of the file name (including the null)
     right two characters.  Here is one of the places where we'd have to
     do something to support 14-character-max file names.  */
  for (p = lockfile + length; p != lockfile && *p != '/'; p--)
    p[2] = *p;

  /* Insert the `.#'.  */
  p[1] = '.';
  p[2] = '#';

  p = p + length + 2;
  ^^^^^^^^^^^^^^^^^^

I think it should say this instead:

  p = lockfile + length + 2

I guess no one ever had a situation where a numeric tail was needed,
because then fill_in_lock_file_name would corrupt the stack.  Or maybe
on most machines nowadays arguments are not passed on the stack, even
in a non-optimized build.

Am I missing something?

Re: Bug in filelock.c

[Paul Eggert]

On 02/23/2013 11:07 AM, Eli Zaretskii wrote:
> Am I missing something?

No, it looks like a clear bug.  Thanks for catching it!

Also, that "p != lockfile" business is bogus;
p can never equal lockfile.

Since the code has obviously never worked and evidently
never been needed and slows Emacs down with unnecessary
syscalls, I propose that we remove it, as follows:

=== modified file 'src/filelock.c'
--- src/filelock.c	2013-02-01 06:30:51 +0000
+++ src/filelock.c	2013-02-23 19:37:31 +0000
@@ -289,44 +289,31 @@ typedef struct
 
 
 /* Write the name of the lock file for FN into LFNAME.  Length will be
-   that of FN plus two more for the leading `.#' plus 1 for the
-   trailing period plus one for the digit after it plus one for the
+   that of FN plus two more for the leading `.#' plus one for the
    null.  */
 #define MAKE_LOCK_NAME(lock, file) \
-  (lock = alloca (SBYTES (file) + 2 + 1 + 1 + 1), \
+  (lock = alloca (SBYTES (file) + 2 + 1), \
    fill_in_lock_file_name (lock, (file)))
 
 static void
 fill_in_lock_file_name (register char *lockfile, register Lisp_Object fn)
 {
   ptrdiff_t length = SBYTES (fn);
-  register char *p;
-  struct stat st;
-  int count = 0;
+  char *p;
 
   strcpy (lockfile, SSDATA (fn));
 
   /* Shift the nondirectory part of the file name (including the null)
      right two characters.  Here is one of the places where we'd have to
      do something to support 14-character-max file names.  */
-  for (p = lockfile + length; p != lockfile && *p != '/'; p--)
+  p = lockfile + length;
+  do
     p[2] = *p;
+  while (*--p != '/');
 
   /* Insert the `.#'.  */
   p[1] = '.';
   p[2] = '#';
-
-  p = p + length + 2;
-
-  while (lstat (lockfile, &st) == 0 && !S_ISLNK (st.st_mode))
-    {
-      if (count > 9)
-	{
-	  *p = '\0';
-	  return;
-	}
-      sprintf (p, ".%d", count++);
-    }
 }
 
 /* Lock the lock file named LFNAME.

Re: Bug in filelock.c

[Eli Zaretskii]

> Date: Sat, 23 Feb 2013 11:39:43 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> On 02/23/2013 11:07 AM, Eli Zaretskii wrote:
> > Am I missing something?
> 
> No, it looks like a clear bug.  Thanks for catching it!

It's actually Emacs who caught it, by crashing ;-) I just debugged it.

> Since the code has obviously never worked and evidently
> never been needed and slows Emacs down with unnecessary
> syscalls, I propose that we remove it, as follows:

If we don't care overwriting files like .#foo (and it looks like we
don't, since no one complained about that), then I agree.

Re: Bug in filelock.c

[Andreas Schwab]

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

> Since the code has obviously never worked

??? It worked perfectly well until about 8 months ago.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Bug in filelock.c

[Paul Eggert]

On 02/23/2013 01:59 PM, Andreas Schwab wrote:
> It worked perfectly well until about 8 months ago.

Thanks, I installed the following patch to the emacs-24 branch
and I assume this fix will propagate into the trunk.

I still don't see the need for that extra business with
using '.#FOO.1' as the lock file name if a file '.#FOO' happens
to exist and is not a symbolic link.  How about if we remove
that stuff (in the trunk)?  Isn't it a waste of a system call for
something nobody should need?

----

Fix regression introduced by July 10 filelock.c patch.
* filelock.c (fill_in_lock_file_name): Fix crash caused by the
2012-07-10 patch to this file.  Reported by Eli Zaretskii in
<http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00533.html>
and diagnosed by Andreas Schwab in
<http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00534.html>.
=== modified file 'src/filelock.c'
--- src/filelock.c	2013-01-01 09:11:05 +0000
+++ src/filelock.c	2013-02-24 00:16:45 +0000
@@ -316,7 +316,7 @@
   p[1] = '.';
   p[2] = '#';

-  p = p + length + 2;
+  p = lockfile + length + 2;

   while (lstat (lockfile, &st) == 0 && !S_ISLNK (st.st_mode))
     {

Re: Bug in filelock.c

[Eli Zaretskii]

> Date: Sat, 23 Feb 2013 16:25:16 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> On 02/23/2013 01:59 PM, Andreas Schwab wrote:
> > It worked perfectly well until about 8 months ago.
> 
> Thanks, I installed the following patch to the emacs-24 branch
> and I assume this fix will propagate into the trunk.

Thanks.  The "p != lockfile" test is still unneeded, though, right?
Should we make that change on the trunk?

Re: Bug in filelock.c

[Paul Eggert]

On 02/23/2013 07:46 PM, Eli Zaretskii wrote:
> The "p != lockfile" test is still unneeded, though, right?
> Should we make that change on the trunk?

Yes, but I'd like to get a handle on whether
the ".1" suffix is needed before fiddling with
that.