From: Rusty Russell Date: Fri, 22 Jun 2012 00:14:41 +0000 (+0930) Subject: ntdb: make database read-only during ntdb_parse() callback. X-Git-Tag: samba-4.0.0beta3~259 X-Git-Url: http://git.samba.org/?p=ddiss%2Fsamba.git;a=commitdiff_plain;h=01ec4a72de56ade54bbbc92e0a408771390c5c12 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 --- diff --git a/lib/ntdb/doc/TDB_porting.txt b/lib/ntdb/doc/TDB_porting.txt index 8b0ca2fec81..5daf94b74b9 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 - #include - - 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 + #include + + 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 + #include + + 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 f6a811a3fa1..4517e255685 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 6c75915899b..5d56b33b5a1 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 8e8458e4d17..df3a9ddc4ea 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 00000000000..8252b812d22 --- /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 +#include +#include +#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 f034631058d..e211a9fc0c7 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',