lockfile: Fix hang locking through a dangling symlink.

open() with O_CREAT|O_EXCL yields EEXIST if the file being opened is a
symlink.  lockfile_try_lock() interpreted that error code to mean that
some other process had created the lock file in the meantime, so it went
around its loop again, which found out the same thing, which led to a hang.

This commit fixes the problem by dropping O_EXCL.  I don't see any reason
that it's actually necessary.  That means that the loop itself is
unnecessary, so this commit drops that too.

Debian bug #681880.
CC: 681880@bugs.debian.org
Reported-by: Bastian Blank <waldi@debian.org>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Reviewed-by: Simon Horman <horms@verge.net.au>

diff --git a/lib/lockfile.c b/lib/lockfile.c
index e0f6328710bd6ab52e4cd0e7a81428f923d8a720..c55be66161a0d05dab2effe02d63680cf150a5b7 100644 (file)
--- a/lib/lockfile.c
+++ b/lib/lockfile.c
@@ -1,4 +1,4 @@
- /* Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
+ /* Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -221,41 +221,23 @@ lockfile_try_lock(const char *name, bool block, struct lockfile **lockfilep)
 
     *lockfilep = NULL;
 
-    /* Open the lock file, first creating it if necessary. */
-    for (;;) {
-        /* Check whether we've already got a lock on that file. */
-        if (!stat(name, &s)) {
-            if (lockfile_find(s.st_dev, s.st_ino)) {
-                return EDEADLK;
-            }
-        } else if (errno != ENOENT) {
-            VLOG_WARN("%s: failed to stat lock file: %s",
-                      name, strerror(errno));
-            return errno;
-        }
-
-        /* Try to open an existing lock file. */
-        fd = open(name, O_RDWR);
-        if (fd >= 0) {
-            break;
-        } else if (errno != ENOENT) {
-            VLOG_WARN("%s: failed to open lock file: %s",
-                      name, strerror(errno));
-            return errno;
-        }
-
-        /* Try to create a new lock file. */
-        VLOG_INFO("%s: lock file does not exist, creating", name);
-        fd = open(name, O_RDWR | O_CREAT | O_EXCL, 0600);
-        if (fd >= 0) {
-            break;
-        } else if (errno != EEXIST) {
-            VLOG_WARN("%s: failed to create lock file: %s",
-                      name, strerror(errno));
-            return errno;
+    /* Check whether we've already got a lock on that file. */
+    if (!stat(name, &s)) {
+        if (lockfile_find(s.st_dev, s.st_ino)) {
+            return EDEADLK;
         }
+    } else if (errno != ENOENT) {
+        VLOG_WARN("%s: failed to stat lock file: %s",
+                  name, strerror(errno));
+        return errno;
+    }
 
-        /* Someone else created the lock file.  Try again. */
+    /* Open the lock file. */
+    fd = open(name, O_RDWR | O_CREAT, 0600);
+    if (fd < 0) {
+        VLOG_WARN("%s: failed to open lock file: %s",
+                  name, strerror(errno));
+        return errno;
     }
 
     /* Get the inode and device number for the lock table. */

diff --git a/tests/lockfile.at b/tests/lockfile.at
index 9cc95a8f2b65cc4e006f46cf6858878602c9aff6..1fa0342f2d3d26dc3200a6c42b63892fff9f7653 100644 (file)
--- a/tests/lockfile.at
+++ b/tests/lockfile.at
@@ -18,3 +18,4 @@ CHECK_LOCKFILE([lock_and_unlock_allows_other_process], [1])
 CHECK_LOCKFILE([lock_timeout_gets_the_lock], [1])
 CHECK_LOCKFILE([lock_timeout_runs_out], [1])
 CHECK_LOCKFILE([lock_multiple], [0])
+CHECK_LOCKFILE([lock_symlink], [0])

diff --git a/tests/test-lockfile.c b/tests/test-lockfile.c
index d28c1b4b7089e227d3aead64794741df42ffd3bd..808ed1e351ddd06da5c5dcff2bfe0d8e77d48560 100644 (file)
--- a/tests/test-lockfile.c
+++ b/tests/test-lockfile.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
 
 #include <errno.h>
 #include <stdlib.h>
+#include <sys/stat.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
@@ -214,6 +215,40 @@ run_lock_multiple(void)
     lockfile_unlock(a);
 }
 
+/* Checks that locking a dangling symlink works OK.  (It used to hang.) */
+static void
+run_lock_symlink(void)
+{
+    struct lockfile *a, *b, *dummy;
+    struct stat s;
+
+    /* Create a symlink .a.~lock~ pointing to .b.~lock~. */
+    CHECK(symlink(".b.~lock~", ".a.~lock~"), 0);
+    CHECK(lstat(".a.~lock~", &s), 0);
+    CHECK(S_ISLNK(s.st_mode) != 0, 1);
+    CHECK(stat(".a.~lock~", &s), -1);
+    CHECK(errno, ENOENT);
+    CHECK(stat(".b.~lock~", &s), -1);
+    CHECK(errno, ENOENT);
+
+    CHECK(lockfile_lock("a", 0, &a), 0);
+    CHECK(lockfile_lock("a", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("b", 0, &dummy), EDEADLK);
+    lockfile_unlock(a);
+
+    CHECK(lockfile_lock("b", 0, &b), 0);
+    CHECK(lockfile_lock("b", 0, &dummy), EDEADLK);
+    CHECK(lockfile_lock("a", 0, &dummy), EDEADLK);
+    lockfile_unlock(b);
+
+    CHECK(lstat(".a.~lock~", &s), 0);
+    CHECK(S_ISLNK(s.st_mode) != 0, 1);
+    CHECK(stat(".a.~lock~", &s), 0);
+    CHECK(S_ISREG(s.st_mode) != 0, 1);
+    CHECK(stat(".b.~lock~", &s), 0);
+    CHECK(S_ISREG(s.st_mode) != 0, 1);
+}
+
 static void
 run_help(void)
 {
@@ -239,6 +274,7 @@ static const struct test tests[] = {
     TEST(lock_timeout_gets_the_lock),
     TEST(lock_timeout_runs_out),
     TEST(lock_multiple),
+    TEST(lock_symlink),
     TEST(help),
     { NULL, NULL }
 #undef TEST