ntdb: make database read-only during ntdb_parse() callback.

Since we have a readlock, any write will grab a write lock: if it happens
to be on the same bucket, we'll fail.

For that reason, enforce read-only so every write operation fails
(even for NTDB_NOLOCK or NTDB_INTERNAL dbs), and document it!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

6 files changed, 195 insertions, 24 deletions

diff --git a/lib/ntdb/doc/TDB_porting.txt b/lib/ntdb/doc/TDB_porting.txt
index 8b0ca2fec8..5daf94b74b 100644
--- a/lib/ntdb/doc/TDB_porting.txt
+++ b/lib/ntdb/doc/TDB_porting.txt
@@ -170,23 +170,6 @@ Interface differences between TDB and NTDB.
 		                   O_CREAT|O_RDWR, 0600, &hashsize);
 	}
 
-- ntdb does locking on read-only databases (ie. O_RDONLY passed to ntdb_open).
-  tdb did not: use the NTDB_NOLOCK flag if you want to suppress locking.
-
-  Example:
-	#include <tdb.h>
-	#include <ntdb.h>
-
-	struct tdb_context *tdb_example(void)
-	{
-		return tdb_open("example.tdb", 0, TDB_DEFAULT, O_RDONLY, 0);
-	}
-
-	struct ntdb_context *ntdb_example(void)
-	{
-		return ntdb_open("example.ntdb", NTDB_NOLOCK, O_RDONLY, NULL);
-	}
-
 - ntdb's log function is simpler than tdb's log function.  The string
   is already formatted, is not terminated by a '\n', and it takes an
   enum ntdb_log_level not a tdb_debug_level, and which has only three
@@ -304,6 +287,80 @@ Interface differences between TDB and NTDB.
 		}
 	}
 
+- ntdb's ntdb_parse_record() takes a type-checked callback data
+  pointer, not a void * (though a void * pointer still works).  The
+  callback function is allowed to do read operations on the database,
+  or write operations if you first call ntdb_lockall().  TDB's
+  tdb_parse_record() did not allow any database access within the
+  callback, could crash if you tried.
+
+  Example:
+	#include <tdb.h>
+	#include <ntdb.h>
+
+	static int tdb_parser(TDB_DATA key, TDB_DATA data, void *private_data)
+	{
+		TDB_DATA *expect = private_data;
+
+		return data.dsize == expect->dsize
+			&& !memcmp(data.dptr, expect->dptr, data.dsize);
+	}
+
+	void tdb_example(struct tdb_context *tdb, TDB_DATA key, NTDB_DATA d)
+	{
+		switch (tdb_parse_record(tdb, key, tdb_parser, &d)) {
+		case -1:
+			printf("parse failed: %s\n", tdb_errorstr(tdb));
+			break;
+		case 0:
+			printf("data was different!\n");
+			break;
+		case 1:
+			printf("data was same!\n");
+			break;
+		}
+	}
+
+	static int ntdb_parser(TDB_DATA key, TDB_DATA data, TDB_DATA *expect)
+	{
+		return ntdb_deq(data, *expect);
+	}
+
+	void ntdb_example(struct ntdb_context *ntdb, NTDB_DATA key, NTDB_DATA d)
+	{
+		enum NTDB_ERROR e;
+
+		e = tdb_parse_record(tdb, key, tdb_parser, &d);
+		switch (e) {
+		case 0:
+			printf("data was different!\n");
+			break;
+		case 1:
+			printf("data was same!\n");
+			break;
+		default:
+			printf("parse failed: %s\n", ntdb_errorstr(e));
+			break;
+		}
+	}
+
+- ntdb does locking on read-only databases (ie. O_RDONLY passed to ntdb_open).
+  tdb did not: use the NTDB_NOLOCK flag if you want to suppress locking.
+
+  Example:
+	#include <tdb.h>
+	#include <ntdb.h>
+
+	struct tdb_context *tdb_example(void)
+	{
+		return tdb_open("example.tdb", 0, TDB_DEFAULT, O_RDONLY, 0);
+	}
+
+	struct ntdb_context *ntdb_example(void)
+	{
+		return ntdb_open("example.ntdb", NTDB_NOLOCK, O_RDONLY, NULL);
+	}
+
 - Failure inside a transaction (such as a lock function failing) does
   not implicitly cancel the transaction; you still need to call
   ntdb_transaction_cancel().
diff --git a/lib/ntdb/lock.c b/lib/ntdb/lock.c
index f6a811a3fa..4517e25568 100644
--- a/lib/ntdb/lock.c
+++ b/lib/ntdb/lock.c
@@ -188,15 +188,15 @@ static enum NTDB_ERROR ntdb_brlock(struct ntdb_context *ntdb,
 {
 	int ret;
 
-	if (ntdb->flags & NTDB_NOLOCK) {
-		return NTDB_SUCCESS;
-	}
-
 	if (rw_type == F_WRLCK && (ntdb->flags & NTDB_RDONLY)) {
 		return ntdb_logerr(ntdb, NTDB_ERR_RDONLY, NTDB_LOG_USE_ERROR,
 				  "Write lock attempted on read-only database");
 	}
 
+	if (ntdb->flags & NTDB_NOLOCK) {
+		return NTDB_SUCCESS;
+	}
+
 	/* A 32 bit system cannot open a 64-bit file, but it could have
 	 * expanded since then: check here. */
 	if ((size_t)(offset + len) != offset + len) {
@@ -533,8 +533,9 @@ enum NTDB_ERROR ntdb_allrecord_lock(struct ntdb_context *ntdb, int ltype,
 	enum NTDB_ERROR ecode;
 	ntdb_bool_err berr;
 
-	if (ntdb->flags & NTDB_NOLOCK)
+	if (ntdb->flags & NTDB_NOLOCK) {
 		return NTDB_SUCCESS;
+	}
 
 	if (!check_lock_pid(ntdb, "ntdb_allrecord_lock", true)) {
 		return NTDB_ERR_LOCK;
diff --git a/lib/ntdb/ntdb.c b/lib/ntdb/ntdb.c
index 6c75915899..5d56b33b5a 100644
--- a/lib/ntdb/ntdb.c
+++ b/lib/ntdb/ntdb.c
@@ -500,10 +500,23 @@ _PUBLIC_ enum NTDB_ERROR ntdb_parse_record_(struct ntdb_context *ntdb,
 	if (!off) {
 		ecode = NTDB_ERR_NOEXIST;
 	} else {
+		unsigned int old_flags;
 		NTDB_DATA d = ntdb_mkdata(keyp + key.dsize,
 					  rec_data_length(&rec));
 
+		/*
+		 * Make sure they don't try to write db, since they
+		 * have read lock!  They can if they've done
+		 * ntdb_lockall(): if it was ntdb_lockall_read, that'll
+		 * stop them doing a write operation anyway.
+		 */
+		old_flags = ntdb->flags;
+		if (!ntdb->file->allrecord_lock.count &&
+		    !(ntdb->flags & NTDB_NOLOCK)) {
+			ntdb->flags |= NTDB_RDONLY;
+		}
 		ecode = parse(key, d, data);
+		ntdb->flags = old_flags;
 		ntdb_access_release(ntdb, keyp);
 	}
 
diff --git a/lib/ntdb/ntdb.h b/lib/ntdb/ntdb.h
index 8e8458e4d1..df3a9ddc4e 100644
--- a/lib/ntdb/ntdb.h
+++ b/lib/ntdb/ntdb.h
@@ -368,9 +368,14 @@ int64_t ntdb_traverse_(struct ntdb_context *ntdb,
  *
  * This avoids a copy for many cases, by handing you a pointer into
  * the memory-mapped database.  It also locks the record to prevent
- * other accesses at the same time.
+ * other accesses at the same time, so it won't change.
  *
- * Do not alter the data handed to parse()!
+ * Within the @parse callback you can perform read operations on the
+ * database, but no write operations: no ntdb_store() or
+ * ntdb_delete(), for example.  The exception is if you call
+ * ntdb_lockall() before ntdb_parse_record().
+ *
+ * Never alter the data handed to parse()!
  */
 #define ntdb_parse_record(ntdb, key, parse, data)			\
 	ntdb_parse_record_((ntdb), (key),				\
diff --git a/lib/ntdb/test/api-95-read-only-during-parse.c b/lib/ntdb/test/api-95-read-only-during-parse.c
new file mode 100644
index 0000000000..8252b812d2
--- /dev/null
+++ b/lib/ntdb/test/api-95-read-only-during-parse.c
@@ -0,0 +1,94 @@
+/* Make sure write operations fail during ntdb_parse(). */
+#include "config.h"
+#include "ntdb.h"
+#include "tap-interface.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include "logging.h"
+
+static struct ntdb_context *ntdb;
+
+/* We could get either of these. */
+static bool xfail(enum NTDB_ERROR ecode)
+{
+	return ecode == NTDB_ERR_RDONLY || ecode == NTDB_ERR_LOCK;
+}
+
+static enum NTDB_ERROR parse(NTDB_DATA key, NTDB_DATA data,
+			     NTDB_DATA *expected)
+{
+	NTDB_DATA add = ntdb_mkdata("another", strlen("another"));
+
+	if (!ntdb_deq(data, *expected)) {
+		return NTDB_ERR_EINVAL;
+	}
+
+	/* These should all fail.*/
+	if (!xfail(ntdb_store(ntdb, add, add, NTDB_INSERT))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_append(ntdb, key, add))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_delete(ntdb, key))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_transaction_start(ntdb))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_chainlock(ntdb, key))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_lockall(ntdb))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_wipe_all(ntdb))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	if (!xfail(ntdb_repack(ntdb))) {
+		return NTDB_ERR_EINVAL;
+	}
+	tap_log_messages--;
+
+	/* Access the record one more time. */
+	if (!ntdb_deq(data, *expected)) {
+		return NTDB_ERR_EINVAL;
+	}
+
+	return NTDB_SUCCESS;
+}
+
+int main(int argc, char *argv[])
+{
+	unsigned int i;
+	int flags[] = { NTDB_DEFAULT, NTDB_NOMMAP, NTDB_CONVERT };
+	NTDB_DATA key = ntdb_mkdata("hello", 5), data = ntdb_mkdata("world", 5);
+
+	plan_tests(sizeof(flags) / sizeof(flags[0]) * 2 + 1);
+	for (i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) {
+		ntdb = ntdb_open("api-95-read-only-during-parse.ntdb",
+				 flags[i]|MAYBE_NOSYNC,
+				 O_RDWR|O_CREAT|O_TRUNC, 0600, &tap_log_attr);
+		ok1(ntdb_store(ntdb, key, data, NTDB_INSERT) == NTDB_SUCCESS);
+		ok1(ntdb_parse_record(ntdb, key, parse, &data) == NTDB_SUCCESS);
+		ntdb_close(ntdb);
+	}
+
+	ok1(tap_log_messages == 0);
+	return exit_status();
+}
diff --git a/lib/ntdb/wscript b/lib/ntdb/wscript
index f034631058..e211a9fc0c 100644
--- a/lib/ntdb/wscript
+++ b/lib/ntdb/wscript
@@ -79,6 +79,7 @@ def configure(conf):
                                 'test/api-91-get-stats.c',
                                 'test/api-92-get-set-readonly.c',
                                 'test/api-93-repack.c',
+                                'test/api-95-read-only-during-parse.c',
                                 'test/api-add-remove-flags.c',
                                 'test/api-check-callback.c',
                                 'test/api-firstkey-nextkey.c',