From: Rusty Russell Date: Fri, 20 May 2011 06:23:12 +0000 (+0930) Subject: tdb2: fix O_RDONLY opens. X-Git-Url: https://git.ozlabs.org/?p=ccan;a=commitdiff_plain;h=de868b8eee34e39b4465dd9def9141b97926e847 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 666aa090..76b8bc31 100644 --- 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 c3feecf7..def81677 100644 --- 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 369511cd..7b79cc3e 100644 --- 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 af0ee9b7..a62efbad 100644 --- a/ccan/tdb2/test/failtest_helper.h +++ b/ccan/tdb2/test/failtest_helper.h @@ -4,7 +4,7 @@ #include /* 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 index 00000000..0f1a4343 --- /dev/null +++ b/ccan/tdb2/test/run-05-readonly-open.c @@ -0,0 +1,88 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#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 9205828a..b13223bc 100644 --- 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; }