From ebdf7663ae9d8b37bd6fd134667c4a25fea86ebf Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Fri, 29 Jan 2010 18:21:09 +0100 Subject: [PATCH] tdb: fix an early release of the global lock that can cause data corruption There was a bug in tdb where the tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); (ending the transaction-"mutex") was done before the /* remove the recovery marker */ This means that when a transaction is committed there is a window where another opener of the file sees the transaction marker while the transaction committer is still fully functional and working on it. This led to transaction being rolled back by that second opener of the file while transaction_commit() gave no error to the caller. This patch moves the F_UNLCK to after the recovery marker was removed, closing this window. --- lib/tdb/common/transaction.c | 31 +++++++++++++++---------------- 1 files changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c index 20f2bfc..b7410e0 100644 --- a/lib/tdb/common/transaction.c +++ b/lib/tdb/common/transaction.c @@ -591,16 +591,20 @@ int _tdb_transaction_cancel(struct tdb_context *tdb) } SAFE_FREE(tdb->transaction->blocks); - if (tdb->transaction->magic_offset) { - const struct tdb_methods *methods = tdb->transaction->io_methods; - uint32_t zero = 0; - - /* remove the recovery marker */ - if (methods->tdb_write(tdb, tdb->transaction->magic_offset, &zero, 4) == -1 || - transaction_sync(tdb, tdb->transaction->magic_offset, 4) == -1) { - TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_cancel: failed to remove recovery magic\n")); - ret = -1; + if (tdb->transaction->prepared) { + if (tdb->transaction->magic_offset) { + const struct tdb_methods *methods = tdb->transaction->io_methods; + uint32_t zero = 0; + + /* remove the recovery marker */ + if (methods->tdb_write(tdb, tdb->transaction->magic_offset, &zero, 4) == -1 || + transaction_sync(tdb, tdb->transaction->magic_offset, 4) == -1) { + TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_cancel: failed to remove recovery magic\n")); + ret = -1; + } } + + tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); } /* remove any global lock created during the transaction */ @@ -947,18 +951,17 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb) return -1; } + tdb->transaction->prepared = true; + if (!(tdb->flags & TDB_NOSYNC)) { /* write the recovery data to the end of the file */ if (transaction_setup_recovery(tdb, &tdb->transaction->magic_offset) == -1) { TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_prepare_commit: failed to setup recovery data\n")); - tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); _tdb_transaction_cancel(tdb); return -1; } } - tdb->transaction->prepared = true; - /* expand the file to the new size if needed */ if (tdb->map_size != tdb->transaction->old_map_size) { if (methods->tdb_expand_file(tdb, tdb->transaction->old_map_size, @@ -966,7 +969,6 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb) tdb->transaction->old_map_size) == -1) { tdb->ecode = TDB_ERR_IO; TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_prepare_commit: expansion failed\n")); - tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); _tdb_transaction_cancel(tdb); return -1; } @@ -1056,7 +1058,6 @@ int tdb_transaction_commit(struct tdb_context *tdb) tdb_transaction_recover(tdb); _tdb_transaction_cancel(tdb); - tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_commit: write failed\n")); return -1; @@ -1072,8 +1073,6 @@ int tdb_transaction_commit(struct tdb_context *tdb) return -1; } - tdb_brlock(tdb, GLOBAL_LOCK, F_UNLCK, F_SETLKW, 0, 1); - /* TODO: maybe write to some dummy hdr field, or write to magic offset without mmap, before the last sync, instead of the -- 1.6.5.7