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>

3 files changed, 91 insertions, 15 deletions

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 <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 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;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 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',