[SCM] Samba Shared Repository - branch master updated

Andrew Bartlett abartlet at samba.org
Tue Mar 17 00:06:04 MDT 2015


The branch, master has been updated
       via  143e7d3 lib/util: Make ECHILD in samba_runcmd_io_handler an error
       via  30e0238 s4-process_standard: Remove signal(SIGCHLD, SIG_IGN)
      from  f212143 selftest: rename env member to nt4_member

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


- Log -----------------------------------------------------------------
commit 143e7d362797dcb2f92de9231966bf63b604e02e
Author: Andrew Bartlett <abartlet at samba.org>
Date:   Thu Feb 19 12:41:34 2015 +1300

    lib/util: Make ECHILD in samba_runcmd_io_handler an error
    
    We now print out a nasty message and set and error if we get ECHILD,
    as we no longer set SIGIGN on SIGCHLD in the standard process model.
    
    This was why samba_kcc was able to fail totally without us noticing.
    
    Signed-off-by: Andrew Bartlett <abartlet at samba.org>
    Reviewed-by: Jelmer Vernooij <jelmer at samba.org>
    
    Autobuild-User(master): Andrew Bartlett <abartlet at samba.org>
    Autobuild-Date(master): Tue Mar 17 07:05:43 CET 2015 on sn-devel-104

commit 30e0238646bc3a891a67dea57baccea8afc1af43
Author: Andrew Bartlett <abartlet at samba.org>
Date:   Thu Feb 19 12:45:31 2015 +1300

    s4-process_standard: Remove signal(SIGCHLD, SIG_IGN)
    
    We replace this with a pipe between parent and child, and then watch
    for a read event in the parent to indicate that the child has gone away.
    
    The removal of signal(SIGCHLD, SIG_IGN) requires us to then call
    waitpid().  We can't do that in a main loop as we want to get the exit
    status to the legitimate waitpid calls in routines like
    samba_runcmd_*().
    
    Signed-off-by: Andrew Bartlett <abartlet at samba.org>
    Reviewed-by: Stefan Metzmacher <metze at samba.org>

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

Summary of changes:
 lib/util/util_runcmd.c          |   6 +-
 source4/smbd/process_standard.c | 176 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 179 insertions(+), 3 deletions(-)


Changeset truncated at 500 lines:

diff --git a/lib/util/util_runcmd.c b/lib/util/util_runcmd.c
index 1ec717f..46c455d 100644
--- a/lib/util/util_runcmd.c
+++ b/lib/util/util_runcmd.c
@@ -288,7 +288,11 @@ static void samba_runcmd_io_handler(struct tevent_context *ev,
 					   SIGCHLD in the standard
 					   process model.
 					*/
-					tevent_req_done(req);
+					DEBUG(0, ("Error in waitpid() unexpectedly got ECHILD "
+						  "for %s child %d - %s, "
+						  "someone has set SIGCHLD to SIG_IGN!\n",
+					  state->arg0, state->pid, strerror(errno)));
+					tevent_req_error(req, errno);
 					return;
 				}
 				DEBUG(0,("Error in waitpid() for child %s - %s \n",
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index c5377b3..d3622f9 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -29,6 +29,14 @@
 #include "param/param.h"
 #include "ldb_wrap.h"
 
+struct standard_child_state {
+	const char *name;
+	pid_t pid;
+	int to_parent_fd;
+	int from_child_fd;
+	struct tevent_fd *from_child_fde;
+};
+
 NTSTATUS process_model_standard_init(void);
 
 /* we hold a pipe open in the parent, and the any child
@@ -42,11 +50,10 @@ static int child_pipe[2];
 static void standard_model_init(void)
 {
 	pipe(child_pipe);
-	signal(SIGCHLD, SIG_IGN);
 }
 
 /*
-  handle EOF on the child pipe
+  handle EOF on the parent-to-all-children pipe in the child
 */
 static void standard_pipe_handler(struct tevent_context *event_ctx, struct tevent_fd *fde, 
 				  uint16_t flags, void *private_data)
@@ -56,6 +63,132 @@ static void standard_pipe_handler(struct tevent_context *event_ctx, struct teven
 }
 
 /*
+  handle EOF on the child pipe in the parent, so we know when a
+  process terminates without using SIGCHLD or waiting on all possible pids.
+
+  We need to ensure we do not ignore SIGCHLD because we need it to
+  work to get a valid error code from samba_runcmd_*().
+ */
+static void standard_child_pipe_handler(struct tevent_context *ev,
+					struct tevent_fd *fde,
+					uint16_t flags,
+					void *private_data)
+{
+	struct standard_child_state *state
+		= talloc_get_type_abort(private_data, struct standard_child_state);
+	int status = 0;
+	pid_t pid;
+
+	/* the child has closed the pipe, assume its dead */
+	errno = 0;
+	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 child %d (%s) - %s, someone has set SIGCHLD "
+				  "to SIG_IGN!\n",
+				  state->pid, state->name, strerror(errno)));
+			TALLOC_FREE(state);
+			return;
+		}
+		DEBUG(0, ("Error in waitpid() for child %d (%s) - %s \n",
+			  state->pid, state->name, strerror(errno)));
+		if (errno == 0) {
+			errno = ECHILD;
+		}
+		TALLOC_FREE(state);
+		return;
+	}
+	if (WIFEXITED(status)) {
+		status = WEXITSTATUS(status);
+		DEBUG(2, ("Child %d (%s) exited with status %d\n",
+			  state->pid, state->name, status));
+	} else if (WIFSIGNALED(status)) {
+		status = WTERMSIG(status);
+		DEBUG(0, ("Child %d (%s) terminated with signal %d\n",
+			  state->pid, state->name, status));
+	}
+	TALLOC_FREE(state);
+	return;
+}
+
+static struct standard_child_state *setup_standard_child_pipe(struct tevent_context *ev,
+							      const char *name)
+{
+	struct standard_child_state *state;
+	int parent_child_pipe[2];
+	int ret;
+
+	/*
+	 * Prepare a pipe to allow us to know when the child exits,
+	 * because it will trigger a read event on this private
+	 * pipe.
+	 *
+	 * We do all this before the accept and fork(), so we can
+	 * clean up if it fails.
+	 */
+	state = talloc_zero(ev, struct standard_child_state);
+	if (state == NULL) {
+		return NULL;
+	}
+
+	if (name == NULL) {
+		name = "";
+	}
+
+	state->name = talloc_strdup(state, name);
+	if (state->name == NULL) {
+		TALLOC_FREE(state);
+		return NULL;
+	}
+
+	ret = pipe(parent_child_pipe);
+	if (ret == -1) {
+		DEBUG(0, ("Failed to create parent-child pipe to handle "
+			  "SIGCHLD to track new process for socket\n"));
+		TALLOC_FREE(state);
+		return NULL;
+	}
+
+	smb_set_close_on_exec(parent_child_pipe[0]);
+	smb_set_close_on_exec(parent_child_pipe[1]);
+
+	state->from_child_fd = parent_child_pipe[0];
+	state->to_parent_fd = parent_child_pipe[1];
+
+	/*
+	 * The basic purpose of calling this handler is to ensure we
+	 * call waitpid() and so avoid zombies (now that we no longer
+	 * user SIGIGN on for SIGCHLD), but it also allows us to clean
+	 * up other resources in the future.
+	 */
+	state->from_child_fde = tevent_add_fd(ev, state,
+					      state->from_child_fd,
+					      TEVENT_FD_READ,
+					      standard_child_pipe_handler,
+					      state);
+	if (state->from_child_fde == NULL) {
+		TALLOC_FREE(state);
+		return NULL;
+	}
+	tevent_fd_set_auto_close(state->from_child_fde);
+
+	return state;
+}
+
+/*
   called when a listening socket becomes readable. 
 */
 static void standard_accept_connection(struct tevent_context *ev, 
@@ -70,6 +203,12 @@ static void standard_accept_connection(struct tevent_context *ev,
 	struct socket_context *sock2;
 	pid_t pid;
 	struct socket_address *c, *s;
+	struct standard_child_state *state;
+
+	state = setup_standard_child_pipe(ev, NULL);
+	if (state == NULL) {
+		return;
+	}
 
 	/* accept an incoming connection. */
 	status = socket_accept(sock, &sock2);
@@ -79,18 +218,33 @@ static void standard_accept_connection(struct tevent_context *ev,
 		/* this looks strange, but is correct. We need to throttle things until
 		   the system clears enough resources to handle this new socket */
 		sleep(1);
+		close(state->to_parent_fd);
+		state->to_parent_fd = -1;
+		TALLOC_FREE(state);
 		return;
 	}
 
 	pid = fork();
 
 	if (pid != 0) {
+		close(state->to_parent_fd);
+		state->to_parent_fd = -1;
+
+		if (pid > 0) {
+			state->pid = pid;
+		} else {
+			TALLOC_FREE(state);
+		}
+
 		/* parent or error code ... */
 		talloc_free(sock2);
 		/* go back to the event loop */
 		return;
 	}
 
+	/* this leaves state->to_parent_fd open */
+	TALLOC_FREE(state);
+
 	pid = getpid();
 
 	/* This is now the child code. We need a completely new event_context to work with */
@@ -149,14 +303,32 @@ static void standard_new_task(struct tevent_context *ev,
 			      void *private_data)
 {
 	pid_t pid;
+	struct standard_child_state *state;
+
+	state = setup_standard_child_pipe(ev, service_name);
+	if (state == NULL) {
+		return;
+	}
 
 	pid = fork();
 
 	if (pid != 0) {
+		close(state->to_parent_fd);
+		state->to_parent_fd = -1;
+
+		if (pid > 0) {
+			state->pid = pid;
+		} else {
+			TALLOC_FREE(state);
+		}
+
 		/* parent or error code ... go back to the event loop */
 		return;
 	}
 
+	/* this leaves state->to_parent_fd open */
+	TALLOC_FREE(state);
+
 	pid = getpid();
 
 	/* this will free all the listening sockets and all state that


-- 
Samba Shared Repository


More information about the samba-cvs mailing list