From: Paul Eggert <eggert@cs.ucla.edu>
To: Glenn Morris <rgm@gnu.org>
Cc: 13807@debbugs.gnu.org
Subject: bug#13807: The lock for 'DIR/FILE' should always be 'DIR/.#FILE'.
Date: Mon, 25 Feb 2013 15:40:11 -0800 [thread overview]
Message-ID: <512BF65B.5010500@cs.ucla.edu> (raw)
In-Reply-To: <2dip5g4154.fsf@fencepost.gnu.org>
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
On 02/25/13 11:57, Glenn Morris wrote:
>> <http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00531.html>.
>
> AFAICS, all that thread says is "we should remove this because it never
> worked", when in fact it did work just fine until recently.
Yes, the discussion should have been clearer. This patch was prompted
by a code inspection after fixing the bug mentioned in that thread; the patch
does not fix the bug (the bug's already fixed). I tried to clarify
this in the revised patch (attached).
> So the motivation for this seems to be entirely as given in the NEWS
> (which is not really where it belongs IMO)
OK, I moved the motivation out of NEWS and into the ChangeLog entry.
> No-one ever reported any (non-theoretical)
> problems with it in practice, AFAIK.
The problem is more likely to happen with today's changes to the
MS-Windows side. And even if the problem was less likely, it's still
a race condition that should get fixed -- the point of that lock
file is to avoid races, after all.
[-- Attachment #2: filelock.txt --]
[-- Type: text/plain, Size: 6410 bytes --]
=== modified file 'etc/ChangeLog'
--- etc/ChangeLog 2013-02-25 17:01:41 +0000
+++ etc/ChangeLog 2013-02-25 19:42:26 +0000
@@ -1,5 +1,7 @@
2013-02-25 Paul Eggert <eggert@cs.ucla.edu>
+ * NEWS: The lock for 'DIR/FILE' is now always 'DIR/.#FILE' (Bug#13807).
+
Simplify data_start configuration (Bug#13783).
* NEWS: Document removal of --with-crt-dir.
* PROBLEMS (LIBS_SYSTEM, LIBS_MACHINE, LIBS_STANDARD): Remove.
=== modified file 'etc/NEWS'
--- etc/NEWS 2013-02-25 17:36:03 +0000
+++ etc/NEWS 2013-02-25 23:00:06 +0000
@@ -319,6 +319,14 @@
** 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. These fallbacks have been removed, so that
+Emacs now no longer locks DIR/FILE 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-25 17:36:03 +0000
+++ src/ChangeLog 2013-02-25 23:33:21 +0000
@@ -1,3 +1,26 @@
+2013-02-25 Paul Eggert <eggert@cs.ucla.edu>
+
+ The lock for 'DIR/FILE' is now always 'DIR/.#FILE' (Bug#13807).
+ The old approach, which fell back on DIR/.#FILE.0 through
+ DIR/.#FILE.9, had race conditions that could not be easily fixed.
+ 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).
+ This patch fixes this and other bugs discovered by a code
+ inspection that was prompted by
+ <http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00531.html>.
+ * filelock.c (MAKE_LOCK_NAME, fill_in_lock_file_name):
+ Don't create DIR/.#FILE.0 through DIR/.#FILE.9.
+ (MAKE_LOCK_NAME, unlock_file, Ffile_locked_p):
+ Use SAFE_ALLOCA to avoid problems with long file names.
+ (MAX_LFINFO): Now a local constant, not a global macro.
+ (IS_LOCK_FILE): Remove.
+ (lock_file_1): Don't inspect errno if symlink call succeeds;
+ that's not portable.
+ (lock_file): Document that this function can return if lock
+ creation fails.
+
2013-02-25 Eli Zaretskii <eliz@gnu.org>
Implement CLASH_DETECTION for MS-Windows.
=== modified file 'src/filelock.c'
--- src/filelock.c 2013-02-25 17:36:03 +0000
+++ src/filelock.c 2013-02-25 23:35:55 +0000
@@ -292,53 +292,22 @@
/* Write the name of the lock file for FNAME into LOCKNAME. 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
+ will be that of FNAME plus two more for the leading `.#' plus one for
the null. */
-#define MAKE_LOCK_NAME(LOCKNAME, FNAME) \
- (LOCKNAME = alloca (SBYTES (FNAME) + 2 + 1 + 1 + 1), \
- fill_in_lock_file_name (LOCKNAME, (FNAME)))
-
-#ifdef WINDOWSNT
-/* 256 chars for user, 1024 chars for host, 10 digits for each of 2 int's. */
-#define MAX_LFINFO (256 + 1024 + 10 + 10 + 2)
- /* min size: .@PID */
-#define IS_LOCK_FILE(ST) (MAX_LFINFO >= (ST).st_size && (ST).st_size >= 3)
-#else
-#define IS_LOCK_FILE(ST) S_ISLNK ((ST).st_mode)
-#endif
+#define MAKE_LOCK_NAME(lockname, fname) \
+ (lockname = SAFE_ALLOCA (SBYTES (fname) + 2 + 1), \
+ fill_in_lock_file_name (lockname, fname))
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 && !IS_LOCK_FILE (st))
- {
- 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);
}
static int
@@ -374,7 +343,7 @@
}
#else
err = symlink (lock_info_str, lfname);
- if (errno == EEXIST && force)
+ if (err != 0 && errno == EEXIST && force)
{
unlink (lfname);
err = symlink (lock_info_str, lfname);
@@ -434,6 +403,8 @@
#else
int fd = emacs_open (lfname, O_RDONLY | O_BINARY, S_IREAD);
ssize_t nbytes;
+ /* 256 chars for user, 1024 chars for host, 10 digits for each of 2 int's. */
+ enum { MAX_LFINFO = 256 + 1024 + 10 + 10 + 2 };
char lfinfo[MAX_LFINFO + 1];
if (fd < 0)
@@ -595,6 +566,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
@@ -680,9 +652,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);
@@ -691,6 +664,8 @@
if (current_lock_owner (0, lfname) == 2)
unlink (lfname);
+
+ SAFE_FREE ();
}
void
@@ -756,9 +731,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);
@@ -775,6 +751,7 @@
if (owner > 0)
FREE_LOCK_INFO (locker);
+ SAFE_FREE ();
return ret;
}
next prev parent reply other threads:[~2013-02-25 23:40 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-24 22:48 bug#13807: The lock for 'DIR/FILE' should always be 'DIR/.#FILE' Paul Eggert
2013-02-25 19:57 ` Glenn Morris
2013-02-25 23:40 ` Paul Eggert [this message]
2013-02-26 22:19 ` bug#13807: updated version to avoid MS-Windows vs non-MS-Windows clashes Paul Eggert
2013-02-27 18:49 ` Eli Zaretskii
2013-03-02 20:43 ` Paul Eggert
2013-03-02 21:17 ` Eli Zaretskii
2013-03-02 22:37 ` Paul Eggert
2013-03-03 16:39 ` Eli Zaretskii
2013-03-03 23:56 ` Paul Eggert
2013-03-04 16:50 ` Eli Zaretskii
2013-03-05 2:25 ` Paul Eggert
2013-03-05 18:38 ` Eli Zaretskii
2013-03-05 22:38 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=512BF65B.5010500@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=13807@debbugs.gnu.org \
--cc=rgm@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.