tdb2: fix O_RDONLY opens.

We tried to get a F_WRLCK on the open lock; we shouldn't do that for a
read-only tdb.  (TDB1 gets away with it because a read-only open skips
all locking).

We also avoid leaking the fd in two tdb_open() failure paths revealed
by this extra testing.

diff --git a/ccan/tdb2/lock.c b/ccan/tdb2/lock.c
index 666aa09038a731d2f8c0b36d605c08c8d5409235..76b8bc3157977462fbebc3983caad5dbb366ed5c 100644 (file)
--- a/ccan/tdb2/lock.c
+++ b/ccan/tdb2/lock.c
@@ -328,13 +328,13 @@ enum TDB_ERROR tdb_lock_and_recover(struct tdb_context *tdb)
                return ecode;
        }
 
-       ecode = tdb_lock_open(tdb, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
+       ecode = tdb_lock_open(tdb, F_WRLCK, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
        if (ecode != TDB_SUCCESS) {
                tdb_allrecord_unlock(tdb, F_WRLCK);
                return ecode;
        }
        ecode = tdb_transaction_recover(tdb);
-       tdb_unlock_open(tdb);
+       tdb_unlock_open(tdb, F_WRLCK);
        tdb_allrecord_unlock(tdb, F_WRLCK);
 
        return ecode;
@@ -615,14 +615,15 @@ again:
        goto again;
 }
 
-enum TDB_ERROR tdb_lock_open(struct tdb_context *tdb, enum tdb_lock_flags flags)
+enum TDB_ERROR tdb_lock_open(struct tdb_context *tdb,
+                            int ltype, enum tdb_lock_flags flags)
 {
-       return tdb_nest_lock(tdb, TDB_OPEN_LOCK, F_WRLCK, flags);
+       return tdb_nest_lock(tdb, TDB_OPEN_LOCK, ltype, flags);
 }
 
-void tdb_unlock_open(struct tdb_context *tdb)
+void tdb_unlock_open(struct tdb_context *tdb, int ltype)
 {
-       tdb_nest_unlock(tdb, TDB_OPEN_LOCK, F_WRLCK);
+       tdb_nest_unlock(tdb, TDB_OPEN_LOCK, ltype);
 }
 
 bool tdb_has_open_lock(struct tdb_context *tdb)

diff --git a/ccan/tdb2/open.c b/ccan/tdb2/open.c
index c3feecf73f34acfd08cabbd81570dd1883ddebc4..def816775c76efa8cd80db786084926c6e04e5d8 100644 (file)
--- a/ccan/tdb2/open.c
+++ b/ccan/tdb2/open.c
@@ -333,6 +333,7 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
        struct tdb_attribute_openhook *openhook = NULL;
        tdb_bool_err berr;
        enum TDB_ERROR ecode;
+       int openlock;
 
        tdb = malloc(sizeof(*tdb) + (name ? strlen(name) + 1 : 0));
        if (!tdb) {
@@ -399,9 +400,11 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
        if ((open_flags & O_ACCMODE) == O_RDONLY) {
                tdb->read_only = true;
                tdb->mmap_flags = PROT_READ;
+               openlock = F_RDLCK;
        } else {
                tdb->read_only = false;
                tdb->mmap_flags = PROT_READ | PROT_WRITE;
+               openlock = F_WRLCK;
        }
 
        /* internal databases don't need any of the rest. */
@@ -446,12 +449,15 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
                        tdb_logerr(tdb, TDB_ERR_IO, TDB_LOG_ERROR,
                                   "tdb_open: could not stat open %s: %s",
                                   name, strerror(errno));
+                       close(fd);
                        goto fail_errno;
                }
 
                ecode = tdb_new_file(tdb);
-               if (ecode != TDB_SUCCESS)
+               if (ecode != TDB_SUCCESS) {
+                       close(fd);
                        goto fail;
+               }
 
                tdb->file->next = files;
                tdb->file->fd = fd;
@@ -462,7 +468,7 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
        }
 
        /* ensure there is only one process initialising at once */
-       ecode = tdb_lock_open(tdb, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
+       ecode = tdb_lock_open(tdb, openlock, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
        if (ecode != TDB_SUCCESS) {
                saved_errno = errno;
                goto fail_errno;
@@ -534,7 +540,7 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
                        goto fail;
        }
 
-       tdb_unlock_open(tdb);
+       tdb_unlock_open(tdb, openlock);
 
        /* This make sure we have current map_size and mmap. */
        tdb->methods->oob(tdb, tdb->file->map_size + 1, true);

diff --git a/ccan/tdb2/private.h b/ccan/tdb2/private.h
index 369511cd5d2f86eb11748df04d932bf8fb85d461..7b79cc3eefced99d4ba84a4820e0d31735227dd3 100644 (file)
--- a/ccan/tdb2/private.h
+++ b/ccan/tdb2/private.h
@@ -570,8 +570,8 @@ enum TDB_ERROR tdb_allrecord_upgrade(struct tdb_context *tdb);
 
 /* Serialize db open. */
 enum TDB_ERROR tdb_lock_open(struct tdb_context *tdb,
-                            enum tdb_lock_flags flags);
-void tdb_unlock_open(struct tdb_context *tdb);
+                            int ltype, enum tdb_lock_flags flags);
+void tdb_unlock_open(struct tdb_context *tdb, int ltype);
 bool tdb_has_open_lock(struct tdb_context *tdb);
 
 /* Serialize db expand. */

diff --git a/ccan/tdb2/test/failtest_helper.h b/ccan/tdb2/test/failtest_helper.h
index af0ee9b7db678b946ff0510e05f4067c2b757468..a62efbad58eb588bb4c1b505d34545754d1f2c4a 100644 (file)
--- a/ccan/tdb2/test/failtest_helper.h
+++ b/ccan/tdb2/test/failtest_helper.h
@@ -4,7 +4,7 @@
 #include <stdbool.h>
 
 /* FIXME: Check these! */
-#define INITIAL_TDB_MALLOC     "open.c", 337, FAILTEST_MALLOC
+#define INITIAL_TDB_MALLOC     "open.c", 338, FAILTEST_MALLOC
 #define URANDOM_OPEN           "open.c", 45, FAILTEST_OPEN
 #define URANDOM_READ           "open.c", 25, FAILTEST_READ
 

diff --git a/ccan/tdb2/test/run-05-readonly-open.c b/ccan/tdb2/test/run-05-readonly-open.c
new file mode 100644 (file)
index 0000000..0f1a434
--- /dev/null
+++ b/ccan/tdb2/test/run-05-readonly-open.c
@@ -0,0 +1,88 @@
+#include <ccan/failtest/failtest_override.h>
+#include <ccan/tdb2/tdb.c>
+#include <ccan/tdb2/open.c>
+#include <ccan/tdb2/free.c>
+#include <ccan/tdb2/lock.c>
+#include <ccan/tdb2/io.c>
+#include <ccan/tdb2/hash.c>
+#include <ccan/tdb2/transaction.c>
+#include <ccan/tdb2/check.c>
+#include <ccan/tap/tap.h>
+#include <ccan/failtest/failtest.h>
+#include "logging.h"
+#include "failtest_helper.h"
+
+static bool failtest_suppress = false;
+
+/* Don't need to test everything here, just want expand testing. */
+static enum failtest_result
+suppress_failure(struct failtest_call *history, unsigned num)
+{
+       if (failtest_suppress)
+               return FAIL_DONT_FAIL;
+       return block_repeat_failures(history, num);
+}
+
+int main(int argc, char *argv[])
+{
+       unsigned int i;
+       struct tdb_context *tdb;
+       int flags[] = { TDB_DEFAULT, TDB_NOMMAP,
+                       TDB_CONVERT, TDB_NOMMAP|TDB_CONVERT };
+       struct tdb_data key = tdb_mkdata("key", 3);
+       struct tdb_data data = tdb_mkdata("data", 4), d;
+       union tdb_attribute seed_attr;
+       unsigned int msgs = 0;
+
+       failtest_init(argc, argv);
+       failtest_hook = suppress_failure;
+       failtest_exit_check = exit_check_log;
+
+       seed_attr.base.attr = TDB_ATTRIBUTE_SEED;
+       seed_attr.base.next = &tap_log_attr;
+       seed_attr.seed.seed = 0;
+
+       failtest_suppress = true;
+       plan_tests(sizeof(flags) / sizeof(flags[0]) * 11);
+       for (i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) {
+               tdb = tdb_open("run-05-readonly-open.tdb", flags[i],
+                              O_RDWR|O_CREAT|O_TRUNC, 0600, &seed_attr);
+               ok1(tdb_store(tdb, key, data, TDB_INSERT) == 0);
+               tdb_close(tdb);
+
+               failtest_suppress = false;
+               tdb = tdb_open("run-05-readonly-open.tdb", flags[i],
+                              O_RDONLY, 0600, &tap_log_attr);
+               if (!ok1(tdb))
+                       break;
+               ok1(tap_log_messages == msgs);
+               /* Fetch should succeed, stores should fail. */
+               if (!ok1(tdb_fetch(tdb, key, &d) == 0))
+                       goto fail;
+               ok1(tdb_deq(d, data));
+               free(d.dptr);
+               if (!ok1(tdb_store(tdb, key, data, TDB_MODIFY)
+                        == TDB_ERR_RDONLY))
+                       goto fail;
+               ok1(tap_log_messages == ++msgs);
+               if (!ok1(tdb_store(tdb, key, data, TDB_INSERT)
+                        == TDB_ERR_RDONLY))
+                       goto fail;
+               ok1(tap_log_messages == ++msgs);
+               failtest_suppress = true;
+               ok1(tdb_check(tdb, NULL, NULL) == 0);
+               tdb_close(tdb);
+               ok1(tap_log_messages == msgs);
+               /* SIGH: failtest bug, it doesn't save the tdb file because
+                * we have it read-only.  If we go around again, it gets
+                * changed underneath us and things get screwy. */
+               if (failtest_has_failed())
+                       break;
+       }
+       failtest_exit(exit_status());
+
+fail:
+       failtest_suppress = true;
+       tdb_close(tdb);
+       failtest_exit(exit_status());
+}

diff --git a/ccan/tdb2/transaction.c b/ccan/tdb2/transaction.c
index 9205828a8a16138d754e0013b68d9b91396b1894..b13223bc2e18c7f82b6fdf4615adc69ca14bbffb 100644 (file)
--- a/ccan/tdb2/transaction.c
+++ b/ccan/tdb2/transaction.c
@@ -509,7 +509,7 @@ static void _tdb_transaction_cancel(struct tdb_context *tdb)
        tdb_transaction_unlock(tdb, F_WRLCK);
 
        if (tdb_has_open_lock(tdb))
-               tdb_unlock_open(tdb);
+               tdb_unlock_open(tdb, F_WRLCK);
 
        SAFE_FREE(tdb->transaction);
 }
@@ -1005,7 +1005,7 @@ static enum TDB_ERROR _tdb_transaction_prepare_commit(struct tdb_context *tdb)
 
        /* get the open lock - this prevents new users attaching to the database
           during the commit */
-       ecode = tdb_lock_open(tdb, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
+       ecode = tdb_lock_open(tdb, F_WRLCK, TDB_LOCK_WAIT|TDB_LOCK_NOCHECK);
        if (ecode != TDB_SUCCESS) {
                return ecode;
        }