Use a separate lockfile to lock the configuration directory.

We can't use meshlink.conf as the lock file, since it can move around.
Instead, create meshlink.lock right below confbase, and keep it always
locked while a meshlink handle is valid.

meshlink_destroy() will also use this lockfile to ensure we don't destroy
a directory that is still in use, and prevent race conditions between
meshlink_destroy() and meshlink_open().

diff --git a/src/conf.c b/src/conf.c
index ca107b0a062c6dc5470dab09e8da44a909a0b4f4..7f92048ce30603245946a047a264172861a2e304 100644 (file)
--- a/src/conf.c
+++ b/src/conf.c
@@ -96,7 +96,7 @@ static void deltree(const char *dirname) {
        rmdir(dirname);
 }
 
-static bool sync_path(const char *pathname) {
+bool sync_path(const char *pathname) {
        assert(pathname);
 
        int fd = open(pathname, O_RDONLY);
@@ -153,11 +153,6 @@ bool config_init(meshlink_handle_t *mesh, const char *conf_subdir) {
                return true;
        }
 
-       if(mkdir(mesh->confbase, 0700) && errno != EEXIST) {
-               logger(mesh, MESHLINK_DEBUG, "Could not create directory %s: %s\n", mesh->confbase, strerror(errno));
-               return false;
-       }
-
        char path[PATH_MAX];
 
        // Create "current" sub-directory in the confbase
@@ -191,7 +186,7 @@ bool config_destroy(const char *confbase, const char *conf_subdir) {
        assert(conf_subdir);
 
        if(!confbase) {
-               return false;
+               return true;
        }
 
        struct stat st;
@@ -456,35 +451,42 @@ bool meshlink_confbase_exists(meshlink_handle_t *mesh) {
        return confbase_exists;
 }
 
-/// Lock the main configuration file.
+/// Lock the main configuration file. Creates confbase if necessary.
 bool main_config_lock(meshlink_handle_t *mesh) {
        if(!mesh->confbase) {
                return true;
        }
 
+       if(mkdir(mesh->confbase, 0700) && errno != EEXIST) {
+               logger(NULL, MESHLINK_ERROR, "Cannot create configuration directory %s: %s", mesh->confbase, strerror(errno));
+               meshlink_close(mesh);
+               meshlink_errno = MESHLINK_ESTORAGE;
+               return NULL;
+       }
+
        char path[PATH_MAX];
-       make_main_path(mesh, "current", path, sizeof(path));
+       snprintf(path, sizeof(path), "%s" SLASH "meshlink.lock", mesh->confbase);
 
-       mesh->conffile = fopen(path, "r");
+       mesh->lockfile = fopen(path, "w+");
 
-       if(!mesh->conffile) {
+       if(!mesh->lockfile) {
                logger(NULL, MESHLINK_ERROR, "Cannot not open %s: %s\n", path, strerror(errno));
                meshlink_errno = MESHLINK_ESTORAGE;
                return false;
        }
 
 #ifdef FD_CLOEXEC
-       fcntl(fileno(mesh->conffile), F_SETFD, FD_CLOEXEC);
+       fcntl(fileno(mesh->lockfile), F_SETFD, FD_CLOEXEC);
 #endif
 
 #ifdef HAVE_MINGW
        // TODO: use _locking()?
 #else
 
-       if(flock(fileno(mesh->conffile), LOCK_EX | LOCK_NB) != 0) {
+       if(flock(fileno(mesh->lockfile), LOCK_EX | LOCK_NB) != 0) {
                logger(NULL, MESHLINK_ERROR, "Cannot lock %s: %s\n", path, strerror(errno));
-               fclose(mesh->conffile);
-               mesh->conffile = NULL;
+               fclose(mesh->lockfile);
+               mesh->lockfile = NULL;
                meshlink_errno = MESHLINK_EBUSY;
                return false;
        }
@@ -496,9 +498,9 @@ bool main_config_lock(meshlink_handle_t *mesh) {
 
 /// Unlock the main configuration file.
 void main_config_unlock(meshlink_handle_t *mesh) {
-       if(mesh->conffile) {
-               fclose(mesh->conffile);
-               mesh->conffile = NULL;
+       if(mesh->lockfile) {
+               fclose(mesh->lockfile);
+               mesh->lockfile = NULL;
        }
 }
 

diff --git a/src/conf.h b/src/conf.h
index b3763ea7b0970c00c2a8382af65d097959fb44e5..49598ee5ba33073b7b79bdeb14c8503abac1412f 100644 (file)
--- a/src/conf.h
+++ b/src/conf.h
@@ -40,6 +40,7 @@ extern bool config_destroy(const char *confbase, const char *conf_subdir);
 extern bool config_copy(struct meshlink_handle *mesh, const char *src_dir_name, const void *src_key, const char *dst_dir_name, const void *dst_key);
 extern bool config_rename(struct meshlink_handle *mesh, const char *old_conf_subdir, const char *new_conf_subdir);
 extern bool config_sync(struct meshlink_handle *mesh, const char *conf_subdir);
+extern bool sync_path(const char *path) __attribute__((__warn_unused_result__));
 
 extern bool main_config_exists(struct meshlink_handle *mesh, const char *conf_subdir);
 extern bool main_config_lock(struct meshlink_handle *mesh);

diff --git a/src/meshlink.c b/src/meshlink.c
index dd571190d86f53b46965e7e2f2aa5be0dd62a174..e6aea4dd3f708a644e482ee8de504414fd4c3e1d 100644 (file)
--- a/src/meshlink.c
+++ b/src/meshlink.c
@@ -903,23 +903,10 @@ static bool meshlink_setup(meshlink_handle_t *mesh) {
                return false;
        }
 
-       if(!main_config_lock(mesh)) {
-               logger(NULL, MESHLINK_ERROR, "Cannot lock main config file\n");
-               meshlink_errno = MESHLINK_ESTORAGE;
-               return false;
-       }
-
        return true;
 }
 
 static bool meshlink_read_config(meshlink_handle_t *mesh) {
-       // Open the configuration file and lock it
-       if(!main_config_lock(mesh)) {
-               logger(NULL, MESHLINK_ERROR, "Cannot lock main config file\n");
-               meshlink_errno = MESHLINK_ESTORAGE;
-               return false;
-       }
-
        config_t config;
 
        if(!main_config_read(mesh, "current", &config, mesh->config_key)) {
@@ -1104,14 +1091,11 @@ bool meshlink_encrypted_key_rotate(meshlink_handle_t *mesh, const void *new_key,
 
        devtool_keyrotate_probe(1);
 
-       main_config_unlock(mesh);
-
        // Rename confbase/current/ to confbase/old
 
        if(!config_rename(mesh, "current", "old")) {
                logger(mesh, MESHLINK_ERROR, "Cannot rename %s/current to %s/old\n", mesh->confbase, mesh->confbase);
                meshlink_errno = MESHLINK_ESTORAGE;
-               main_config_lock(mesh);
                pthread_mutex_unlock(&mesh->mutex);
                return false;
        }
@@ -1123,18 +1107,12 @@ bool meshlink_encrypted_key_rotate(meshlink_handle_t *mesh, const void *new_key,
        if(!config_rename(mesh, "new", "current")) {
                logger(mesh, MESHLINK_ERROR, "Cannot rename %s/new to %s/current\n", mesh->confbase, mesh->confbase);
                meshlink_errno = MESHLINK_ESTORAGE;
-               main_config_lock(mesh);
                pthread_mutex_unlock(&mesh->mutex);
                return false;
        }
 
        devtool_keyrotate_probe(3);
 
-       if(!main_config_lock(mesh)) {
-               pthread_mutex_unlock(&mesh->mutex);
-               return false;
-       }
-
        // Cleanup the "old" confbase sub-directory
 
        if(!config_destroy(mesh->confbase, "old")) {
@@ -1325,6 +1303,12 @@ meshlink_handle_t *meshlink_open_ex(const meshlink_open_params_t *params) {
 
        meshlink_queue_init(&mesh->outpacketqueue);
 
+       // Atomically lock the configuration directory.
+       if(!main_config_lock(mesh)) {
+               meshlink_close(mesh);
+               return NULL;
+       }
+
        // If no configuration exists yet, create it.
 
        if(!meshlink_confbase_exists(mesh)) {
@@ -1641,16 +1625,57 @@ bool meshlink_destroy(const char *confbase) {
                return false;
        }
 
-       if(!config_destroy(confbase, "current")) {
-               logger(NULL, MESHLINK_ERROR, "Cannot remove confbase sub-directories %s: %s\n", confbase, strerror(errno));
+       /* Exit early if the confbase directory itself doesn't exist */
+       if(access(confbase, F_OK) && errno == ENOENT) {
+               return true;
+       }
+
+       /* Take the lock the same way meshlink_open() would. */
+       char lockfilename[PATH_MAX];
+       snprintf(lockfilename, sizeof(lockfilename), "%s" SLASH "meshlink.lock", confbase);
+
+       FILE *lockfile = fopen(lockfilename, "w+");
+
+       if(!lockfile) {
+               logger(NULL, MESHLINK_ERROR, "Could not open lock file %s: %s", lockfilename, strerror(errno));
+               meshlink_errno = MESHLINK_ESTORAGE;
+               return false;
+       }
+
+#ifdef FD_CLOEXEC
+       fcntl(fileno(lockfile), F_SETFD, FD_CLOEXEC);
+#endif
+
+#ifdef HAVE_MINGW
+       // TODO: use _locking()?
+#else
+
+       if(flock(fileno(lockfile), LOCK_EX | LOCK_NB) != 0) {
+               logger(NULL, MESHLINK_ERROR, "Configuration directory %s still in use\n", lockfilename);
+               fclose(lockfile);
+               meshlink_errno = MESHLINK_EBUSY;
+               return false;
+       }
+
+#endif
+
+       if(!config_destroy(confbase, "current") || !config_destroy(confbase, "new") || !config_destroy(confbase, "old")) {
+               logger(NULL, MESHLINK_ERROR, "Cannot remove sub-directories in %s: %s\n", confbase, strerror(errno));
+               return false;
+       }
+
+       if(unlink(lockfilename)) {
+               logger(NULL, MESHLINK_ERROR, "Cannot remove lock file %s: %s\n", lockfilename, strerror(errno));
+               fclose(lockfile);
+               meshlink_errno = MESHLINK_ESTORAGE;
                return false;
        }
 
-       config_destroy(confbase, "new");
-       config_destroy(confbase, "old");
+       fclose(lockfile);
 
-       if(rmdir(confbase) && errno != ENOENT) {
-               logger(NULL, MESHLINK_ERROR, "Cannot remove directory %s: %s\n", confbase, strerror(errno));
+       /* TODO: do we need to remove confbase? Potential race condition? */
+       if(!sync_path(confbase)) {
+               logger(NULL, MESHLINK_ERROR, "Cannot sync directory %s: %s\n", confbase, strerror(errno));
                meshlink_errno = MESHLINK_ESTORAGE;
                return false;
        }

diff --git a/src/meshlink_internal.h b/src/meshlink_internal.h
index bad5825591968f4d5bba0ebe88f42108bc70bbc6..5f87632c6c7742da92e0cc5620c3ac7259bbb9cc 100644 (file)
--- a/src/meshlink_internal.h
+++ b/src/meshlink_internal.h
@@ -169,7 +169,7 @@ struct meshlink_handle {
 
        // Configuration
        char *confbase;
-       FILE *conffile;
+       FILE *lockfile;
        void *config_key;
 
        // Thread management

diff --git a/test/basic.c b/test/basic.c
index 4ad1c93fb0ed6639d9eea983def0bd667564c049..f591df7b54df1e6271a9c868a80bcb78d6464e00 100644 (file)
--- a/test/basic.c
+++ b/test/basic.c
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <sys/time.h>
 #include <assert.h>
+#include <dirent.h>
 
 #include "meshlink.h"
 #include "utils.h"
@@ -21,7 +22,14 @@ int main() {
        meshlink_handle_t *mesh = meshlink_open("basic_conf", "foo", "basic", DEV_CLASS_BACKBONE);
        assert(mesh);
 
-       meshlink_set_log_cb(mesh, MESHLINK_DEBUG, log_cb);
+       // Check that we can't open a second instance of the same node.
+
+       meshlink_handle_t *mesh2 = meshlink_open("basic_conf", "foo", "basic", DEV_CLASS_BACKBONE);
+       assert(!mesh2);
+
+       // Check that we cannot destroy an instance that is in use.
+
+       assert(!meshlink_destroy("basic_conf"));
 
        // Check that our own node exists.
 
@@ -56,13 +64,21 @@ int main() {
 
        assert(meshlink_start(mesh));
        meshlink_stop(mesh);
-
-       // That's it.
-
        meshlink_close(mesh);
 
        // Destroy the mesh.
 
        assert(meshlink_destroy("basic_conf"));
-       assert(access("basic_conf", F_OK) == -1 && errno == ENOENT);
+
+       // Check that the configuration directory is completely empty.
+
+       DIR *dir = opendir("basic_conf");
+       assert(dir);
+       struct dirent *ent;
+
+       while((ent = readdir(dir))) {
+               assert(!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."));
+       }
+
+       closedir(dir);
 }

diff --git a/test/basicpp.cpp b/test/basicpp.cpp
index c52f394ae247871a251d09d283ed713a01e7fb5d..ff49e3d428f749e9d7d9845c6176d494ed47782f 100644 (file)
--- a/test/basicpp.cpp
+++ b/test/basicpp.cpp
@@ -3,59 +3,72 @@
 #include <unistd.h>
 #include <cerrno>
 #include <cassert>
+#include <dirent.h>
 
 #include "meshlink++.h"
 
 using namespace std;
 
 int main() {
+       assert(meshlink::destroy("basicpp_conf"));
+
        // Open a new meshlink instance.
 
-       assert(meshlink::destroy("basicpp_conf"));
-       meshlink::mesh mesh("basicpp_conf", "foo", "basicpp", DEV_CLASS_BACKBONE);
-       assert(mesh.isOpen());
+       {
+               meshlink::mesh mesh("basicpp_conf", "foo", "basicpp", DEV_CLASS_BACKBONE);
+               assert(mesh.isOpen());
 
-       // Check that our own node exists.
+               // Check that our own node exists.
 
-       meshlink::node *self = mesh.get_self();
-       assert(self);
-       assert(!strcmp(self->name, "foo"));
+               meshlink::node *self = mesh.get_self();
+               assert(self);
+               assert(!strcmp(self->name, "foo"));
 
-       // Disable local discovery.
+               // Disable local discovery.
 
-       mesh.enable_discovery(false);
+               mesh.enable_discovery(false);
 
-       // Start and stop the mesh.
+               // Start and stop the mesh.
 
-       assert(mesh.start());
-       mesh.stop();
+               assert(mesh.start());
+               mesh.stop();
 
-       // Make sure we can start and stop the mesh again.
+               // Make sure we can start and stop the mesh again.
 
-       assert(mesh.start());
-       mesh.stop();
+               assert(mesh.start());
+               mesh.stop();
 
-       // Close the mesh and open it again, now with a different name parameter.
+               // Close the mesh and open it again, now with a different name parameter.
 
-       mesh.close();
-       assert(mesh.open("basicpp_conf", "bar", "basicpp", DEV_CLASS_BACKBONE));
+               mesh.close();
+               assert(mesh.open("basicpp_conf", "bar", "basicpp", DEV_CLASS_BACKBONE));
 
-       // Check that the name is ignored now, and that we still are "foo".
+               // Check that the name is ignored now, and that we still are "foo".
 
-       assert(!mesh.get_node("bar"));
-       self = mesh.get_self();
-       assert(self);
-       assert(!strcmp(self->name, "foo"));
+               assert(!mesh.get_node("bar"));
+               self = mesh.get_self();
+               assert(self);
+               assert(!strcmp(self->name, "foo"));
 
-       // Start and stop the mesh.
+               // Start and stop the mesh.
 
-       mesh.enable_discovery(false);
+               mesh.enable_discovery(false);
 
-       assert(mesh.start());
-       mesh.stop();
+               assert(mesh.start());
+               mesh.stop();
+       }
+
+       // Destroy the mesh.
 
        assert(meshlink::destroy("basicpp_conf"));
-       assert(access("basic.conf", F_OK) == -1 && errno == ENOENT);
+
+       DIR *dir = opendir("basicpp_conf");
+       assert(dir);
+       struct dirent *ent;
+       while((ent = readdir(dir))) {
+               assert(!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."));
+       }
+       closedir(dir);
 
        return 0;
 }

diff --git a/test/encrypted.c b/test/encrypted.c
index a97e0eec1e1f63604808e553ab268a667db036a5..c7d5bc4b7fb310c7f640ee40f2f6e64f309a9717 100644 (file)
--- a/test/encrypted.c
+++ b/test/encrypted.c
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <sys/time.h>
 #include <assert.h>
+#include <dirent.h>
 
 #include "meshlink.h"
 #include "utils.h"
@@ -40,5 +41,15 @@ int main() {
        // Destroy the mesh.
 
        assert(meshlink_destroy("encrypted_conf"));
-       assert(access("encrypted_conf", F_OK) == -1 && errno == ENOENT);
+
+       DIR *dir = opendir("encrypted_conf");
+       assert(dir);
+       struct dirent *ent;
+
+       while((ent = readdir(dir))) {
+               fprintf(stderr, "%s\n", ent->d_name);
+               assert(!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."));
+       }
+
+       closedir(dir);
 }