Process hierarchy on a DC?
Ralph Böhme
slow at samba.org
Fri Apr 28 10:06:33 UTC 2017
On Thu, Apr 27, 2017 at 02:39:21PM -0700, Jeremy Allison wrote:
> Much though I hate to revert, I think this might be a good case
> for it temporarily.
two patchsets attached:
1) revert the use of tfork in samba_runcmd_send(). This restores the process
hierarchy and stopping samba by sending a single SIGTERM
2) tfork fix: the first commit in the second patchset fixes the "can't kill all
samba processes by sending the main samba process a sigterm" (the other are just
cleanup and a fix on top)
The problem with 2 is that it leaves us with a process hierarchy like this:
PPID PID PGID SID TTY TPGID STAT UID TIME COMMAND
1 516 510 510 ? -1 S 111 0:02 avahi-daemon: running [samba-ad.local]
1 29209 29209 29209 ? -1 Ss 0 0:00 ./bin/samba
29209 29210 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29211 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29213 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29215 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29216 29209 29209 ? -1 R 0 0:00 \_ ./bin/samba
29209 29217 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29218 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29220 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29221 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29222 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29223 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29224 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
29209 29225 29209 29209 ? -1 S 0 0:00 \_ ./bin/samba
1 29214 29209 29209 ? -1 S 0 0:00 ./bin/samba
29214 29219 29219 29219 ? -1 Ss 0 0:00 \_ /home/slow/git/samba/scratch/bin/smbd -D --option=server role check:inhibit=yes --foreground
29219 29236 29219 29219 ? -1 S 0 0:00 \_ /home/slow/git/samba/scratch/bin/smbd -D --option=server role check:inhibit=yes --foreground
29219 29237 29219 29219 ? -1 S 0 0:00 \_ /home/slow/git/samba/scratch/bin/smbd -D --option=server role check:inhibit=yes --foreground
29219 29238 29219 29219 ? -1 S 0 0:00 \_ /home/slow/git/samba/scratch/bin/smbd -D --option=server role check:inhibit=yes --foreground
1 29228 29209 29209 ? -1 S 0 0:00 ./bin/samba
29228 29230 29230 29230 ? -1 Ss 0 0:00 \_ /home/slow/git/samba/scratch/bin/winbindd -D --option=server role check:inhibit=yes --foreground
29230 29239 29230 29230 ? -1 S 0 0:00 \_ /home/slow/git/samba/scratch/bin/winbindd -D --option=server role check:inhibit=yes --foreground
kill 29209 would work though (thanks to smbd and winbindd watching out for EOF
on stdin).
If this process hierarchy is a problem then I guess we need two functions in the
future:
- apply the revert and rename samba_runcmd_send (and the callers) to something
like samba_runchld_send()
- re-add a samba_runcmd_send() that uses tfork
samba_runchld_send() would be used for starting long lived processes where you
care about the process hierarchy, preserving the problematic behaviour that you
have to correctly handle SIGCHLD yourself. So just the existing behaviour.
samba_runcmd_send() would be used for starting short lived processes or when you
just don't care about the process hierarchy and you wan't to be sure that the
command you run doesn't generate a SIGCHLD
Guess this needs some more reasoning about, so please apply the revert for now
to undo the damage done. Sorry for the hassle!
-slow
-------------- next part --------------
From 06d57702f8aba1109f13a195b18d2f3ecff069b1 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 28 Apr 2017 11:33:24 +0200
Subject: [PATCH] Revert "lib/util: make use of tfork in samba_runcmd_send()"
This reverts commit 292e46ab12d8ec172c9d3b26330d8d6028a1d5a5.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/util/util_runcmd.c | 103 ++++++++++++++++++++++---------------------------
lib/util/util_runcmd.h | 3 +-
2 files changed, 48 insertions(+), 58 deletions(-)
diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index 7a2e1c6..9264cbb 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -29,8 +29,6 @@
#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)
{
@@ -108,7 +106,7 @@ struct tevent_req *samba_runcmd_send(TALLOC_CTX *mem_ctx,
return tevent_req_post(req, ev);
}
- state->pid = tfork(&state->fd_status, NULL);
+ state->pid = fork();
if (state->pid == (pid_t)-1) {
close(p1[0]);
close(p1[1]);
@@ -132,12 +130,10 @@ 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);
@@ -149,7 +145,6 @@ 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);
@@ -160,26 +155,11 @@ 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);
}
@@ -251,10 +231,6 @@ 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;
@@ -262,39 +238,12 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
level = state->stderr_log_level;
fd = state->fd_stderr;
} else {
- 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 (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;
}
+ if (!(flags & TEVENT_FD_READ)) {
+ return;
+ }
n = read(fd, &state->buf[state->buf_used],
sizeof(state->buf) - state->buf_used);
@@ -304,11 +253,53 @@ 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 a887658..26fdbc6 100644
--- a/lib/util/util_runcmd.h
+++ b/lib/util/util_runcmd.h
@@ -27,8 +27,7 @@ struct samba_runcmd_state {
int stderr_log_level;
struct tevent_fd *fde_stdout;
struct tevent_fd *fde_stderr;
- struct tevent_fd *fde_status;
- int fd_stdin, fd_stdout, fd_stderr, fd_status;
+ int fd_stdin, fd_stdout, fd_stderr;
char *arg0;
pid_t pid;
char buf[1024];
--
2.9.3
-------------- next part --------------
From 5b9c9deb13033c12f816efaeb7a6847ef57ae318 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 26 Apr 2017 00:48:39 +0200
Subject: [PATCH 1/3] lib/util/tfork: correct handling of broken pipe
The caller might exit or for some other reason close the pipe. Catch
this case and do a normal exit. This prevents triggering panic handlers
which would cause:
- this "watch" process to hang
- all "watched" processes to become zombies and be left around
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/util/tfork.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index 37c00e6..cc338f0 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -165,7 +165,12 @@ static pid_t level2_fork_and_wait(int child_ready_fd)
written = sys_write(fd, &status, sizeof(status));
if (written != sizeof(status)) {
- abort();
+ /*
+ * The caller might have closed the pipe or exitted.
+ */
+ if (errno != EPIPE) {
+ abort();
+ }
}
_exit(0);
--
2.9.3
From 958abef688c28953c2f2fdcf78d4faf992f2b939 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 26 Apr 2017 01:44:41 +0200
Subject: [PATCH 2/3] lib/util/tfork: rework EINTR loop around dup2
No change in behaviour, just a simpler loop, less indentation.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/util/tfork.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index cc338f0..0ab91ef 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -131,24 +131,20 @@ static pid_t level2_fork_and_wait(int child_ready_fd)
* 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.
*/
- while (true) {
+ do {
fd = dup2(tfork_global->status_pipe[1], 0);
- if (fd == -1) {
- if (errno == EINTR) {
- continue;
- }
- status = errno;
+ } while (fd == -1 && errno == EINTR);
+ if (fd == -1) {
+ status = errno;
- kill(tfork_global->level3_pid, SIGKILL);
+ kill(tfork_global->level3_pid, SIGKILL);
- written = sys_write(tfork_global->status_pipe[1],
- &status, sizeof(status));
- if (written != sizeof(status)) {
- abort();
- }
- _exit(0);
+ written = sys_write(tfork_global->status_pipe[1],
+ &status, sizeof(status));
+ if (written != sizeof(status)) {
+ abort();
}
- break;
+ _exit(0);
}
closefrom(1);
--
2.9.3
From 516abc1644e6dc8d10f09d25d3242f12eaa60e6a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 26 Apr 2017 01:47:21 +0200
Subject: [PATCH 3/3] lib/util/tfork: don't dup if status_pipe[1] is already on
stdin
In theory tfork_global->status_pipe[1] could well be fd 0, so we'd be
calling dup2(0, 0) which would return EINVAL.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/util/tfork.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/util/tfork.c b/lib/util/tfork.c
index 0ab91ef..2d485ce 100644
--- a/lib/util/tfork.c
+++ b/lib/util/tfork.c
@@ -131,20 +131,22 @@ static pid_t level2_fork_and_wait(int child_ready_fd)
* 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.
*/
- do {
- fd = dup2(tfork_global->status_pipe[1], 0);
- } while (fd == -1 && errno == EINTR);
- if (fd == -1) {
- status = errno;
+ if (tfork_global->status_pipe[1] > 0) {
+ do {
+ fd = dup2(tfork_global->status_pipe[1], 0);
+ } while (fd == -1 && errno == EINTR);
+ if (fd == -1) {
+ status = errno;
- kill(tfork_global->level3_pid, SIGKILL);
+ kill(tfork_global->level3_pid, SIGKILL);
- written = sys_write(tfork_global->status_pipe[1],
- &status, sizeof(status));
- if (written != sizeof(status)) {
- abort();
+ written = sys_write(tfork_global->status_pipe[1],
+ &status, sizeof(status));
+ if (written != sizeof(status)) {
+ abort();
+ }
+ _exit(0);
}
- _exit(0);
}
closefrom(1);
--
2.9.3
More information about the samba-technical
mailing list