Bugfix: mapfile locking is broken/racy

While we attempt to use a lockfile to grant exclusive access to the
mapfile, our implementation is buggy.  Specifically, we create a lockfile,
then lock it, at which point new instances can open the lockfile and
attempt to lock it, which will cause them to block.  However, when we are
ready to unlock it, we unlink the file.  This causes existing lock waiters
to get a lock on an unlinked inode while a different instance may now
create a new lockfile and get an exclusive lock on it.

There are several possible fixes.  The chosen one is to test if
->s_nlink is zero after we get the lock and to retry if it isn't.
This means:
  - failing to unlink a file doesn't leave a stale lock
  - we can block waiting to get a lock rather than busy-waiting
  - we don't need to leave a lock file permanently in place.

Reported-by: Doug Ledford <dledford@redhat.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/mapfile.c b/mapfile.c
index 79a6bd80cb70bb4e33c84617cce5b8469a0e17ff..8e40b6b7c14cbadf7f0e00c3a53c853ecea2a60e 100644 (file)
--- a/mapfile.c
+++ b/mapfile.c
@@ -119,7 +119,8 @@ static FILE *lf = NULL;
 static int lwhich = 0;
 int map_lock(struct map_ent **melp)
 {
-       if (lf == NULL) {
+       while (lf == NULL) {
+               struct stat buf;
                lf = open_map(2, &lwhich);
                if (lf == NULL)
                        return -1;
@@ -128,6 +129,15 @@ int map_lock(struct map_ent **melp)
                        lf = NULL;
                        return -1;
                }
+               if (fstat(fileno(lf), &buf) != 0 ||
+                   buf.st_nlink == 0) {
+                       /* The owner of the lock unlinked it,
+                        * so we have a lock on a stale file,
+                        * try again
+                        */
+                       fclose(lf);
+                       lf = NULL;
+               }
        }
        if (*melp)
                map_free(*melp);
@@ -138,10 +148,13 @@ int map_lock(struct map_ent **melp)
 void map_unlock(struct map_ent **melp)
 {
        if (lf) {
-               flock(fileno(lf), LOCK_UN);
+               /* must unlink before closing the file,
+                * as only the owner of the lock may
+                * unlink the file
+                */
+               unlink(mapname[lwhich][2]);
                fclose(lf);
        }
-       unlink(mapname[lwhich][2]);
        lf = NULL;
 }