[SCM] Samba Shared Repository - branch master updated

Ralph Böhme slow at samba.org
Sat Sep 16 21:51:03 UTC 2017


The branch, master has been updated
       via  563bbb9 util_runcmd: Free the fde in event handler.
       via  6c36ea0 lib/util: only close the event_fd in tfork if the caller didn't call tfork_event_fd()
       via  f6a40ff util/tfork: Write to the status pipe
       via  28edf70 tests util/tfork: Tests for status and event fd
      from  adf46ff cli_credentials: Apply some const

https://git.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit 563bbb9c24d1d0bcc64530a6635b8b82d1ebb24d
Author: Gary Lockyer <gary at catalyst.net.nz>
Date:   Fri Sep 8 14:03:25 2017 +1200

    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.
    
    Bug: https://bugzilla.samba.org/show_bug.cgi?id=13037
    
    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>
    
    Autobuild-User(master): Ralph Böhme <slow at samba.org>
    Autobuild-Date(master): Sat Sep 16 23:50:27 CEST 2017 on sn-devel-144

commit 6c36ea0737ae12fc97e4a024588e6a3845caf329
Author: Ralph Boehme <slow at samba.org>
Date:   Sat Sep 16 01:22:31 2017 -0700

    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().
    
    Bug: https://bugzilla.samba.org/show_bug.cgi?id=13037
    
    Signed-off-by: Ralph Boehme <slow at samba.org>
    Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
    Reviewed-by: Stefan Metzmacher <metze at samba.org>

commit f6a40ff2a1c133b6c30cf3ce29d7bb3ea005e3c8
Author: Gary Lockyer <gary at catalyst.net.nz>
Date:   Mon Sep 11 10:25:49 2017 +1200

    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.
    
    Bug: https://bugzilla.samba.org/show_bug.cgi?id=13037
    
    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>

commit 28edf7012b5fa474897055c8c1a4c438c69b8323
Author: Gary Lockyer <gary at catalyst.net.nz>
Date:   Mon Sep 11 14:48:21 2017 +1200

    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.
    
    Bug: https://bugzilla.samba.org/show_bug.cgi?id=13037
    
    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>

-----------------------------------------------------------------------

Summary of changes:
 lib/util/tests/tfork.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/util/tfork.c       |  27 +++++-
 lib/util/tfork.h       |   7 +-
 lib/util/util_runcmd.c |   1 +
 4 files changed, 281 insertions(+), 5 deletions(-)


Changeset truncated at 500 lines:

diff --git a/lib/util/tests/tfork.c b/lib/util/tests/tfork.c
index c900502..bf642fe 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 "system/select.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/lib/util/tfork.c b/lib/util/tfork.c
index cc2e0a0..ca4ab50 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);
 	}
 
@@ -795,9 +795,14 @@ 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)
@@ -838,7 +843,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 {
@@ -846,7 +862,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 1fea2ba..89c9f72 100644
--- a/lib/util/tfork.h
+++ b/lib/util/tfork.h
@@ -65,12 +65,17 @@ 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. By calling tfork_event_fd() ownership of the fd
+ * is transferred to the caller, calling tfork_event_fd() again will trigger an
+ * abort().
+ *
  * @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/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index ac74371..6077fdd 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -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);


-- 
Samba Shared Repository



More information about the samba-cvs mailing list