[PATCH] tfork fix issues with waiter process termination
Ralph Böhme
slow at samba.org
Sat Sep 16 08:29:06 UTC 2017
Hi Gary
On Fri, Sep 15, 2017 at 11:39:15PM +0000, Ralph Böhme via samba-technical wrote:
> On Thu, Sep 14, 2017 at 05:23:47AM +0000, Gary Lockyer via samba-technical wrote:
> > Patches to fix issues with waiter process termination and the closing of
> > the event FD, see the commit comments for a detailed description.
>
> thanks a *lot* for working on this. Looking at this and the other patches right
> now...
goodness, glad you spotted this before the 4.7 release. Guess you had some fun
debugging this, I probably ruined a few weekends of yours and owe you a beer.
Your fix for the status fd is spot on, I'd like to change the event fd fix
however. Depending on whether tfork_event_fd() has been called, ownership of the
event fd should be transferred to the caller.
Please take a look at attached patchset. The FIXUP commits are some minor
fixes. The event fd changes are in the "lib/util: only close the event_fd in
tfork if the caller didn't call tfork_event_fd()" commit.
-slow
-------------- next part --------------
From 4df397541eb2171da81f3b8a6764bc6af8831605 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Mon, 11 Sep 2017 14:48:21 +1200
Subject: [PATCH 1/6] tests util/tfork: Tests for status and event fd
Add tests to ensure that:
- The event_fd becomes readable once the worker process has terminated
- That the event_fd is not closed by the tfork code.
- If this is done in tevent code and the event fde has not been
freed, "Bad talloc magic value - " errors can result.
- That the status call does not block if the parent process launches
more than one child process.
- The status file descriptor for a child is passed to the
subsequent children. These processes hold the FD open, so that
closing the fd does not make the read end go readable, and the
process calling status blocks.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
lib/util/tests/tfork.c | 251 +++++++++++++++++++++++++++++++++++++++++++++
selftest/knownfail.d/tfork | 1 +
2 files changed, 252 insertions(+)
create mode 100644 selftest/knownfail.d/tfork
diff --git a/lib/util/tests/tfork.c b/lib/util/tests/tfork.c
index c9005021b8a..2a1490213da 100644
--- a/lib/util/tests/tfork.c
+++ b/lib/util/tests/tfork.c
@@ -22,6 +22,7 @@
#include <tevent.h>
#include "system/filesys.h"
#include "system/wait.h"
+#include "poll.h"
#include "libcli/util/ntstatus.h"
#include "torture/torture.h"
#include "lib/util/data_blob.h"
@@ -533,6 +534,248 @@ done:
return ok;
}
+/*
+ * Test to ensure that the event_fd becomes readable after
+ * a tfork_process terminates.
+ */
+static bool test_tfork_event_file_handle(struct torture_context *tctx)
+{
+ bool ok = true;
+
+ struct tfork *t1 = NULL;
+ pid_t child1;
+ struct pollfd poll1[] = { {-1, POLLIN} };
+
+ struct tfork *t2 = NULL;
+ pid_t child2;
+ struct pollfd poll2[] = { {-1, POLLIN} };
+
+
+ t1 = tfork_create();
+ if (t1 == NULL) {
+ torture_fail(tctx, "tfork failed\n");
+ return false;
+ }
+
+ child1 = tfork_child_pid(t1);
+ if (child1 == 0) {
+ /*
+ * Parent process will kill this with a SIGTERM
+ * so 10 seconds should be plenty
+ */
+ sleep(10);
+ exit(1);
+ }
+ poll1[0].fd = tfork_event_fd(t1);
+
+ t2 = tfork_create();
+ if (t2 == NULL) {
+ torture_fail(tctx, "tfork failed\n");
+ return false;
+ }
+ child2 = tfork_child_pid(t2);
+ if (child2 == 0) {
+ /*
+ * Parent process will kill this with a SIGTERM
+ * so 10 seconds should be plenty
+ */
+ sleep(10);
+ exit(2);
+ }
+ poll2[0].fd = tfork_event_fd(t2);
+
+ /*
+ * Have forked two process and are in the master process
+ * Expect that both event_fds are unreadable
+ */
+ poll(poll1, 1, 0);
+ ok = !(poll1[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd readable\n");
+ poll(poll2, 1, 0);
+ ok = !(poll2[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd readable\n");
+
+ /* Kill the first child process */
+ kill(child1, SIGKILL);
+ sleep(1);
+
+ /*
+ * Have killed the first child, so expect it's event_fd to have gone
+ * readable.
+ *
+ */
+ poll(poll1, 1, 0);
+ ok = (poll1[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd not readable\n");
+ poll(poll2, 1, 0);
+ ok = !(poll2[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 2 event fd readable\n");
+
+ /* Kill the secind child process */
+ kill(child2, SIGKILL);
+ sleep(1);
+ /*
+ * Have killed the children, so expect their event_fd's to have gone
+ * readable.
+ *
+ */
+ poll(poll1, 1, 0);
+ ok = (poll1[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd not readable\n");
+ poll(poll2, 1, 0);
+ ok = (poll2[0].revents & POLLIN);
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 2 event fd not readable\n");
+
+done:
+ return ok;
+}
+
+/*
+ * Test to ensure that the status calls behave as expected after a process
+ * terminates.
+ *
+ * As the parent process owns the status fd's they get passed to all
+ * subsequent children after a tfork. So it's possible for another
+ * child process to hold the status pipe open.
+ *
+ * The event fd needs to be left open by tfork, as a close in the status
+ * code can cause issues in tevent code.
+ *
+ */
+static bool test_tfork_status_handle(struct torture_context *tctx)
+{
+ bool ok = true;
+
+ struct tfork *t1 = NULL;
+ pid_t child1;
+
+ struct tfork *t2 = NULL;
+ pid_t child2;
+
+ int status;
+ int fd;
+ int ev1_fd;
+ int ev2_fd;
+
+
+ t1 = tfork_create();
+ if (t1 == NULL) {
+ torture_fail(tctx, "tfork failed\n");
+ return false;
+ }
+
+ child1 = tfork_child_pid(t1);
+ if (child1 == 0) {
+ /*
+ * Parent process will kill this with a SIGTERM
+ * so 10 seconds should be plenty
+ */
+ sleep(10);
+ exit(1);
+ }
+ ev1_fd = tfork_event_fd(t1);
+
+ t2 = tfork_create();
+ if (t2 == NULL) {
+ torture_fail(tctx, "tfork failed\n");
+ return false;
+ }
+ child2 = tfork_child_pid(t2);
+ if (child2 == 0) {
+ /*
+ * Parent process will kill this with a SIGTERM
+ * so 10 seconds should be plenty
+ */
+ sleep(10);
+ exit(2);
+ }
+ ev2_fd = tfork_event_fd(t2);
+
+ /*
+ * Have forked two process and are in the master process
+ * expect that the status call will block, and hence return -1
+ * as the processes are still running
+ * The event fd's should be open.
+ */
+ status = tfork_status(&t1, false);
+ ok = status == -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork status available for non terminated "
+ "process 1\n");
+ /* Is the event fd open? */
+ fd = dup(ev1_fd);
+ ok = fd != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd is not open");
+
+ status = tfork_status(&t2, false);
+ ok = status == -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork status avaiable for non terminated "
+ "process 2\n");
+ /* Is the event fd open? */
+ fd = dup(ev2_fd);
+ ok = fd != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 2 event fd is not open");
+
+ /*
+ * Kill the first process, it's status should be readable
+ * and it's event_fd should be open
+ * The second process's status should be unreadable.
+ */
+ kill(child1, SIGTERM);
+ sleep(1);
+ status = tfork_status(&t1, false);
+ ok = status != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork status for child 1 not available after "
+ "termination\n");
+ /* Is the event fd open? */
+ fd = dup(ev2_fd);
+ ok = fd != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd is not open");
+
+ status = tfork_status(&t2, false);
+ ok = status == -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork status available for child 2 after "
+ "termination of child 1\n");
+
+ /*
+ * Kill the second process, it's status should be readable
+ */
+ kill(child2, SIGTERM);
+ sleep(1);
+ status = tfork_status(&t2, false);
+ ok = status != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork status for child 2 not available after "
+ "termination\n");
+
+ /* Check that the event fd's are still open */
+ /* Is the event fd open? */
+ fd = dup(ev1_fd);
+ ok = fd != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 1 event fd is not open");
+ /* Is the event fd open? */
+ fd = dup(ev2_fd);
+ ok = fd != -1;
+ torture_assert_goto(tctx, ok, ok, done,
+ "tfork process 2 event fd is not open");
+
+done:
+ return ok;
+}
+
struct torture_suite *torture_local_tfork(TALLOC_CTX *mem_ctx)
{
struct torture_suite *suite =
@@ -574,5 +817,13 @@ struct torture_suite *torture_local_tfork(TALLOC_CTX *mem_ctx)
"tfork_cmd_send",
test_tfork_cmd_send);
+ torture_suite_add_simple_test(suite,
+ "tfork_event_file_handle",
+ test_tfork_event_file_handle);
+
+ torture_suite_add_simple_test(suite,
+ "tfork_status_handle",
+ test_tfork_status_handle);
+
return suite;
}
diff --git a/selftest/knownfail.d/tfork b/selftest/knownfail.d/tfork
new file mode 100644
index 00000000000..ea06b01a44e
--- /dev/null
+++ b/selftest/knownfail.d/tfork
@@ -0,0 +1 @@
+samba4.local.tfork.tfork_status_handle\(none\)
--
2.13.5
From c59436edcaff73c564344f22efa626b80c8b9fe0 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Mon, 11 Sep 2017 10:25:49 +1200
Subject: [PATCH 2/6] util/tfork: Write to the status pipe
The previous design relied on only calling close() of the status pipe.
We now write a single 0 byte to the status FD as well as closing it in the
parent process. Both of these operations typically trigger a read
event on the other end of the FD, held in the waiter process (the child).
The child process blocks on the status FD, until it becomes readable.
However if there is a sibling process that was launched after the waiter
process they also will hold the status FD open and the status FD would,
until this change, never become readable to the waiter process (the child).
This caused the waiter process (child) not to exit and the parent process
to hang in tfork_status() while expecting the waitpid() to return.
That is, file descriptors are essentially global variables copied
to children in the process tree. The last child that (unwittingly) holds
the file descriptor open is the one that needs to trigger the close() this
code previously depended on.
Without this change, there is no notification of process death until
all these unrelated children exit for their own reasons.
We can write up to 4K (PIPE_BUF) into this pipe before blocking,
but we only write one byte. Additionally sys_write() refuses to block.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
lib/util/tfork.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index cc2e0a05f4e..fbc2e251e8d 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -838,7 +838,18 @@ int tfork_status(struct tfork **_t, bool wait)
/*
* This triggers process exit in the waiter.
+ * We write to the fd as well as closing it, as any tforked sibling
+ * processes will also have the writable end of this socket open.
+ *
*/
+ {
+ size_t nwritten;
+ nwritten = sys_write(t->status_fd, &(char){0}, sizeof(char));
+ if (nwritten != sizeof(char)) {
+ close(t->status_fd);
+ return -1;
+ }
+ }
close(t->status_fd);
do {
--
2.13.5
From bcc20fc7f1dd7eac8689c2508336bde14a31a1d8 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 15 Sep 2017 19:00:56 -0700
Subject: [PATCH 3/6] FIXUP: we expect to read 1 byte from the pipe...
---
lib/util/tfork.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index fbc2e251e8d..71f5e1b6ce9 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -670,7 +670,7 @@ static pid_t tfork_start_waiter_and_worker(struct tfork_state *state,
}
_exit(errno);
}
- if (nread != 0) {
+ if (nread != 1) {
_exit(255);
}
--
2.13.5
From 65c9aa099328d9b07d2b53b797c5afedaa9b141f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 16 Sep 2017 01:22:31 -0700
Subject: [PATCH 4/6] lib/util: only close the event_fd in tfork if the caller
didn't call tfork_event_fd()
Make closing of the event_fd the global responsibility of the
parent process if it called tfork_event_fd().
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/util/tfork.c | 13 ++++++++++---
lib/util/tfork.h | 5 ++++-
selftest/knownfail.d/tfork | 1 -
3 files changed, 14 insertions(+), 5 deletions(-)
delete mode 100644 selftest/knownfail.d/tfork
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index 71f5e1b6ce9..3a2f82add06 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -795,9 +795,13 @@ pid_t tfork_child_pid(const struct tfork *t)
return t->worker_pid;
}
-int tfork_event_fd(const struct tfork *t)
+int tfork_event_fd(struct tfork *t)
{
- return t->event_fd;
+ int fd = t->event_fd;
+
+ assert(t->event_fd != -1);
+ t->event_fd = -1;
+ return fd;
}
int tfork_status(struct tfork **_t, bool wait)
@@ -857,7 +861,10 @@ int tfork_status(struct tfork **_t, bool wait)
} while ((pid == -1) && (errno == EINTR));
assert(pid == t->waiter_pid);
- close(t->event_fd);
+ if (t->event_fd != -1) {
+ close(t->event_fd);
+ t->event_fd = -1;
+ }
free(t);
t = NULL;
diff --git a/lib/util/tfork.h b/lib/util/tfork.h
index 1fea2ba4129..8880823a5d0 100644
--- a/lib/util/tfork.h
+++ b/lib/util/tfork.h
@@ -65,12 +65,15 @@ pid_t tfork_child_pid(const struct tfork *t);
*
* @param[in] t Pointer to struct tfork returned by tfork_create()
*
+ * It is the callers responsibility to ensure that the event fd returned by
+ * tfork_event_fd() is closed.
+ *
* @return An fd that becomes readable when the child created with
* tfork_create() terminates. It is guaranteed that a
* subsequent call to tfork_status() will not block and return
* the exit status of the child.
**/
-int tfork_event_fd(const struct tfork *t);
+int tfork_event_fd(struct tfork *t);
/**
* @brief Wait for the child to terminate and return its exit status
diff --git a/selftest/knownfail.d/tfork b/selftest/knownfail.d/tfork
deleted file mode 100644
index ea06b01a44e..00000000000
--- a/selftest/knownfail.d/tfork
+++ /dev/null
@@ -1 +0,0 @@
-samba4.local.tfork.tfork_status_handle\(none\)
--
2.13.5
From eeea2de9e8bd50d294d0c776448ca5af3d3cb662 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Fri, 8 Sep 2017 14:03:25 +1200
Subject: [PATCH 5/6] util_runcmd: Free the fde in event handler.
Free the fde in the event handler to prevent the event triggering again
While not strictly necessary in this case, this code serves as an
example of the usage of tfork.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
lib/util/util_runcmd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index ac74371e76d..f14bb53b636 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -265,7 +265,7 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
fd = state->fd_stderr;
} else {
int status;
-
+
status = tfork_status(&state->tfork, false);
if (status == -1) {
if (errno == EAGAIN || errno == EWOULDBLOCK) {
@@ -275,6 +275,7 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
tevent_req_error(req, errno);
return;
}
+ TALLOC_FREE(fde);
if (WIFEXITED(status)) {
status = WEXITSTATUS(status);
--
2.13.5
From e52887222528d3ab1676168d23cf220c9a7cab7b Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 16 Sep 2017 00:28:54 -0700
Subject: [PATCH 6/6] FIXUP
---
lib/util/util_runcmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index f14bb53b636..6077fddde56 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -265,7 +265,7 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
fd = state->fd_stderr;
} else {
int status;
-
+
status = tfork_status(&state->tfork, false);
if (status == -1) {
if (errno == EAGAIN || errno == EWOULDBLOCK) {
--
2.13.5
More information about the samba-technical
mailing list