[Patches] g_lock conflict detection broken when processing stale entries (bug #)
Stefan Metzmacher
metze at samba.org
Wed Dec 20 09:15:53 UTC 2017
Hi,
here're patches for https://bugzilla.samba.org/show_bug.cgi?id=13195
Please review and push:-)
Thanks!
metze
-------------- next part --------------
From 8fc9defd720e82fd20f7a6421296e900ff5f1476 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 20 Dec 2017 09:44:40 +0100
Subject: [PATCH 1/2] torture3: add LOCAL-G-LOCK6 test
This is a regression test for bug #13195.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13195
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
selftest/knownfail.d/local-g-lock6 | 1 +
source3/selftest/tests.py | 1 +
source3/torture/proto.h | 1 +
source3/torture/test_g_lock.c | 147 +++++++++++++++++++++++++++++++++++++
source3/torture/torture.c | 1 +
5 files changed, 151 insertions(+)
create mode 100644 selftest/knownfail.d/local-g-lock6
diff --git a/selftest/knownfail.d/local-g-lock6 b/selftest/knownfail.d/local-g-lock6
new file mode 100644
index 0000000..14fd5c8
--- /dev/null
+++ b/selftest/knownfail.d/local-g-lock6
@@ -0,0 +1 @@
+^samba3.smbtorture_s3.LOCAL-G-LOCK6.smbtorture
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index f8d2a4d..5b8ab79 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -158,6 +158,7 @@ local_tests = [
"LOCAL-G-LOCK3",
"LOCAL-G-LOCK4",
"LOCAL-G-LOCK5",
+ "LOCAL-G-LOCK6",
"LOCAL-NAMEMAP-CACHE1",
"LOCAL-hex_encode_buf",
"LOCAL-remove_duplicate_addrs2"]
diff --git a/source3/torture/proto.h b/source3/torture/proto.h
index 12c76a7..7ab2fe1 100644
--- a/source3/torture/proto.h
+++ b/source3/torture/proto.h
@@ -132,6 +132,7 @@ bool run_g_lock2(int dummy);
bool run_g_lock3(int dummy);
bool run_g_lock4(int dummy);
bool run_g_lock5(int dummy);
+bool run_g_lock6(int dummy);
bool run_g_lock_ping_pong(int dummy);
bool run_local_namemap_cache1(int dummy);
diff --git a/source3/torture/test_g_lock.c b/source3/torture/test_g_lock.c
index 253d20c..7d11471 100644
--- a/source3/torture/test_g_lock.c
+++ b/source3/torture/test_g_lock.c
@@ -695,6 +695,153 @@ bool run_g_lock5(int dummy)
return true;
}
+struct lock6_parser_state {
+ size_t num_locks;
+};
+
+static void lock6_parser(const struct g_lock_rec *locks,
+ size_t num_locks,
+ const uint8_t *data,
+ size_t datalen,
+ void *private_data)
+{
+ struct lock6_parser_state *state = private_data;
+ state->num_locks = num_locks;
+}
+
+/*
+ * Test cleanup with contention and stale locks
+ */
+
+bool run_g_lock6(int dummy)
+{
+ struct tevent_context *ev = NULL;
+ struct messaging_context *msg = NULL;
+ struct g_lock_ctx *ctx = NULL;
+ const char *lockname = "lock6";
+ pid_t child;
+ int exit_pipe[2], ready_pipe[2];
+ NTSTATUS status;
+ size_t i, nprocs;
+ int ret;
+ bool ok;
+ ssize_t nread;
+ char c;
+
+ if ((pipe(exit_pipe) != 0) || (pipe(ready_pipe) != 0)) {
+ perror("pipe failed");
+ return false;
+ }
+
+ ok = get_g_lock_ctx(talloc_tos(), &ev, &msg, &ctx);
+ if (!ok) {
+ fprintf(stderr, "get_g_lock_ctx failed");
+ return false;
+ }
+
+ nprocs = 2;
+ for (i=0; i<nprocs; i++) {
+
+ child = fork();
+
+ if (child == -1) {
+ perror("fork failed");
+ return false;
+ }
+
+ if (child == 0) {
+ TALLOC_FREE(ctx);
+
+ status = reinit_after_fork(msg, ev, false, "");
+
+ close(ready_pipe[0]);
+ close(exit_pipe[1]);
+
+ ok = get_g_lock_ctx(talloc_tos(), &ev, &msg, &ctx);
+ if (!ok) {
+ fprintf(stderr, "get_g_lock_ctx failed");
+ exit(1);
+ }
+ status = g_lock_lock(ctx, lockname, G_LOCK_READ,
+ (struct timeval) { .tv_sec = 1 });
+ if (!NT_STATUS_IS_OK(status)) {
+ fprintf(stderr,
+ "child g_lock_lock failed %s\n",
+ nt_errstr(status));
+ exit(1);
+ }
+ if (i == 0) {
+ exit(0);
+ }
+ close(ready_pipe[1]);
+ nread = sys_read(exit_pipe[0], &c, sizeof(c));
+ if (nread != 0) {
+ fprintf(stderr, "sys_read returned %zu (%s)\n",
+ nread, strerror(errno));
+ exit(1);
+ }
+ exit(0);
+ }
+ }
+
+ close(ready_pipe[1]);
+
+ nread = sys_read(ready_pipe[0], &c, sizeof(c));
+ if (nread != 0) {
+ fprintf(stderr, "sys_read returned %zu (%s)\n",
+ nread, strerror(errno));
+ return false;
+ }
+
+ {
+ int child_status;
+ ret = waitpid(-1, &child_status, 0);
+ if (ret == -1) {
+ perror("waitpid failed");
+ return false;
+ }
+ }
+
+ {
+ struct lock6_parser_state state;
+
+ status = g_lock_dump(ctx, lockname, lock6_parser, &state);
+ if (!NT_STATUS_IS_OK(status)) {
+ fprintf(stderr, "g_lock_dump returned %s\n",
+ nt_errstr(status));
+ return false;
+ }
+
+ if (state.num_locks != nprocs) {
+ fprintf(stderr, "nlocks=%zu, expected %zu\n",
+ state.num_locks, nprocs);
+ return false;
+ }
+
+ status = g_lock_lock(ctx, lockname, G_LOCK_WRITE,
+ (struct timeval) { .tv_sec = 1 });
+ if (!NT_STATUS_EQUAL(status, NT_STATUS_IO_TIMEOUT)) {
+ fprintf(stderr, "g_lock_lock should have failed with %s - %s\n",
+ nt_errstr(NT_STATUS_IO_TIMEOUT),
+ nt_errstr(status));
+ return false;
+ }
+ }
+
+ close(exit_pipe[1]);
+
+ {
+ int child_status;
+ ret = waitpid(-1, &child_status, 0);
+ if (ret == -1) {
+ perror("waitpid failed");
+ return false;
+ }
+ }
+
+ return true;
+}
+
extern int torture_numops;
extern int torture_nprocs;
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index 232e8ff..1a9cf5b 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -11585,6 +11585,7 @@ static struct {
{ "LOCAL-G-LOCK3", run_g_lock3, 0 },
{ "LOCAL-G-LOCK4", run_g_lock4, 0 },
{ "LOCAL-G-LOCK5", run_g_lock5, 0 },
+ { "LOCAL-G-LOCK6", run_g_lock6, 0 },
{ "LOCAL-G-LOCK-PING-PONG", run_g_lock_ping_pong, 0 },
{ "LOCAL-CANONICALIZE-PATH", run_local_canonicalize_path, 0 },
{ "LOCAL-NAMEMAP-CACHE1", run_local_namemap_cache1, 0 },
--
1.9.1
From af59fdb8ddbe58a58b9db0b6624741763ac41e64 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 20 Dec 2017 08:25:19 +0100
Subject: [PATCH 2/2] g_lock: fix cleanup of stale entries in g_lock_trylock()
g_lock_trylock() always incremented the counter 'i', even after cleaning a stale
entry at position 'i', which means it skipped checking for a conflict against
the new entry at position 'i'.
As result a process could get a write lock, while there're still
some read lock holders. Once we get into that problem, also more than
one write lock are possible.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13195
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
selftest/knownfail.d/local-g-lock6 | 1 -
source3/lib/g_lock.c | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
delete mode 100644 selftest/knownfail.d/local-g-lock6
diff --git a/selftest/knownfail.d/local-g-lock6 b/selftest/knownfail.d/local-g-lock6
deleted file mode 100644
index 14fd5c8..0000000
--- a/selftest/knownfail.d/local-g-lock6
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.smbtorture_s3.LOCAL-G-LOCK6.smbtorture
diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
index 50ea566..bd63dad 100644
--- a/source3/lib/g_lock.c
+++ b/source3/lib/g_lock.c
@@ -230,7 +230,7 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
}
}
- for (i=0; i<lck.num_recs; i++) {
+ for (i=0; i<lck.num_recs;) {
struct g_lock_rec lock;
g_lock_get_rec(&lck, i, &lock);
@@ -269,7 +269,9 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
*/
g_lock_rec_del(&lck, i);
modified = true;
+ continue;
}
+ i++;
}
modified = true;
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20171220/4eccc3b9/signature.sig>
More information about the samba-technical
mailing list