summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRusty Russell <rusty@rustcorp.com.au>2012-06-22 09:44:41 +0930
committerRusty Russell <rusty@rustcorp.com.au>2012-06-22 07:35:17 +0200
commit01ec4a72de56ade54bbbc92e0a408771390c5c12 (patch)
tree1ee7082cac28cee704ebc0eb6e76d372baa31405
parentbd5c061932d9aaf2e66cd56a39743c9ff34c3a88 (diff)
downloadsamba-01ec4a72de56ade54bbbc92e0a408771390c5c12.tar.gz
samba-01ec4a72de56ade54bbbc92e0a408771390c5c12.tar.bz2
samba-01ec4a72de56ade54bbbc92e0a408771390c5c12.zip
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>
-rw-r--r--lib/ntdb/doc/TDB_porting.txt91
-rw-r--r--lib/ntdb/lock.c11
-rw-r--r--lib/ntdb/ntdb.c13
-rw-r--r--lib/ntdb/ntdb.h9
-rw-r--r--lib/ntdb/test/api-95-read-only-during-parse.c94
-rw-r--r--lib/ntdb/wscript1
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',