[PATCH] Fix a tdb glitch
Jeremy Allison
jra at samba.org
Mon Apr 10 18:38:23 UTC 2017
On Mon, Apr 10, 2017 at 08:12:56AM +0200, vl--- via samba-technical wrote:
> Hi!
>
> Attached find fix for a small locking bug in tdb.
>
> Review appreciated!
Nice catch ! RB+. Pushed.
> From e614f312f1a020ab3bc515799974b0ce6f7a04ab Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 Nov 2016 21:38:58 +0100
> Subject: [PATCH 1/3] tdb: Fix some signed/unsigned hickups
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> lib/tdb/common/lock.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/lib/tdb/common/lock.c b/lib/tdb/common/lock.c
> index 195dbb5..594c287 100644
> --- a/lib/tdb/common/lock.c
> +++ b/lib/tdb/common/lock.c
> @@ -294,7 +294,7 @@ fail:
> static struct tdb_lock_type *find_nestlock(struct tdb_context *tdb,
> tdb_off_t offset)
> {
> - unsigned int i;
> + int i;
>
> for (i=0; i<tdb->num_lockrecs; i++) {
> if (tdb->lockrecs[i].off == offset) {
> @@ -381,7 +381,7 @@ static int tdb_lock_and_recover(struct tdb_context *tdb)
>
> static bool have_data_locks(const struct tdb_context *tdb)
> {
> - unsigned int i;
> + int i;
>
> for (i = 0; i < tdb->num_lockrecs; i++) {
> if (tdb->lockrecs[i].off >= lock_offset(-1))
> @@ -560,7 +560,8 @@ static int tdb_allrecord_check(struct tdb_context *tdb, int ltype,
> return -1;
> }
>
> - if (tdb->allrecord_lock.count && tdb->allrecord_lock.ltype == ltype) {
> + if (tdb->allrecord_lock.count &&
> + tdb->allrecord_lock.ltype == (uint32_t)ltype) {
> tdb->allrecord_lock.count++;
> return 0;
> }
> @@ -706,7 +707,7 @@ int tdb_allrecord_unlock(struct tdb_context *tdb, int ltype, bool mark_lock)
> }
>
> /* Upgradable locks are marked as write locks. */
> - if (tdb->allrecord_lock.ltype != ltype
> + if (tdb->allrecord_lock.ltype != (uint32_t)ltype
> && (!tdb->allrecord_lock.off || ltype != F_RDLCK)) {
> tdb->ecode = TDB_ERR_LOCK;
> return -1;
> @@ -945,7 +946,8 @@ bool tdb_have_extra_locks(struct tdb_context *tdb)
> /* The transaction code uses this to remove all locks. */
> void tdb_release_transaction_locks(struct tdb_context *tdb)
> {
> - unsigned int i, active = 0;
> + int i;
> + unsigned int active = 0;
>
> if (tdb->allrecord_lock.count != 0) {
> tdb_allrecord_unlock(tdb, tdb->allrecord_lock.ltype, false);
> --
> 2.1.4
>
>
> From b579c097f1d76ed1cd40f02e11bfff4c187d78d1 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 Nov 2016 21:40:15 +0100
> Subject: [PATCH 2/3] tdb: Do lock upgrades properly
>
> When a process holds a readlock and wants to upgrade, this needs to be
> reflected in the underlying lock. Without this, it is possible to cheat:
> One process holds a readlock, and another process wants to write this
> record. All the writer has to do is take a readonly lock on the key and
> then do the store.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> lib/tdb/common/lock.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/lib/tdb/common/lock.c b/lib/tdb/common/lock.c
> index 594c287..4ad70cf 100644
> --- a/lib/tdb/common/lock.c
> +++ b/lib/tdb/common/lock.c
> @@ -321,6 +321,22 @@ int tdb_nest_lock(struct tdb_context *tdb, uint32_t offset, int ltype,
>
> new_lck = find_nestlock(tdb, offset);
> if (new_lck) {
> + if ((new_lck->ltype == F_RDLCK) && (ltype == F_WRLCK)) {
> + if (!tdb_have_mutexes(tdb)) {
> + int ret;
> + /*
> + * Upgrade the underlying fcntl
> + * lock. Mutexes don't do readlocks,
> + * so this only applies to fcntl
> + * locking.
> + */
> + ret = tdb_brlock(tdb, ltype, offset, 1, flags);
> + if (ret != 0) {
> + return ret;
> + }
> + }
> + new_lck->ltype = F_WRLCK;
> + }
> /*
> * Just increment the in-memory struct, posix locks
> * don't stack.
> --
> 2.1.4
>
>
> From 0cf2ee29f5c4c13e66e999fd85f6b22aa68ad9d5 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 8 Nov 2016 17:01:56 +0100
> Subject: [PATCH 3/3] tdb: Test for readonly lock upgrade bug
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> lib/tdb/test/run-rdlock-upgrade.c | 166 ++++++++++++++++++++++++++++++++++++++
> lib/tdb/wscript | 1 +
> 2 files changed, 167 insertions(+)
> create mode 100644 lib/tdb/test/run-rdlock-upgrade.c
>
> diff --git a/lib/tdb/test/run-rdlock-upgrade.c b/lib/tdb/test/run-rdlock-upgrade.c
> new file mode 100644
> index 0000000..042001b
> --- /dev/null
> +++ b/lib/tdb/test/run-rdlock-upgrade.c
> @@ -0,0 +1,166 @@
> +#include "../common/io.c"
> +#include "../common/tdb.c"
> +#include "../common/lock.c"
> +#include "../common/freelist.c"
> +#include "../common/traverse.c"
> +#include "../common/transaction.c"
> +#include "../common/error.c"
> +#include "../common/open.c"
> +#include "../common/check.c"
> +#include "../common/hash.c"
> +#include "../common/mutex.c"
> +#include "tap-interface.h"
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <stdarg.h>
> +#include "logging.h"
> +
> +static TDB_DATA key, data;
> +
> +static void do_chainlock(const char *name, int tdb_flags, int up, int down)
> +{
> + struct tdb_context *tdb;
> + int ret;
> + ssize_t nread, nwritten;
> + char c = 0;
> +
> + tdb = tdb_open_ex(name, 3, tdb_flags,
> + O_RDWR|O_CREAT, 0755, &taplogctx, NULL);
> + ok(tdb, "tdb_open_ex should succeed");
> +
> + ret = tdb_chainlock_read(tdb, key);
> + ok(ret == 0, "tdb_chainlock_read should succeed");
> +
> + nwritten = write(up, &c, sizeof(c));
> + ok(nwritten == sizeof(c), "write should succeed");
> +
> + nread = read(down, &c, sizeof(c));
> + ok(nread == 0, "read should succeed");
> +
> + exit(0);
> +}
> +
> +static void do_trylock(const char *name, int tdb_flags, int up, int down)
> +{
> + struct tdb_context *tdb;
> + int ret;
> + ssize_t nread, nwritten;
> + char c = 0;
> +
> + tdb = tdb_open_ex(name, 3, tdb_flags,
> + O_RDWR|O_CREAT, 0755, &taplogctx, NULL);
> + ok(tdb, "tdb_open_ex should succeed");
> +
> + /*
> + * tdb used to have a bug where with fcntl locks an upgrade
> + * from a readlock to writelock did not check for the
> + * underlying fcntl lock. Mutexes don't distinguish between
> + * readlocks and writelocks, so that bug does not apply here.
> + */
> +
> + ret = tdb_chainlock_read(tdb, key);
> + ok(ret == 0, "tdb_chainlock_read should succeed");
> +
> + ret = tdb_chainlock_nonblock(tdb, key);
> + ok(ret == -1, "tdb_chainlock_nonblock should fail");
> +
> + nwritten = write(up, &c, sizeof(c));
> + ok(nwritten == sizeof(c), "write should succeed");
> +
> + nread = read(down, &c, sizeof(c));
> + ok(nread == 0, "read should succeed");
> +
> + exit(0);
> +}
> +
> +static int do_tests(const char *name, int tdb_flags)
> +{
> + int ret;
> + pid_t chainlock_child, store_child;
> + int chainlock_down[2];
> + int chainlock_up[2];
> + int store_down[2];
> + int store_up[2];
> + char c;
> + ssize_t nread;
> +
> + key.dsize = strlen("hi");
> + key.dptr = discard_const_p(uint8_t, "hi");
> + data.dsize = strlen("world");
> + data.dptr = discard_const_p(uint8_t, "world");
> +
> + ret = pipe(chainlock_down);
> + ok(ret == 0, "pipe should succeed");
> +
> + ret = pipe(chainlock_up);
> + ok(ret == 0, "pipe should succeed");
> +
> + ret = pipe(store_down);
> + ok(ret == 0, "pipe should succeed");
> +
> + ret = pipe(store_up);
> + ok(ret == 0, "pipe should succeed");
> +
> + chainlock_child = fork();
> + ok(chainlock_child != -1, "fork should succeed");
> +
> + if (chainlock_child == 0) {
> + close(chainlock_up[0]);
> + close(chainlock_down[1]);
> + close(store_up[0]);
> + close(store_up[1]);
> + close(store_down[0]);
> + close(store_down[1]);
> + do_chainlock(name, tdb_flags,
> + chainlock_up[1], chainlock_down[0]);
> + exit(0);
> + }
> + close(chainlock_up[1]);
> + close(chainlock_down[0]);
> +
> + nread = read(chainlock_up[0], &c, sizeof(c));
> + ok(nread == sizeof(c), "read should succeed");
> +
> + /*
> + * Now we have a process holding a chain read lock. Start
> + * another process trying to write lock. This should fail.
> + */
> +
> + store_child = fork();
> + ok(store_child != -1, "fork should succeed");
> +
> + if (store_child == 0) {
> + close(chainlock_up[0]);
> + close(chainlock_down[1]);
> + close(store_up[0]);
> + close(store_down[1]);
> + do_trylock(name, tdb_flags,
> + store_up[1], store_down[0]);
> + exit(0);
> + }
> + close(store_up[1]);
> + close(store_down[0]);
> +
> + nread = read(store_up[0], &c, sizeof(c));
> + ok(nread == sizeof(c), "read should succeed");
> +
> + close(chainlock_up[0]);
> + close(chainlock_down[1]);
> + close(store_up[0]);
> + close(store_down[1]);
> + diag("%s tests done", name);
> + return exit_status();
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + int ret;
> +
> + ret = do_tests("rdlock-upgrade.tdb",
> + TDB_CLEAR_IF_FIRST |
> + TDB_INCOMPATIBLE_HASH);
> + ok(ret == 0, "rdlock-upgrade.tdb tests should succeed");
> +
> + return exit_status();
> +}
> diff --git a/lib/tdb/wscript b/lib/tdb/wscript
> index 0d682eb..693787c 100644
> --- a/lib/tdb/wscript
> +++ b/lib/tdb/wscript
> @@ -34,6 +34,7 @@ tdb1_unit_tests = [
> 'run-readonly-check',
> 'run-rescue',
> 'run-rescue-find_entry',
> + 'run-rdlock-upgrade',
> 'run-rwlock-check',
> 'run-summary',
> 'run-transaction-expand',
> --
> 2.1.4
>
More information about the samba-technical
mailing list