[PATCH] tfork

Ralph Böhme slow at samba.org
Tue Apr 18 19:55:56 UTC 2017


Hi!

Attached is a patchset that adds a signal and waitpid() avoiding fork()
replacement: tfork().

It was based on an older patchset from metze, called double_fork at that time,
that I extended to fork three-times in order to return a pollable status fd.

It's intented use is in samba_runcmd_send() where it avoids calling waitpid()
and dealing with SIGCHLD to get the exit status of the executed command.

With this change it is safe to call samba_runcmd_send() in smbd code as an async
replacement for smbrun().

I have a usecase here where someone wants to run shell scripts from source3
code without blocking.

Please review & push if happy.

Thanks!
-slow
-------------- next part --------------
From 169a378fdf513202df410dcbec9ab14cb7e55340 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 23 Sep 2010 18:10:02 +0200
Subject: [PATCH 1/5] lib/util: add tfork()

trippe-fork to avoid handling SIGCHLD in the parent.

This function is a workaround for the problem of using fork() in
library code. In that case the library should avoid to set a global
signal handler for SIGCHLD, because the application may wants to use its
own handler.

status_fd can be used to wait for the child to exit and get its exit
status.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/tfork.c       | 329 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/util/tfork.h       |  52 ++++++++
 lib/util/wscript_build |   4 +-
 3 files changed, 383 insertions(+), 2 deletions(-)
 create mode 100644 lib/util/tfork.c
 create mode 100644 lib/util/tfork.h

diff --git a/lib/util/tfork.c b/lib/util/tfork.c
new file mode 100644
index 0000000..23dba3f
--- /dev/null
+++ b/lib/util/tfork.c
@@ -0,0 +1,329 @@
+/*
+   fork on steroids to avoid SIGCHLD and waitpid
+
+   Copyright (C) Stefan Metzmacher 2010
+   Copyright (C) Ralph Boehme 2017
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "replace.h"
+#include "system/wait.h"
+#include "system/filesys.h"
+#include "lib/util/samba_util.h"
+#include "lib/util/sys_rw.h"
+#include "lib/util/tfork.h"
+#include "lib/util/debug.h"
+
+struct tfork_state {
+	void (*old_sig_chld)(int);
+	int status_pipe[2];
+	pid_t *parent;
+
+	pid_t level0_pid;
+	int level0_status;
+
+	pid_t level1_pid;
+	int level1_errno;
+
+	pid_t level2_pid;
+	int level2_errno;
+
+	pid_t level3_pid;
+};
+
+/*
+ * TODO: We should make this global thread local
+ */
+static struct tfork_state *tfork_global;
+
+static void tfork_sig_chld(int signum)
+{
+	int ret;
+
+	if (tfork_global->level1_pid > 0) {
+		ret = waitpid(tfork_global->level1_pid,
+			      &tfork_global->level0_status,
+			      WNOHANG);
+		if (ret == tfork_global->level1_pid) {
+			tfork_global->level1_pid = -1;
+			return;
+		}
+	}
+
+	/*
+	 * Not our child, forward to old handler
+	 */
+
+	 if (tfork_global->old_sig_chld == SIG_IGN) {
+		return;
+	}
+
+	if (tfork_global->old_sig_chld == SIG_DFL) {
+		return;
+	}
+
+	tfork_global->old_sig_chld(signum);
+}
+
+static pid_t level2_fork_and_wait(int child_ready_fd)
+{
+	int ret;
+	int status;
+	ssize_t written;
+	pid_t pid;
+	int fd;
+	bool wait;
+
+	/*
+	 * Child level 2.
+	 *
+	 * Do a final fork and if the tfork() caller passed a status_fd, wait
+	 * for child3 and return its exit status via status_fd.
+	 */
+
+	pid = fork();
+	if (pid == 0) {
+		/*
+		 * Child level 3, this one finally returns from tfork() as child
+		 * with pid 0.
+		 *
+		 * Cleanup all ressources we allocated before returning.
+		 */
+		close(child_ready_fd);
+		close(tfork_global->status_pipe[1]);
+
+		if (tfork_global->parent != NULL) {
+			/*
+			 * we're in the child and return the level0 parent pid
+			 */
+			*tfork_global->parent = tfork_global->level0_pid;
+		}
+
+		anonymous_shared_free(tfork_global);
+		tfork_global = NULL;
+
+		return 0;
+	}
+
+	tfork_global->level3_pid = pid;
+	if (tfork_global->level3_pid == -1) {
+		tfork_global->level2_errno = errno;
+		_exit(0);
+	}
+
+	sys_write(child_ready_fd, &(char){0}, 1);
+
+	if (tfork_global->status_pipe[1] == -1) {
+		_exit(0);
+	}
+	wait = true;
+
+	/*
+	 * We're going to stay around until child3 exits, so lets close all fds
+	 * other then the pipe fd we may have inherited from the caller.
+	 */
+	fd = dup2(tfork_global->status_pipe[1], 0);
+	if (fd == -1) {
+		status = errno;
+		kill(tfork_global->level3_pid, SIGKILL);
+		wait = false;
+	}
+	closefrom(1);
+
+	while (wait) {
+		ret = waitpid(tfork_global->level3_pid, &status, 0);
+		if (ret == -1) {
+			if (errno == EINTR) {
+				continue;
+			}
+			status = errno;
+		}
+		break;
+	}
+
+	written = sys_write(fd, &status, sizeof(status));
+	if (written != sizeof(status)) {
+		abort();
+	}
+
+	_exit(0);
+}
+
+pid_t tfork(int *status_fd, pid_t *parent)
+{
+	int ret;
+	pid_t pid;
+	pid_t child;
+
+	tfork_global = (struct tfork_state *)
+		anonymous_shared_allocate(sizeof(struct tfork_state));
+	if (tfork_global == NULL) {
+		return -1;
+	}
+
+	tfork_global->parent = parent;
+	tfork_global->status_pipe[0] = -1;
+	tfork_global->status_pipe[1] = -1;
+
+	tfork_global->level0_pid = getpid();
+	tfork_global->level0_status = -1;
+	tfork_global->level1_pid = -1;
+	tfork_global->level1_errno = ECANCELED;
+	tfork_global->level2_pid = -1;
+	tfork_global->level2_errno = ECANCELED;
+	tfork_global->level3_pid = -1;
+
+	if (status_fd != NULL) {
+		ret = pipe(&tfork_global->status_pipe[0]);
+		if (ret != 0) {
+			int saved_errno = errno;
+
+			anonymous_shared_free(tfork_global);
+			tfork_global = NULL;
+			errno = saved_errno;
+			return -1;
+		}
+
+		*status_fd = tfork_global->status_pipe[0];
+	}
+
+	/*
+	 * We need to set our own signal handler to prevent any existing signal
+	 * handler from reaping our child.
+	 */
+	tfork_global->old_sig_chld = CatchSignal(SIGCHLD, tfork_sig_chld);
+
+	pid = fork();
+	if (pid == 0) {
+		int level2_pipe[2];
+		char c;
+		ssize_t nread;
+
+		/*
+		 * Child level 1.
+		 *
+		 * Restore SIGCHLD handler
+		 */
+		CatchSignal(SIGCHLD, SIG_DFL);
+
+		/*
+		 * Close read end of the signal pipe, we don't need it anymore
+		 * and don't want to leak it into childs.
+		 */
+		if (tfork_global->status_pipe[0] != -1) {
+			close(tfork_global->status_pipe[0]);
+			tfork_global->status_pipe[0] = -1;
+		}
+
+		/*
+		 * Create a pipe for waiting for the child level 2 to finish
+		 * forking.
+		 */
+		ret = pipe(&level2_pipe[0]);
+		if (ret != 0) {
+			tfork_global->level1_errno = errno;
+			_exit(0);
+		}
+
+		pid = fork();
+		if (pid == 0) {
+
+			/*
+			 * Child level 2.
+			 */
+
+			close(level2_pipe[0]);
+			return level2_fork_and_wait(level2_pipe[1]);
+		}
+
+		tfork_global->level2_pid = pid;
+		if (tfork_global->level2_pid == -1) {
+			tfork_global->level1_errno = errno;
+			_exit(0);
+		}
+
+		close(level2_pipe[1]);
+		level2_pipe[1] = -1;
+
+		nread = sys_read(level2_pipe[0], &c, 1);
+		if (nread != 1) {
+			abort();
+		}
+		_exit(0);
+	}
+
+	tfork_global->level1_pid = pid;
+	if (tfork_global->level1_pid == -1) {
+		int saved_errno = errno;
+
+		anonymous_shared_free(tfork_global);
+		tfork_global = NULL;
+		errno = saved_errno;
+		return -1;
+	}
+
+	/*
+	 * By using the helper variable pid we avoid a TOCTOU with the signal
+	 * handler that will set tfork_global->level1_pid to -1 (which would
+	 * cause waitpid() to block waiting for another exitted child).
+	 *
+	 * We can't avoid the race waiting for pid twice (in the signal handler
+	 * and then again here in the while loop), but we must avoid waiting for
+	 * -1 and this does the trick.
+	 */
+	pid = tfork_global->level1_pid;
+
+	while (tfork_global->level1_pid != -1) {
+		ret = waitpid(pid, &tfork_global->level0_status, 0);
+		if (ret == -1 && errno == EINTR) {
+			continue;
+		}
+
+		break;
+	}
+
+	CatchSignal(SIGCHLD, tfork_global->old_sig_chld);
+
+	if (tfork_global->level0_status != 0) {
+		anonymous_shared_free(tfork_global);
+		tfork_global = NULL;
+		errno = ECHILD;
+		return -1;
+	}
+
+	if (tfork_global->level2_pid == -1) {
+		int saved_errno = tfork_global->level1_errno;
+
+		anonymous_shared_free(tfork_global);
+		tfork_global = NULL;
+		errno = saved_errno;
+		return -1;
+	}
+
+	if (tfork_global->level3_pid == -1) {
+		int saved_errno = tfork_global->level2_errno;
+
+		anonymous_shared_free(tfork_global);
+		tfork_global = NULL;
+		errno = saved_errno;
+		return -1;
+	}
+
+	child = tfork_global->level3_pid;
+	anonymous_shared_free(tfork_global);
+	tfork_global = NULL;
+
+	return child;
+}
diff --git a/lib/util/tfork.h b/lib/util/tfork.h
new file mode 100644
index 0000000..0c62fc3
--- /dev/null
+++ b/lib/util/tfork.h
@@ -0,0 +1,52 @@
+/*
+   fork on steroids to avoid SIGCHLD and waitpid
+
+   Copyright (C) Stefan Metzmacher 2010
+   Copyright (C) Ralph Boehme 2017
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef LIB_UTIL_TFORK_H
+#define LIB_UTIL_TFORK_H
+
+/**
+ * @brief a fork() that avoids SIGCHLD and waitpid
+ *
+ * This function is a workaround for the problem of using fork() in
+ * library code. In that case the library should avoid to set a global
+ * signal handler for SIGCHLD, because the application may wants to use its
+ * own handler.
+ *
+ * The child process will start with SIGCHLD handler set to SIG_DFL, so the
+ * child might need to setup its own handler.
+ *
+ * @param[out] status_fd  If this is not NULL, tfork creates a pipe and returns
+ *                        the readable end via this pointer. The caller can
+ *                        wait for the process to finish by polling the
+ *                        status_fd for readability and can then read the exit
+ *                        status (an int).
+ *
+ * @param[out] parent     The PID of the parent process, if 0 is returned
+ *                        otherwise the variable will not be touched at all.
+ *                        It is possible to pass NULL.
+ *
+ * @return                On success, the PID of the child process is returned
+ *                        in the parent, and 0 is returned in the child. On
+ *                        failure, -1 is returned in the parent, no child
+ *                        process is created, and errno is set appropriately.
+ */
+int tfork(int *status_fd, int *parent);
+
+#endif /* LIB_UTIL_TFORK_H */
diff --git a/lib/util/wscript_build b/lib/util/wscript_build
index 91505eb..a2fff9e 100644
--- a/lib/util/wscript_build
+++ b/lib/util/wscript_build
@@ -120,10 +120,10 @@ else:
                     idtree_random.c base64.c
                     util_str.c util_str_common.c ms_fnmatch.c
                     server_id.c dprintf.c pidfile.c
-                    tevent_debug.c memcache.c unix_match.c''',
+                    tevent_debug.c memcache.c unix_match.c tfork.c''',
                   deps='samba-util-core DYNCONFIG close-low-fd tini tiniparser genrand',
                   public_deps='talloc tevent execinfo pthread LIBCRYPTO charset util_setid systemd systemd-daemon',
-                  public_headers='debug.h attr.h byteorder.h data_blob.h memory.h safe_string.h time.h talloc_stack.h string_wrappers.h idtree.h idtree_random.h blocking.h signal.h substitute.h fault.h genrand.h',
+                  public_headers='debug.h attr.h byteorder.h data_blob.h memory.h safe_string.h time.h talloc_stack.h string_wrappers.h idtree.h idtree_random.h blocking.h signal.h substitute.h fault.h genrand.h tfork.h',
                   header_path= [ ('dlinklist.h samba_util.h', '.'), ('*', 'util') ],
                   local_include=False,
                   vnum='0.0.1',
-- 
2.9.3


From e6d5438973b7cf588fd823e2b918f0a3bd326342 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 11 Apr 2017 17:32:01 +0200
Subject: [PATCH 2/5] lib/util: add a test for tfork()

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/tests/tfork.c              | 102 ++++++++++++++++++++++++++++++++++++
 source4/torture/local/local.c       |   1 +
 source4/torture/local/wscript_build |   1 +
 3 files changed, 104 insertions(+)
 create mode 100644 lib/util/tests/tfork.c

diff --git a/lib/util/tests/tfork.c b/lib/util/tests/tfork.c
new file mode 100644
index 0000000..7963aaf
--- /dev/null
+++ b/lib/util/tests/tfork.c
@@ -0,0 +1,102 @@
+/*
+ * Tests for tfork
+ *
+ * Copyright Ralph Boehme <slow at samba.org> 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "replace.h"
+#include <talloc.h>
+#include "system/filesys.h"
+#include "libcli/util/ntstatus.h"
+#include "torture/torture.h"
+#include "lib/util/data_blob.h"
+#include "torture/local/proto.h"
+#include "lib/util/tfork.h"
+#include "lib/util/samba_util.h"
+#include "lib/util/sys_rw.h"
+
+static bool test_tfork_simple(struct torture_context *tctx)
+{
+	pid_t pid;
+	pid_t parent = getpid();
+	pid_t parent_arg;
+
+	pid = tfork(NULL, &parent_arg);
+	if (pid == 0) {
+		torture_comment(tctx, "my parent pid is %d\n", parent);
+		torture_assert(tctx, parent == parent_arg, "tfork failed\n");
+		_exit(0);
+	}
+	if (pid == -1) {
+		torture_fail(tctx, "tfork failed\n");
+		return false;
+	}
+
+	return true;
+}
+
+static bool test_tfork_status(struct torture_context *tctx)
+{
+	pid_t child;
+	int status;
+	ssize_t nread;
+	int status_fd = -1;
+	bool ok = true;
+
+	child = tfork(&status_fd, NULL);
+	if (child == 0) {
+		_exit(123);
+	}
+	if (child == -1) {
+		torture_fail(tctx, "tfork failed\n");
+		return false;
+	}
+
+	nread = sys_read(status_fd, &status, sizeof(status));
+	if (nread != sizeof(status)) {
+		torture_fail(tctx, "sys_read failed\n");
+	}
+
+	torture_assert_goto(tctx, WIFEXITED(status) == true, ok, done,
+			    "tfork failed\n");
+	torture_assert_goto(tctx, WEXITSTATUS(status) == 123, ok, done,
+			    "tfork failed\n");
+
+	torture_comment(tctx, "exit status [%d]\n", WEXITSTATUS(status));
+
+done:
+	if (status_fd != -1) {
+		close(status_fd);
+	}
+
+	return ok;
+}
+
+struct torture_suite *torture_local_tfork(TALLOC_CTX *mem_ctx)
+{
+	struct torture_suite *suite =
+		torture_suite_create(mem_ctx, "tfork");
+
+	torture_suite_add_simple_test(suite,
+				      "tfork_simple",
+				      test_tfork_simple);
+
+	torture_suite_add_simple_test(suite,
+				      "tfork_status",
+				      test_tfork_status);
+
+	return suite;
+}
diff --git a/source4/torture/local/local.c b/source4/torture/local/local.c
index 89066c5..353cc27 100644
--- a/source4/torture/local/local.c
+++ b/source4/torture/local/local.c
@@ -75,6 +75,7 @@
 	torture_local_nss,
 	torture_local_fsrvp,
 	torture_local_util_str_escape,
+	torture_local_tfork,
 	NULL
 };
 
diff --git a/source4/torture/local/wscript_build b/source4/torture/local/wscript_build
index 2f1a7c8..6cbb14d 100644
--- a/source4/torture/local/wscript_build
+++ b/source4/torture/local/wscript_build
@@ -21,6 +21,7 @@ TORTURE_LOCAL_SOURCE = '''../../../lib/util/charset/tests/iconv.c
 	../../../lib/util/tests/strv_util.c
 	../../../lib/util/tests/util.c
 	../../../lib/util/tests/util_str_escape.c
+	../../../lib/util/tests/tfork.c
 	verif_trailer.c
 	nss_tests.c
 	fsrvp_state.c'''
-- 
2.9.3


From a650b27bbfd0ffbb2fe812af2cbaa1f18f2e4375 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 11 Apr 2017 20:05:05 +0200
Subject: [PATCH 3/5] lib/util: make use of tfork in samba_runcmd_send()

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util_runcmd.c | 103 +++++++++++++++++++++++++++----------------------
 lib/util/util_runcmd.h |   3 +-
 2 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index 9264cbb..7a2e1c6 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -29,6 +29,8 @@
 #include "system/filesys.h"
 #include "../lib/util/tevent_unix.h"
 #include "../lib/util/util_runcmd.h"
+#include "../lib/util/tfork.h"
+#include "../lib/util/sys_rw.h"
 
 static int samba_runcmd_state_destructor(struct samba_runcmd_state *state)
 {
@@ -106,7 +108,7 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 		return tevent_req_post(req, ev);
 	}
 
-	state->pid = fork();
+	state->pid = tfork(&state->fd_status, NULL);
 	if (state->pid == (pid_t)-1) {
 		close(p1[0]);
 		close(p1[1]);
@@ -130,10 +132,12 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 		set_blocking(state->fd_stdout, false);
 		set_blocking(state->fd_stderr, false);
 		set_blocking(state->fd_stdin,  false);
+		set_blocking(state->fd_status, false);
 
 		smb_set_close_on_exec(state->fd_stdin);
 		smb_set_close_on_exec(state->fd_stdout);
 		smb_set_close_on_exec(state->fd_stderr);
+		smb_set_close_on_exec(state->fd_status);
 
 		talloc_set_destructor(state, samba_runcmd_state_destructor);
 
@@ -145,6 +149,7 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 		if (tevent_req_nomem(state->fde_stdout, req)) {
 			close(state->fd_stdout);
 			close(state->fd_stderr);
+			close(state->fd_status);
 			return tevent_req_post(req, ev);
 		}
 		tevent_fd_set_auto_close(state->fde_stdout);
@@ -155,11 +160,26 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
 						  samba_runcmd_io_handler,
 						  req);
 		if (tevent_req_nomem(state->fde_stdout, req)) {
+			close(state->fd_stdout);
 			close(state->fd_stderr);
+			close(state->fd_status);
 			return tevent_req_post(req, ev);
 		}
 		tevent_fd_set_auto_close(state->fde_stderr);
 
+		state->fde_status = tevent_add_fd(ev, state,
+						  state->fd_status,
+						  TEVENT_FD_READ,
+						  samba_runcmd_io_handler,
+						  req);
+		if (tevent_req_nomem(state->fde_stdout, req)) {
+			close(state->fd_stdout);
+			close(state->fd_stderr);
+			close(state->fd_status);
+			return tevent_req_post(req, ev);
+		}
+		tevent_fd_set_auto_close(state->fde_status);
+
 		if (!timeval_is_zero(&endtime)) {
 			tevent_req_set_endtime(req, ev, endtime);
 		}
@@ -231,6 +251,10 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
 	char *p;
 	int n, fd;
 
+	if (!(flags & TEVENT_FD_READ)) {
+		return;
+	}
+
 	if (fde == state->fde_stdout) {
 		level = state->stdout_log_level;
 		fd = state->fd_stdout;
@@ -238,13 +262,40 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
 		level = state->stderr_log_level;
 		fd = state->fd_stderr;
 	} else {
-		return;
-	}
+		int status;
+		ssize_t nread;
+
+		/*
+		 * Note: reading on a non-blocking pipe is guaranteed to deliver
+		 * any pending data up to some kernel limit which is way beyond
+		 * what we're reading here.
+		 */
+		nread = sys_read(state->fd_status, &status, sizeof(status));
+		if (nread != sizeof(status)) {
+			DBG_ERR("Bad read on status pipe\n");
+			tevent_req_error(req, EIO);
+			return;
+		}
 
-	if (!(flags & TEVENT_FD_READ)) {
+		if (WIFEXITED(status)) {
+			status = WEXITSTATUS(status);
+		} else if (WIFSIGNALED(status)) {
+			status = WTERMSIG(status);
+		} else {
+			status = ECHILD;
+		}
+
+		DBG_NOTICE("Child %s exited %d\n", state->arg0, status);
+		if (status != 0) {
+			tevent_req_error(req, status);
+			return;
+		}
+
+		tevent_req_done(req);
 		return;
 	}
 
+
 	n = read(fd, &state->buf[state->buf_used],
 		 sizeof(state->buf) - state->buf_used);
 	if (n > 0) {
@@ -253,53 +304,11 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
 		if (fde == state->fde_stdout) {
 			talloc_free(fde);
 			state->fde_stdout = NULL;
+			return;
 		}
 		if (fde == state->fde_stderr) {
 			talloc_free(fde);
 			state->fde_stderr = NULL;
-		}
-		if (state->fde_stdout == NULL &&
-		    state->fde_stderr == NULL) {
-			int status;
-			/* the child has closed both stdout and
-			 * stderr, assume its dead */
-			pid_t pid = waitpid(state->pid, &status, 0);
-			if (pid != state->pid) {
-				if (errno == ECHILD) {
-					/* this happens when the
-					   parent has set SIGCHLD to
-					   SIG_IGN. In that case we
-					   can only get error
-					   information for the child
-					   via its logging. We should
-					   stop using SIG_IGN on
-					   SIGCHLD in the standard
-					   process model.
-					*/
-					DEBUG(0, ("Error in waitpid() unexpectedly got ECHILD "
-						  "for %s child %d - %s, "
-						  "someone has set SIGCHLD to SIG_IGN!\n",
-					state->arg0, (int)state->pid, strerror(errno)));
-					tevent_req_error(req, errno);
-					return;
-				}
-				DEBUG(0,("Error in waitpid() for child %s - %s \n",
-					 state->arg0, strerror(errno)));
-				if (errno == 0) {
-					errno = ECHILD;
-				}
-				tevent_req_error(req, errno);
-				return;
-			}
-			status = WEXITSTATUS(status);
-			DEBUG(3,("Child %s exited with status %d\n",
-				 state->arg0, status));
-			if (status != 0) {
-				tevent_req_error(req, status);
-				return;
-			}
-
-			tevent_req_done(req);
 			return;
 		}
 		return;
diff --git a/lib/util/util_runcmd.h b/lib/util/util_runcmd.h
index 26fdbc6..a887658 100644
--- a/lib/util/util_runcmd.h
+++ b/lib/util/util_runcmd.h
@@ -27,7 +27,8 @@ struct samba_runcmd_state {
 	int stderr_log_level;
 	struct tevent_fd *fde_stdout;
 	struct tevent_fd *fde_stderr;
-	int fd_stdin, fd_stdout, fd_stderr;
+	struct tevent_fd *fde_status;
+	int fd_stdin, fd_stdout, fd_stderr, fd_status;
 	char *arg0;
 	pid_t pid;
 	char buf[1024];
-- 
2.9.3


From c4ae5a9a8b38c0bb0c745b73fd19cd9a868df306 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 15 Apr 2017 09:09:21 +0200
Subject: [PATCH 4/5] wafsamba: add source directory define SRCDIR to config.h

This will be used in the next commit to prepare the path to a test
script in a smbtorture test.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 buildtools/wafsamba/wscript | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/buildtools/wafsamba/wscript b/buildtools/wafsamba/wscript
index 4eef008..430d164 100644
--- a/buildtools/wafsamba/wscript
+++ b/buildtools/wafsamba/wscript
@@ -212,6 +212,8 @@ def configure(conf):
     conf.env.hlist = []
     conf.env.srcdir = conf.srcdir
 
+    conf.define('SRCDIR', conf.env['srcdir'])
+
     if Options.options.timestamp_dependencies:
         conf.ENABLE_TIMESTAMP_DEPENDENCIES()
 
-- 
2.9.3


From 7219fbd0849b452eec155950df63db8f684e7029 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 11 Apr 2017 20:00:05 +0200
Subject: [PATCH 5/5] lib/util: add a test for samba_runcmd_send()

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 lib/util/tests/tfork.c      | 36 ++++++++++++++++++++++++++++++++++++
 testprogs/blackbox/tfork.sh | 15 +++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100755 testprogs/blackbox/tfork.sh

diff --git a/lib/util/tests/tfork.c b/lib/util/tests/tfork.c
index 7963aaf..7a96928 100644
--- a/lib/util/tests/tfork.c
+++ b/lib/util/tests/tfork.c
@@ -19,6 +19,7 @@
 
 #include "replace.h"
 #include <talloc.h>
+#include <tevent.h>
 #include "system/filesys.h"
 #include "libcli/util/ntstatus.h"
 #include "torture/torture.h"
@@ -85,6 +86,37 @@ done:
 	return ok;
 }
 
+static bool test_tfork_cmd_send(struct torture_context *tctx)
+{
+	struct tevent_context *ev = NULL;
+	struct tevent_req *req = NULL;
+	const char *cmd[2] = { NULL, NULL };
+	bool ok = true;
+
+	ev = tevent_context_init(tctx);
+	torture_assert_goto(tctx, ev != NULL, ok, done,
+			    "tevent_context_init failed\n");
+
+	cmd[0] = talloc_asprintf(tctx, "%s/testprogs/blackbox/tfork.sh", SRCDIR);
+	torture_assert_goto(tctx, cmd[0] != NULL, ok, done,
+			    "talloc_asprintf failed\n");
+
+	req = samba_runcmd_send(tctx, ev, timeval_zero(), 1, 0,
+				cmd, "foo", NULL);
+	torture_assert_goto(tctx, req != NULL, ok, done,
+			    "samba_runcmd_send failed\n");
+
+	ok = tevent_req_poll(req, ev);
+	torture_assert_goto(tctx, ok, ok, done, "tevent_req_poll failed\n");
+
+	torture_comment(tctx, "samba_runcmd_send test finished\n");
+
+done:
+	TALLOC_FREE(ev);
+
+	return ok;
+}
+
 struct torture_suite *torture_local_tfork(TALLOC_CTX *mem_ctx)
 {
 	struct torture_suite *suite =
@@ -98,5 +130,9 @@ struct torture_suite *torture_local_tfork(TALLOC_CTX *mem_ctx)
 				      "tfork_status",
 				      test_tfork_status);
 
+	torture_suite_add_simple_test(suite,
+				      "tfork_cmd_send",
+				      test_tfork_cmd_send);
+
 	return suite;
 }
diff --git a/testprogs/blackbox/tfork.sh b/testprogs/blackbox/tfork.sh
new file mode 100755
index 0000000..0f75a8c
--- /dev/null
+++ b/testprogs/blackbox/tfork.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+sleep 1
+
+echo stdout >&1
+echo $1 >&1
+echo stderror >&2
+
+# close stdout and stderror, but don't exit yet
+exec 1>&-
+exec 2>&-
+
+sleep 1
+
+exit 0
-- 
2.9.3



More information about the samba-technical mailing list