Quantcast

[PATCH] tfork

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] tfork

Samba - samba-technical mailing list
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

tfork.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:

> 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.

I'm looking at this right now, but it's making my brain hurt :-).

I've gotten as far as realizing the use of _exit() gets you out
of all sorts of reinit_after_fork() problems and atexit()
issues.

Still reviewing....

Jeremy.


> From 169a378fdf513202df410dcbec9ab14cb7e55340 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
On Tue, 2017-04-18 at 16:22 -0700, Jeremy Allison via samba-technical
wrote:

> On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-
> technical wrote:
> > 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.
>
> I'm looking at this right now, but it's making my brain hurt :-).
>
> I've gotten as far as realizing the use of _exit() gets you out
> of all sorts of reinit_after_fork() problems and atexit()
> issues.
>
> Still reviewing....
>
> Jeremy.

Having been bitten by the SIGCHLD causing the loss of error codes
(until I reworked the standard process model code to use pipes to
detect which child dies), I'm very glad to see some innovation here.

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:

> 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.

OK, there's one comment I want to raise and discuss
before pushing. Not sure if it matters, but at least it's
something we need to have considered I think.

Inside this code you use CatchSignal() to set and remove the
signal handler for SIGTERM in the parent.

Inside CatchSignal() we have:

        ZERO_STRUCT(act);

        act.sa_handler = handler;
#ifdef SA_RESTART
        /*
         * We *want* SIGALRM to interrupt a system call.
         */
        if(signum != SIGALRM)
                act.sa_flags = SA_RESTART;

which means that on any signal != SIGALRM we *always* set
the SA_RESTART flag to restart slow system calls.

However, inside lib/tevent/tevent_signal.c we have:

                struct sigaction act;
                ZERO_STRUCT(act);
                act.sa_handler = tevent_common_signal_handler;
                act.sa_flags = sa_flags;
....
                if (sigaction(signum, &act, sig_state->oldact[signum]) == -1) {

where sa_flags is passed in from the caller of
tevent_add_signal().

All of the callers to tevent_add_signal() I can
find don't use SA_RESTART flags. They all pass
zero in the sa_flags parameter, which means that no slow
system calls will be restarted if the caught signal
is received.

In addition, CatchSignal() only returns the old
act.sa_handler, *not* the content of the old act.sa_flags.

Which means when you call CatchSignal() to temporarily
replace a signal handler added by tevent_add_signal()
and then call CatchSignal() to put it back, you
overwrite any sa_flags that were passed into the
tevent_add_signal() call (not that there are any
right now :-), and will always add in SA_RESTART.

So depending on the order of calling CatchSignal()
or tevent_add_signal() we're going to get different
signal handling behavior (restart or not) on slow
system calls.

This patch doesn't make it any worse though :-).

But long run I think we need to remove all
#ifdef HAVE_SIGACTION tests and just *require*
sigaction for Samba to run.

Then we need to look at fixing CatchSignal()
to take a struct sigaction, and return the existing
struct sigaction instead of void, so it can
be used inside library code to temporarily
replace an existing tevent_add_signal() handler
and correctly replace it.

Eventually we might want to look at removing
the use of CatchSignal() and varients entirely.

Thoughts ?

I'm still reviewing the code, and this discussion
can go on in parallel with pushing this - this
code doesn't make the problem any worse than it
already is :-).

Cheers,

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:

> 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.

Finally reviewed, LGTM. Pushed.

Still want to discuss the CatchSignal() issue though :-).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 02:57:53PM -0700, Jeremy Allison wrote:

> On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:
> > 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.
>
> Finally reviewed, LGTM. Pushed.

great, thanks! Failed in a DNS test, hopefully unrelated. Repushed.

> Still want to discuss the CatchSignal() issue though :-).

Will respond to the other mail in a second.

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 01:32:57PM -0700, Jeremy Allison wrote:

> On Tue, Apr 18, 2017 at 09:55:56PM +0200, Ralph Böhme via samba-technical wrote:
> > 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.
>
> OK, there's one comment I want to raise and discuss
> before pushing. Not sure if it matters, but at least it's
> something we need to have considered I think.
>
> Inside this code you use CatchSignal() to set and remove the
> signal handler for SIGTERM in the parent.

SIGCHLD, but yes, I do use CatchSignal(). :)

> Inside CatchSignal() we have:
>
>         ZERO_STRUCT(act);
>
>         act.sa_handler = handler;
> #ifdef SA_RESTART
>         /*
>          * We *want* SIGALRM to interrupt a system call.
>          */
>         if(signum != SIGALRM)
>                 act.sa_flags = SA_RESTART;
>
> which means that on any signal != SIGALRM we *always* set
> the SA_RESTART flag to restart slow system calls.
>
> However, inside lib/tevent/tevent_signal.c we have:
>
> struct sigaction act;
>                 ZERO_STRUCT(act);
>                 act.sa_handler = tevent_common_signal_handler;
>                 act.sa_flags = sa_flags;
> ....
> if (sigaction(signum, &act, sig_state->oldact[signum]) == -1) {
>
> where sa_flags is passed in from the caller of
> tevent_add_signal().
>
> All of the callers to tevent_add_signal() I can
> find don't use SA_RESTART flags. They all pass
> zero in the sa_flags parameter, which means that no slow
> system calls will be restarted if the caught signal
> is received.

SA_RESTART is just screwed. Imho the code must deal with EINTR anyway in most if
not all places, so we should just remove it where applicable. The tevent signal
interface allows passing it, but we should not use it in our code.

I only see two callers: tdb_robust_mutex_handler() who doesn't need it and
heimdal.

> In addition, CatchSignal() only returns the old
> act.sa_handler, *not* the content of the old act.sa_flags.
>
> Which means when you call CatchSignal() to temporarily
> replace a signal handler added by tevent_add_signal()
> and then call CatchSignal() to put it back, you
> overwrite any sa_flags that were passed into the
> tevent_add_signal() call (not that there are any
> right now :-), and will always add in SA_RESTART.
>
> So depending on the order of calling CatchSignal()
> or tevent_add_signal() we're going to get different
> signal handling behavior (restart or not) on slow
> system calls.

oh.

> This patch doesn't make it any worse though :-).

ah. *phew* :)

>
> But long run I think we need to remove all
> #ifdef HAVE_SIGACTION tests and just *require*
> sigaction for Samba to run.

yes, abort configure if it's not available on the system.

> Then we need to look at fixing CatchSignal()
> to take a struct sigaction, and return the existing
> struct sigaction instead of void, so it can
> be used inside library code to temporarily
> replace an existing tevent_add_signal() handler
> and correctly replace it.
>
> Eventually we might want to look at removing
> the use of CatchSignal() and varients entirely.

It's a nice utility function where the caller mustn't prepate the sigaction structure.

Maybe change it to take and additional sa_flags and two addtional out args, ie

int CatchSignal(int signum,
                int sa_flags,
                void (*handler)(int),
                int *old_sa_flags,
                void (**old_handler)(int));

Most existing callers would just call CatchSignal(..., NULL, NULL);

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
On Thu, Apr 20, 2017 at 05:05:45PM +0200, Ralph Böhme wrote:
>
> SA_RESTART is just screwed. Imho the code must deal with EINTR anyway in most if
> not all places, so we should just remove it where applicable. The tevent signal
> interface allows passing it, but we should not use it in our code.
>
> I only see two callers: tdb_robust_mutex_handler() who doesn't need it and
> heimdal.

So do you want to remove the automatic adding of
SA_RESTART inside of CatchSignal() ?

> It's a nice utility function where the caller mustn't prepate the sigaction structure.
>
> Maybe change it to take and additional sa_flags and two addtional out args, ie
>
> int CatchSignal(int signum,
>                 int sa_flags,
> void (*handler)(int),
>                 int *old_sa_flags,
> void (**old_handler)(int));
>
> Most existing callers would just call CatchSignal(..., NULL, NULL);

Yep, I like that. Might prepare a patch for review that
fixes this !

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] tfork

Samba - samba-technical mailing list
On Thu, Apr 20, 2017 at 03:23:13PM -0700, Jeremy Allison wrote:

> On Thu, Apr 20, 2017 at 05:05:45PM +0200, Ralph Böhme wrote:
> >
> > SA_RESTART is just screwed. Imho the code must deal with EINTR anyway in most if
> > not all places, so we should just remove it where applicable. The tevent signal
> > interface allows passing it, but we should not use it in our code.
> >
> > I only see two callers: tdb_robust_mutex_handler() who doesn't need it and
> > heimdal.
>
> So do you want to remove the automatic adding of
> SA_RESTART inside of CatchSignal() ?

yes.

> > It's a nice utility function where the caller mustn't prepate the sigaction structure.
> >
> > Maybe change it to take and additional sa_flags and two addtional out args, ie
> >
> > int CatchSignal(int signum,
> >                 int sa_flags,
> > void (*handler)(int),
> >                 int *old_sa_flags,
> > void (**old_handler)(int));
> >
> > Most existing callers would just call CatchSignal(..., NULL, NULL);
>
> Yep, I like that. Might prepare a patch for review that
> fixes this !

:)

-slow

Loading...