Commit 176844c8 authored by Rusty Russell's avatar Rusty Russell

Import 898b5edfe757cb145960b8f3631029bfd5592119 from ctdb:

Author: Volker Lendecke <vl@samba.org>  2010-01-30 03:51:09

    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.
parent 9d045ca0
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include <errno.h> #include <errno.h>
#include <ccan/tdb/tdb.h> #include <ccan/tdb/tdb.h>
#include <ccan/tap/tap.h> #include <ccan/tap/tap.h>
#include <stdio.h>
static volatile sig_atomic_t alarmed; static volatile sig_atomic_t alarmed;
static void do_alarm(int signum) static void do_alarm(int signum)
......
...@@ -135,6 +135,9 @@ struct tdb_transaction { ...@@ -135,6 +135,9 @@ struct tdb_transaction {
bool prepared; bool prepared;
tdb_off_t magic_offset; tdb_off_t magic_offset;
/* set when the GLOBAL_LOCK has been taken */
bool global_lock_taken;
/* old file size before transaction */ /* old file size before transaction */
tdb_len_t old_map_size; tdb_len_t old_map_size;
...@@ -501,6 +504,11 @@ int _tdb_transaction_cancel(struct tdb_context *tdb, int ltype) ...@@ -501,6 +504,11 @@ int _tdb_transaction_cancel(struct tdb_context *tdb, int ltype)
} }
} }
if (tdb->transaction->global_lock_taken) {
tdb_brunlock(tdb, F_WRLCK, GLOBAL_LOCK, 1);
tdb->transaction->global_lock_taken = false;
}
/* remove any global lock created during the transaction */ /* remove any global lock created during the transaction */
if (tdb->global_lock.count != 0) { if (tdb->global_lock.count != 0) {
tdb_brunlock(tdb, tdb->global_lock.ltype, tdb_brunlock(tdb, tdb->global_lock.ltype,
...@@ -960,11 +968,12 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb) ...@@ -960,11 +968,12 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb)
return -1; return -1;
} }
tdb->transaction->global_lock_taken = true;
if (!(tdb->flags & TDB_NOSYNC)) { if (!(tdb->flags & TDB_NOSYNC)) {
/* write the recovery data to the end of the file */ /* write the recovery data to the end of the file */
if (transaction_setup_recovery(tdb, &tdb->transaction->magic_offset) == -1) { 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_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_prepare_commit: failed to setup recovery data\n"));
tdb_brunlock(tdb, F_WRLCK, GLOBAL_LOCK, 1);
_tdb_transaction_cancel(tdb, F_WRLCK); _tdb_transaction_cancel(tdb, F_WRLCK);
return -1; return -1;
} }
...@@ -979,7 +988,6 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb) ...@@ -979,7 +988,6 @@ static int _tdb_transaction_prepare_commit(struct tdb_context *tdb)
tdb->transaction->old_map_size) == -1) { tdb->transaction->old_map_size) == -1) {
tdb->ecode = TDB_ERR_IO; tdb->ecode = TDB_ERR_IO;
TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_prepare_commit: expansion failed\n")); TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_prepare_commit: expansion failed\n"));
tdb_brunlock(tdb, F_WRLCK, GLOBAL_LOCK, 1);
_tdb_transaction_cancel(tdb, F_WRLCK); _tdb_transaction_cancel(tdb, F_WRLCK);
return -1; return -1;
} }
...@@ -1069,7 +1077,6 @@ int tdb_transaction_commit(struct tdb_context *tdb) ...@@ -1069,7 +1077,6 @@ int tdb_transaction_commit(struct tdb_context *tdb)
tdb_transaction_recover(tdb); tdb_transaction_recover(tdb);
_tdb_transaction_cancel(tdb, F_WRLCK); _tdb_transaction_cancel(tdb, F_WRLCK);
tdb_brunlock(tdb, F_WRLCK, GLOBAL_LOCK, 1);
TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_commit: write failed\n")); TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_transaction_commit: write failed\n"));
return -1; return -1;
...@@ -1085,8 +1092,6 @@ int tdb_transaction_commit(struct tdb_context *tdb) ...@@ -1085,8 +1092,6 @@ int tdb_transaction_commit(struct tdb_context *tdb)
return -1; return -1;
} }
tdb_brunlock(tdb, F_WRLCK, GLOBAL_LOCK, 1);
/* /*
TODO: maybe write to some dummy hdr field, or write to magic TODO: maybe write to some dummy hdr field, or write to magic
offset without mmap, before the last sync, instead of the offset without mmap, before the last sync, instead of the
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment