From bef6f1b02e95370ecb2cb44be87c82afc9cb74b2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 31 Aug 2011 15:31:08 +0930 Subject: [PATCH] tdb2: check lock owner in tdb1 backend. This reports errors if we fork() while holding a lock, or misuse a tdb which we have dual-opened. --- ccan/tdb2/lock.c | 14 +++++-- ccan/tdb2/private.h | 6 +++ ccan/tdb2/tdb1_lock.c | 95 ++++++++++++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 27 deletions(-) diff --git a/ccan/tdb2/lock.c b/ccan/tdb2/lock.c index 7d4311c8..bf62d971 100644 --- a/ccan/tdb2/lock.c +++ b/ccan/tdb2/lock.c @@ -30,7 +30,7 @@ #include /* If we were threaded, we could wait for unlock, but we're not, so fail. */ -static enum TDB_ERROR owner_conflict(struct tdb_context *tdb, const char *call) +enum TDB_ERROR owner_conflict(struct tdb_context *tdb, const char *call) { return tdb_logerr(tdb, TDB_ERR_LOCK, TDB_LOG_USE_ERROR, "%s: lock owned by another tdb in this process.", @@ -38,8 +38,7 @@ static enum TDB_ERROR owner_conflict(struct tdb_context *tdb, const char *call) } /* If we fork, we no longer really own locks. */ -static bool check_lock_pid(struct tdb_context *tdb, - const char *call, bool log) +bool check_lock_pid(struct tdb_context *tdb, const char *call, bool log) { /* No locks? No problem! */ if (tdb->file->allrecord_lock.count == 0 @@ -787,6 +786,11 @@ enum TDB_ERROR tdb_unlock_hashes(struct tdb_context *tdb, return tdb_logerr(tdb, TDB_ERR_LOCK, TDB_LOG_ERROR, "tdb_unlock_hashes RO allrecord!"); } + if (tdb->file->allrecord_lock.owner != tdb) { + return tdb_logerr(tdb, TDB_ERR_LOCK, TDB_LOG_USE_ERROR, + "tdb_unlock_hashes:" + " not locked by us!"); + } return TDB_SUCCESS; } @@ -817,6 +821,10 @@ enum TDB_ERROR tdb_lock_free_bucket(struct tdb_context *tdb, tdb_off_t b_off, if (!check_lock_pid(tdb, "tdb_lock_free_bucket", true)) return TDB_ERR_LOCK; + if (tdb->file->allrecord_lock.owner != tdb) { + return owner_conflict(tdb, "tdb_lock_free_bucket"); + } + if (tdb->file->allrecord_lock.ltype == F_WRLCK) return 0; return tdb_logerr(tdb, TDB_ERR_LOCK, TDB_LOG_ERROR, diff --git a/ccan/tdb2/private.h b/ccan/tdb2/private.h index 30338d77..26ca7b30 100644 --- a/ccan/tdb2/private.h +++ b/ccan/tdb2/private.h @@ -463,6 +463,12 @@ enum TDB_ERROR tdb_read_convert(struct tdb_context *tdb, tdb_off_t off, void tdb_inc_seqnum(struct tdb_context *tdb); /* lock.c: */ +/* Print message because another tdb owns a lock we want. */ +enum TDB_ERROR owner_conflict(struct tdb_context *tdb, const char *call); + +/* If we fork, we no longer really own locks. */ +bool check_lock_pid(struct tdb_context *tdb, const char *call, bool log); + /* Lock/unlock a range of hashes. */ enum TDB_ERROR tdb_lock_hashes(struct tdb_context *tdb, tdb_off_t hash_lock, tdb_len_t hash_range, diff --git a/ccan/tdb2/tdb1_lock.c b/ccan/tdb2/tdb1_lock.c index 495e1d1b..e59874fb 100644 --- a/ccan/tdb2/tdb1_lock.c +++ b/ccan/tdb2/tdb1_lock.c @@ -149,34 +149,43 @@ static int tdb1_lock_list(struct tdb_context *tdb, int list, int ltype, bool check = false; /* a allrecord lock allows us to avoid per chain locks */ - if (tdb->file->allrecord_lock.count && - (ltype == tdb->file->allrecord_lock.ltype || ltype == F_RDLCK)) { - return 0; + if (tdb->file->allrecord_lock.count) { + if (!check_lock_pid(tdb, "tdb1_lock_list", true)) { + tdb->last_error = TDB_ERR_LOCK; + return -1; + } + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error = owner_conflict(tdb, "tdb1_lock_list"); + return -1; + } + if (ltype == tdb->file->allrecord_lock.ltype + || ltype == F_RDLCK) { + return 0; + } + tdb->last_error = tdb_logerr(tdb, TDB_ERR_LOCK, + TDB_LOG_USE_ERROR, + "tdb1_lock_list:" + " already have read lock"); + return -1; } - if (tdb->file->allrecord_lock.count) { - tdb->last_error = TDB_ERR_LOCK; - ret = -1; - } else { - /* Only check when we grab first data lock. */ - check = !have_data_locks(tdb); - ret = tdb1_nest_lock(tdb, lock_offset(list), ltype, waitflag); + /* Only check when we grab first data lock. */ + check = !have_data_locks(tdb); + ret = tdb1_nest_lock(tdb, lock_offset(list), ltype, waitflag); + + if (ret == 0 && check) { + tdb_bool_err berr = tdb1_needs_recovery(tdb); - if (ret == 0 && check) { - tdb_bool_err berr = tdb1_needs_recovery(tdb); + if (berr < 0) { + return -1; + } + if (berr == true) { + tdb1_nest_unlock(tdb, lock_offset(list), ltype); - if (berr < 0) { + if (tdb1_lock_and_recover(tdb) == -1) { return -1; } - if (berr == true) { - tdb1_nest_unlock(tdb, lock_offset(list), ltype); - - if (tdb1_lock_and_recover(tdb) == -1) { - return -1; - } - return tdb1_lock_list(tdb, list, ltype, - waitflag); - } + return tdb1_lock_list(tdb, list, ltype, waitflag); } } return ret; @@ -222,6 +231,10 @@ int tdb1_unlock(struct tdb_context *tdb, int list, int ltype) /* a global lock allows us to avoid per chain locks */ if (tdb->file->allrecord_lock.count && (ltype == tdb->file->allrecord_lock.ltype || ltype == F_RDLCK)) { + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error = owner_conflict(tdb, "tdb1_unlock"); + return -1; + } return 0; } @@ -314,9 +327,9 @@ int tdb1_allrecord_lock(struct tdb_context *tdb, int ltype, return -1; } - /* FIXME: Temporary cast. */ - tdb->file->allrecord_lock.owner = (void *)(struct tdb_context *)tdb; + tdb->file->allrecord_lock.owner = tdb; tdb->file->allrecord_lock.count = 1; + tdb->file->locker = getpid(); /* If it's upgradable, it's actually exclusive so we can treat * it as a write lock. */ tdb->file->allrecord_lock.ltype = upgradable ? F_WRLCK : ltype; @@ -362,6 +375,11 @@ int tdb1_allrecord_unlock(struct tdb_context *tdb, int ltype) } if (tdb->file->allrecord_lock.count > 1) { + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error + = owner_conflict(tdb, "tdb1_allrecord_unlock"); + return -1; + } tdb->file->allrecord_lock.count--; return 0; } @@ -412,6 +430,15 @@ int tdb1_chainunlock_read(struct tdb_context *tdb, TDB_DATA key) int tdb1_lock_record(struct tdb_context *tdb, tdb1_off_t off) { if (tdb->file->allrecord_lock.count) { + if (!check_lock_pid(tdb, "tdb1_lock_record", true)) { + tdb->last_error = TDB_ERR_LOCK; + return -1; + } + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error = owner_conflict(tdb, + "tdb1_lock_record"); + return -1; + } return 0; } return off ? tdb1_brlock(tdb, F_RDLCK, off, 1, TDB_LOCK_WAIT) : 0; @@ -429,6 +456,15 @@ int tdb1_write_lock_record(struct tdb_context *tdb, tdb1_off_t off) if (i->off == off) return -1; if (tdb->file->allrecord_lock.count) { + if (!check_lock_pid(tdb, "tdb1_write_lock_record", true)) { + tdb->last_error = TDB_ERR_LOCK; + return -1; + } + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error + = owner_conflict(tdb, "tdb1_write_lock_record"); + return -1; + } if (tdb->file->allrecord_lock.ltype == F_WRLCK) { return 0; } @@ -440,6 +476,12 @@ int tdb1_write_lock_record(struct tdb_context *tdb, tdb1_off_t off) int tdb1_write_unlock_record(struct tdb_context *tdb, tdb1_off_t off) { if (tdb->file->allrecord_lock.count) { + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error + = owner_conflict(tdb, + "tdb1_write_unlock_record"); + return -1; + } return 0; } return tdb1_brunlock(tdb, F_WRLCK, off, 1); @@ -452,6 +494,11 @@ int tdb1_unlock_record(struct tdb_context *tdb, tdb1_off_t off) uint32_t count = 0; if (tdb->file->allrecord_lock.count) { + if (tdb->file->allrecord_lock.owner != tdb) { + tdb->last_error = owner_conflict(tdb, + "tdb1_unlock_record"); + return -1; + } return 0; } -- 2.39.2