From 3d82f786ecdd6747e90fe480a15de8c3fcea5f7b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 17 Feb 2013 20:56:34 +1030 Subject: ntdb: fix database corruption when transaction doesn't change anything. ntdb's transaction code has an optimization which tdb's doesnt: it only writes the parts of blocks whose contents have changed. This means we can actually have a transaction which turns out to need no recovery region. This breaks the recovery setup logic, which sets the current recovery size to 0 if there's no recovery area, and assumes that we'll always create a new recovery area since the recovery will always need > 0 bytes. In fact, if we really haven't changed anything, we can skip the transaction commit altogether: since this happens at least once with Samba, it's worth doing. Signed-off-by: Rusty Russell Reviewed-by: Andrew Bartlett --- lib/ntdb/test/api-60-noop-transaction.c | 58 +++++++++++++++++++++++++++++++++ lib/ntdb/transaction.c | 47 +++++++++++++++++--------- lib/ntdb/wscript | 1 + 3 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 lib/ntdb/test/api-60-noop-transaction.c (limited to 'lib/ntdb') diff --git a/lib/ntdb/test/api-60-noop-transaction.c b/lib/ntdb/test/api-60-noop-transaction.c new file mode 100644 index 0000000000..a429e67907 --- /dev/null +++ b/lib/ntdb/test/api-60-noop-transaction.c @@ -0,0 +1,58 @@ +#include "private.h" // struct ntdb_context +#include "ntdb.h" +#include "tap-interface.h" +#include +#include +#include +#include +#include "logging.h" + +int main(int argc, char *argv[]) +{ + unsigned int i; + struct ntdb_context *ntdb; + int flags[] = { NTDB_DEFAULT, NTDB_NOMMAP, + NTDB_CONVERT, NTDB_NOMMAP|NTDB_CONVERT }; + NTDB_DATA key = ntdb_mkdata("key", 3); + NTDB_DATA data = ntdb_mkdata("data", 4), d; + + plan_tests(sizeof(flags) / sizeof(flags[0]) * 12 + 1); + + for (i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) { + ntdb = ntdb_open("api-60-transaction.ntdb", + flags[i]|MAYBE_NOSYNC, + O_RDWR|O_CREAT|O_TRUNC, 0600, &tap_log_attr); + ok1(ntdb); + if (!ntdb) + continue; + + ok1(ntdb_store(ntdb, key, data, NTDB_INSERT) == 0); + + ok1(ntdb_transaction_start(ntdb) == 0); + /* Do an identical replace. */ + ok1(ntdb_store(ntdb, key, data, NTDB_REPLACE) == 0); + ok1(ntdb_transaction_commit(ntdb) == 0); + + ok1(ntdb_check(ntdb, NULL, NULL) == 0); + ok1(ntdb_fetch(ntdb, key, &d) == NTDB_SUCCESS); + ok1(ntdb_deq(data, d)); + free(d.dptr); + ntdb_close(ntdb); + + /* Reopen, fetch. */ + ntdb = ntdb_open("api-60-transaction.ntdb", + flags[i]|MAYBE_NOSYNC, + O_RDWR, 0600, &tap_log_attr); + ok1(ntdb); + if (!ntdb) + continue; + ok1(ntdb_check(ntdb, NULL, NULL) == 0); + ok1(ntdb_fetch(ntdb, key, &d) == NTDB_SUCCESS); + ok1(ntdb_deq(data, d)); + free(d.dptr); + ntdb_close(ntdb); + } + + ok1(tap_log_messages == 0); + return exit_status(); +} diff --git a/lib/ntdb/transaction.c b/lib/ntdb/transaction.c index 9608be43e8..f276216632 100644 --- a/lib/ntdb/transaction.c +++ b/lib/ntdb/transaction.c @@ -453,10 +453,23 @@ static enum NTDB_ERROR transaction_sync(struct ntdb_context *ntdb, return NTDB_SUCCESS; } +static void free_transaction_blocks(struct ntdb_context *ntdb) +{ + int i; + + /* free all the transaction blocks */ + for (i=0;itransaction->num_blocks;i++) { + if (ntdb->transaction->blocks[i] != NULL) { + ntdb->free_fn(ntdb->transaction->blocks[i], + ntdb->alloc_data); + } + } + SAFE_FREE(ntdb, ntdb->transaction->blocks); + ntdb->transaction->num_blocks = 0; +} static void _ntdb_transaction_cancel(struct ntdb_context *ntdb) { - int i; enum NTDB_ERROR ecode; if (ntdb->transaction == NULL) { @@ -473,14 +486,7 @@ static void _ntdb_transaction_cancel(struct ntdb_context *ntdb) ntdb->file->map_size = ntdb->transaction->old_map_size; - /* free all the transaction blocks */ - for (i=0;itransaction->num_blocks;i++) { - if (ntdb->transaction->blocks[i] != NULL) { - ntdb->free_fn(ntdb->transaction->blocks[i], - ntdb->alloc_data); - } - } - SAFE_FREE(ntdb, ntdb->transaction->blocks); + free_transaction_blocks(ntdb); if (ntdb->transaction->magic_offset) { const struct ntdb_methods *methods = ntdb->transaction->io_methods; @@ -875,6 +881,17 @@ static enum NTDB_ERROR transaction_setup_recovery(struct ntdb_context *ntdb) if (NTDB_PTR_IS_ERR(recovery)) return NTDB_PTR_ERR(recovery); + /* If we didn't actually change anything we overwrote? */ + if (recovery_size == 0) { + /* In theory, we could have just appended data. */ + if (ntdb->transaction->num_blocks * NTDB_PGSIZE + < ntdb->transaction->old_map_size) { + free_transaction_blocks(ntdb); + } + ntdb->free_fn(recovery, ntdb->alloc_data); + return NTDB_SUCCESS; + } + ecode = ntdb_recovery_area(ntdb, methods, &recovery_off, recovery); if (ecode) { ntdb->free_fn(recovery, ntdb->alloc_data); @@ -1064,12 +1081,6 @@ _PUBLIC_ enum NTDB_ERROR ntdb_transaction_commit(struct ntdb_context *ntdb) return NTDB_SUCCESS; } - /* check for a null transaction */ - if (ntdb->transaction->blocks == NULL) { - _ntdb_transaction_cancel(ntdb); - return NTDB_SUCCESS; - } - if (!ntdb->transaction->prepared) { ecode = _ntdb_transaction_prepare_commit(ntdb); if (ecode != NTDB_SUCCESS) { @@ -1078,6 +1089,12 @@ _PUBLIC_ enum NTDB_ERROR ntdb_transaction_commit(struct ntdb_context *ntdb) } } + /* check for a null transaction (prepare_commit may do this!) */ + if (ntdb->transaction->blocks == NULL) { + _ntdb_transaction_cancel(ntdb); + return NTDB_SUCCESS; + } + methods = ntdb->transaction->io_methods; /* perform all the writes */ diff --git a/lib/ntdb/wscript b/lib/ntdb/wscript index 1a4b02bb25..63c8402d66 100644 --- a/lib/ntdb/wscript +++ b/lib/ntdb/wscript @@ -72,6 +72,7 @@ def configure(conf): 'test/api-20-alloc-attr.c', 'test/api-21-parse_record.c', 'test/api-55-transaction.c', + 'test/api-60-noop-transaction.c', 'test/api-80-tdb_fd.c', 'test/api-81-seqnum.c', 'test/api-82-lockattr.c', -- cgit