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 <rusty@rustcorp.com.au>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/lib/ntdb/test/api-60-noop-transaction.c b/lib/ntdb/test/api-60-noop-transaction.c
new file mode 100644 (file)
index 0000000..a429e67
--- /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 <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#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 9608be43e8ffd2e59f45f5b56f8f31d6b95e08a5..f276216632813aac0a9397e305ffe9950af7f2d9 100644 (file)
--- 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;i<ntdb->transaction->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;i<ntdb->transaction->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 1a4b02bb25496a1ee009a7c262c6d73f7a14465e..63c8402d661d41ab3b2b61036fdc567b12c74b62 100644 (file)
--- 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',