From 6c897377f68fc23ca9a8b23a6ca204517998b2e9 Mon Sep 17 00:00:00 2001 From: Guus Sliepen Date: Sat, 9 Nov 2019 17:52:19 +0100 Subject: [PATCH] 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(). --- src/conf.c | 38 ++++++++++--------- src/conf.h | 1 + src/meshlink.c | 81 +++++++++++++++++++++++++++-------------- src/meshlink_internal.h | 2 +- test/basic.c | 26 ++++++++++--- test/basicpp.cpp | 69 +++++++++++++++++++++-------------- test/encrypted.c | 13 ++++++- 7 files changed, 149 insertions(+), 81 deletions(-) diff --git a/src/conf.c b/src/conf.c index ca107b0a..7f92048c 100644 --- 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 b3763ea7..49598ee5 100644 --- 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 dd571190..e6aea4dd 100644 --- 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 bad58255..5f87632c 100644 --- 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 4ad1c93f..f591df7b 100644 --- a/test/basic.c +++ b/test/basic.c @@ -8,6 +8,7 @@ #include #include #include +#include #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 c52f394a..ff49e3d4 100644 --- a/test/basicpp.cpp +++ b/test/basicpp.cpp @@ -3,59 +3,72 @@ #include #include #include +#include #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 a97e0eec..c7d5bc4b 100644 --- a/test/encrypted.c +++ b/test/encrypted.c @@ -8,6 +8,7 @@ #include #include #include +#include #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); } -- 2.39.2