Subject: bug#13807: closed (Re: bug#13807: updated version to avoid MS-Windows vs non-MS-Windows clashes)

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 05 Mar 2013 22:40:02 +0000

Your bug report

#13807: The lock for 'DIR/FILE' should always be 'DIR/.#FILE'.

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
13807: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13807
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#13807: updated version to avoid MS-Windows vs non-MS-Windows clashes
From: Paul Eggert <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Tue, 05 Mar 2013 14:38:44 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

On 03/05/13 10:38, Eli Zaretskii wrote:
> We will need to define away O_NOFOLLOW and ELOOP, to get this to
> compile on Windows.  I think the right place for the former is
> nt/inc/unistd.h, near O_NOCTTY, and for the latter nt/inc/ms-w32.h,
> where ENOTSUP is defined.

Thanks, I did the former, but for the latter it's possible ELOOP
won't be defined on a (very old) POSIXish host, so I put a conditional
definition for it into filelock.c itself, which should work for
MS-Windows as well.  Installed as trunk bzr 111948 and marking
this as done.

Subject: The lock for 'DIR/FILE' should always be 'DIR/.#FILE'.
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Sun, 24 Feb 2013 14:48:53 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Tags: patch

Attached is a proposed cleanup patch, following up on the thread in
<http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00531.html>.
=== modified file 'etc/ChangeLog'
--- etc/ChangeLog	2013-02-19 14:44:03 +0000
+++ etc/ChangeLog	2013-02-24 22:44:04 +0000
@@ -1,3 +1,7 @@
+2013-02-24  Paul Eggert  <[email-masked]>
+
+	* NEWS: The lock for 'DIR/FILE' is now always 'DIR/.#FILE'.
+
 2013-02-14  Michael Albinus  <[email-masked]>
 
 	* NEWS: Tramp methods "scpc" and "rsyncc" are discontinued.

=== modified file 'etc/NEWS'
--- etc/NEWS	2013-02-21 06:55:19 +0000
+++ etc/NEWS	2013-02-24 22:44:04 +0000
@@ -316,6 +316,19 @@
 ** The `defalias-fset-function' property lets you catch calls to defalias
 and redirect them to your own function instead of `fset'.
 
+** The lock for 'DIR/FILE' is now always 'DIR/.#FILE'.
+When you edit DIR/FILE, Emacs normally creates a symbolic link
+DIR/.#FILE as a lock that warns other instances of Emacs that DIR/FILE
+is being edited.  Formerly, if there was already a non-symlink file
+named DIR/.#FILE, Emacs fell back on the lock names DIR/.#FILE.0
+through DIR/.#FILE.9.  This approach had race conditions that could
+not be easily fixed.  File names beginning with '.#' are unusual
+enough that the fallbacks were more trouble than they were worth, so
+if DIR/.#FILE is a non-symlink file, Emacs now does not create a lock
+file for DIR/FILE; that is, DIR/FILE is no longer partly protected by
+a lock if DIR/.#FILE is a non-symlink file ("partly" because the
+locking mechanism was never reliable in that case).
+
 ** The 9th element returned by `file-attributes' is now unspecified.
 Formerly, it was t if the file's gid would change if file were deleted
 and recreated.  This value has been inaccurate for years on many

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2013-02-24 19:45:17 +0000
+++ src/ChangeLog	2013-02-24 22:44:04 +0000
@@ -1,3 +1,16 @@
+2013-02-24  Paul Eggert  <[email-masked]>
+
+	The lock for 'DIR/FILE' is now always 'DIR/.#FILE'.
+	* filelock.c (MAKE_LOCK_NAME, fill_in_lock_file_name):
+	Don't create DIR/.#FILE.0 through DIR/.#FILE.9, as this was
+	more trouble than it was worth and led to race conditions of its own.
+	(MAKE_LOCK_NAME, unlock_file, Ffile_locked_p):
+	Use SAFE_ALLOCA to avoid problems with long file names.
+	(lock_file_1): Don't inspect errno if symlink call succeeds;
+	that's not portable.
+	(lock_file): Document that this functionn can return if
+	lock creation fails.
+
 2013-02-24  Andreas Schwab  <[email-masked]>
 
 	* xdisp.c (set_message): Only check for debug-on-message if STRING

=== modified file 'src/filelock.c'
--- src/filelock.c	2013-02-24 19:45:17 +0000
+++ src/filelock.c	2013-02-24 22:44:04 +0000
@@ -289,44 +289,22 @@

 /* 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
-   null.  */
+   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), \
-   fill_in_lock_file_name (lock, (file)))
+  (lock = SAFE_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)
+fill_in_lock_file_name (char *lockfile, Lisp_Object fn)
 {
-  ptrdiff_t length = SBYTES (fn);
-  register char *p;
-  struct stat st;
-  int count = 0;
-
-  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[2] = *p;
-
-  /* Insert the `.#'.  */
-  p[1] = '.';
-  p[2] = '#';
-
-  p = lockfile + length + 2;
-
-  while (lstat (lockfile, &st) == 0 && !S_ISLNK (st.st_mode))
-    {
-      if (count > 9)
-	{
-	  *p = '\0';
-	  return;
-	}
-      sprintf (p, ".%d", count++);
-    }
+  char *last_slash = memrchr (SSDATA (fn), '/', SBYTES (fn));
+  char *base = last_slash + 1;
+  ptrdiff_t dirlen = base - SSDATA (fn);
+
+  memcpy (lockfile, SSDATA (fn), dirlen);
+  lockfile[dirlen] = '.';
+  lockfile[dirlen + 1] = '#';
+  strcpy (lockfile + dirlen + 2, base);
 }
 
 /* Lock the lock file named LFNAME.
@@ -357,7 +335,7 @@
 	    user_name, host_name, pid, boot);
 
   err = symlink (lock_info_str, lfname);
-  if (errno == EEXIST && force)
+  if (err != 0 && errno == EEXIST && force)
     {
       unlink (lfname);
       err = symlink (lock_info_str, lfname);
@@ -520,6 +498,7 @@
    decided to go ahead without locking.
 
    When this returns, either the lock is locked for us,
+   or lock creation failed,
    or the user has said to go ahead without locking.
 
    If the file is locked by someone else, this calls
@@ -599,9 +578,10 @@
 }
 
 void
-unlock_file (register Lisp_Object fn)
+unlock_file (Lisp_Object fn)
 {
-  register char *lfname;
+  char *lfname;
+  USE_SAFE_ALLOCA;
 
   fn = Fexpand_file_name (fn, Qnil);
   fn = ENCODE_FILE (fn);
@@ -610,6 +590,8 @@
 
   if (current_lock_owner (0, lfname) == 2)
     unlink (lfname);
+
+  SAFE_FREE ();
 }
 
 void
@@ -675,9 +657,10 @@
   (Lisp_Object filename)
 {
   Lisp_Object ret;
-  register char *lfname;
+  char *lfname;
   int owner;
   lock_info_type locker;
+  USE_SAFE_ALLOCA;
 
   filename = Fexpand_file_name (filename, Qnil);
 
@@ -694,6 +677,7 @@
   if (owner > 0)
     FREE_LOCK_INFO (locker);
 
+  SAFE_FREE ();
   return ret;
 }