From af971fb6cf853c3a5f41aa00918013903aba1ff3 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Sun, 2 May 2010 08:07:50 -0400 Subject: Properly set up SIGCHLD handlers Instead of having all-purpose SIGCHLD handlers that try to catch every occurrence, we instead create a per-PID handler. This will allow us to specify callbacks to occur when certain children exit. --- src/providers/child_common.c | 110 ++++++++++++++++++++++++++------ src/providers/child_common.h | 12 ++++ src/providers/ipa/ipa_init.c | 15 ----- src/providers/krb5/krb5_auth.c | 6 ++ src/providers/krb5/krb5_init.c | 1 - src/providers/ldap/sdap_child_helpers.c | 18 +++--- 6 files changed, 116 insertions(+), 46 deletions(-) diff --git a/src/providers/child_common.c b/src/providers/child_common.c index b98025577..a8a4e0409 100644 --- a/src/providers/child_common.c +++ b/src/providers/child_common.c @@ -33,6 +33,42 @@ #include "db/sysdb.h" #include "providers/child_common.h" +struct sss_child_ctx { + struct tevent_signal *sige; + pid_t pid; + int child_status; + sss_child_callback_t cb; + void *pvt; +}; + +int child_handler_setup(struct tevent_context *ev, int pid, + sss_child_callback_t cb, void *pvt) +{ + struct sss_child_ctx *child_ctx; + + DEBUG(8, ("Setting up signal handler up for pid [%d]\n", pid)); + + child_ctx = talloc_zero(ev, struct sss_child_ctx); + if (child_ctx == NULL) { + return ENOMEM; + } + + child_ctx->sige = tevent_add_signal(ev, child_ctx, SIGCHLD, SA_SIGINFO, + child_sig_handler, child_ctx); + if(!child_ctx->sige) { + /* Error setting up signal handler */ + talloc_free(child_ctx); + return ENOMEM; + } + + child_ctx->pid = pid; + child_ctx->cb = cb; + child_ctx->pvt = pvt; + + DEBUG(8, ("Signal handler set up for pid [%d]\n", pid)); + return EOK; +} + /* Async communication with the child process via a pipe */ struct write_pipe_state { @@ -256,37 +292,71 @@ void fd_nonblocking(int fd) return; } +static void child_invoke_callback(struct tevent_context *ev, + struct tevent_immediate *imm, + void *pvt); void child_sig_handler(struct tevent_context *ev, struct tevent_signal *sige, int signum, int count, void *__siginfo, void *pvt) { - int ret; - int child_status; + int ret, err; + struct sss_child_ctx *child_ctx; + struct tevent_immediate *imm; + + if (count <= 0) { + DEBUG(0, ("SIGCHLD handler called with invalid child count\n")); + return; + } + + child_ctx = talloc_get_type(pvt, struct sss_child_ctx); + DEBUG(7, ("Waiting for child [%d].\n", child_ctx->pid)); - DEBUG(7, ("Waiting for [%d] childeren.\n", count)); - do { - errno = 0; - ret = waitpid(-1, &child_status, WNOHANG); - - if (ret == -1) { - DEBUG(1, ("waitpid failed [%d][%s].\n", errno, strerror(errno))); - } else if (ret == 0) { - DEBUG(1, ("waitpid did not found a child with changed status.\n")); - } else { - if (WEXITSTATUS(child_status) != 0) { - DEBUG(1, ("child [%d] failed with status [%d].\n", ret, - child_status)); - } else { - DEBUG(4, ("child [%d] finished successful.\n", ret)); - } + errno = 0; + ret = waitpid(child_ctx->pid, &child_ctx->child_status, WNOHANG); + + if (ret == -1) { + err = errno; + DEBUG(1, ("waitpid failed [%d][%s].\n", err, strerror(err))); + } else if (ret == 0) { + DEBUG(1, ("waitpid did not found a child with changed status.\n")); + } else if WIFEXITED(child_ctx->child_status) { + if (WEXITSTATUS(child_ctx->child_status) != 0) { + DEBUG(1, ("child [%d] failed with status [%d].\n", ret, + child_ctx->child_status)); + } else { + DEBUG(4, ("child [%d] finished successfully.\n", ret)); } - --count; - } while (count < 0); + /* Invoke the callback in a tevent_immediate handler + * so that it is safe to free the tevent_signal * + */ + imm = tevent_create_immediate(ev); + if (imm == NULL) { + DEBUG(0, ("Out of memory invoking sig handler callback\n")); + return; + } + + tevent_schedule_immediate(imm, ev,child_invoke_callback, + child_ctx); + } return; } +static void child_invoke_callback(struct tevent_context *ev, + struct tevent_immediate *imm, + void *pvt) +{ + struct sss_child_ctx *child_ctx = + talloc_get_type(pvt, struct sss_child_ctx); + if (child_ctx->cb) { + child_ctx->cb(child_ctx->child_status, child_ctx->sige, child_ctx->pvt); + } + + /* Stop monitoring for this child */ + talloc_free(child_ctx); +} + static errno_t prepare_child_argv(TALLOC_CTX *mem_ctx, int child_debug_fd, const char *binary, diff --git a/src/providers/child_common.h b/src/providers/child_common.h index 0b2081d2d..22a77dbbe 100644 --- a/src/providers/child_common.h +++ b/src/providers/child_common.h @@ -45,6 +45,18 @@ struct io_buffer { size_t size; }; +/* Callback to be invoked when a sigchld handler is called. + * The tevent_signal * associated with the handler will be + * freed automatically when this function returns. + */ +typedef void (*sss_child_callback_t)(int child_status, + struct tevent_signal *sige, + void *pvt); + +/* Set up child termination signal handler */ +int child_handler_setup(struct tevent_context *ev, int pid, + sss_child_callback_t cb, void *pvt); + /* Async communication with the child process via a pipe */ struct tevent_req *write_pipe_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c index d2f9b3dcb..cb178c877 100644 --- a/src/providers/ipa/ipa_init.c +++ b/src/providers/ipa/ipa_init.c @@ -164,7 +164,6 @@ int sssm_ipa_auth_init(struct be_ctx *bectx, struct ipa_auth_ctx *ipa_auth_ctx; struct krb5_ctx *krb5_auth_ctx; struct sdap_auth_ctx *sdap_auth_ctx; - struct tevent_signal *sige; FILE *debug_filep; unsigned v; int ret; @@ -238,20 +237,6 @@ int sssm_ipa_auth_init(struct be_ctx *bectx, goto done; } - if (ipa_options->id_ctx == NULL) { - DEBUG(9, ("Adding SIGCHLD handler for Kerberos child.\n")); - sige = tevent_add_signal(bectx->ev, krb5_auth_ctx, SIGCHLD, SA_SIGINFO, - child_sig_handler, NULL); - if (sige == NULL) { - DEBUG(1, ("tevent_add_signal failed.\n")); - ret = ENOMEM; - goto done; - } - } else { - DEBUG(9, ("IPA id provider already initialized, " - "assuming that a SIGCHLD handler is already in place.\n")); - } - if (debug_to_file != 0) { ret = open_debug_file_ex("krb5_child", &debug_filep); if (ret != EOK) { diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index a3ae39428..e16bc3933 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -467,6 +467,12 @@ static errno_t fork_child(struct tevent_req *req, struct tevent_context *ev, fd_nonblocking(kr->read_from_child_fd); fd_nonblocking(kr->write_to_child_fd); + ret = child_handler_setup(ev, pid, NULL, NULL); + if (ret != EOK) { + DEBUG(1, ("Could not set up child signal handler\n")); + return ret; + } + err = activate_child_timeout_handler(req, ev, kr); if (err != EOK) { DEBUG(1, ("activate_child_timeout_handler failed.\n")); diff --git a/src/providers/krb5/krb5_init.c b/src/providers/krb5/krb5_init.c index 03d952607..0ad589268 100644 --- a/src/providers/krb5/krb5_init.c +++ b/src/providers/krb5/krb5_init.c @@ -141,7 +141,6 @@ int sssm_krb5_auth_init(struct be_ctx *bectx, goto fail; } talloc_steal(sige, sig_realm); - if (debug_to_file != 0) { ret = open_debug_file_ex("krb5_child", &debug_filep); if (ret != EOK) { diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c index 273fc6787..c61f3cceb 100644 --- a/src/providers/ldap/sdap_child_helpers.c +++ b/src/providers/ldap/sdap_child_helpers.c @@ -77,7 +77,8 @@ static int sdap_child_destructor(void *ptr) return 0; } -static errno_t sdap_fork_child(struct sdap_child *child) +static errno_t sdap_fork_child(struct tevent_context *ev, + struct sdap_child *child) { int pipefd_to_child[2]; int pipefd_from_child[2]; @@ -118,6 +119,11 @@ static errno_t sdap_fork_child(struct sdap_child *child) fd_nonblocking(child->read_from_child_fd); fd_nonblocking(child->write_to_child_fd); + ret = child_handler_setup(ev, pid, NULL, NULL); + if (ret != EOK) { + return ret; + } + } else { /* error */ err = errno; DEBUG(1, ("fork failed [%d][%s].\n", err, strerror(err))); @@ -275,7 +281,7 @@ struct tevent_req *sdap_get_tgt_send(TALLOC_CTX *mem_ctx, goto fail; } - ret = sdap_fork_child(state->child); + ret = sdap_fork_child(state->ev, state->child); if (ret != EOK) { DEBUG(1, ("sdap_fork_child failed.\n")); goto fail; @@ -422,7 +428,6 @@ int setup_child(struct sdap_id_ctx *ctx) { int ret; const char *mech; - struct tevent_signal *sige; unsigned v; FILE *debug_filep; @@ -432,13 +437,6 @@ int setup_child(struct sdap_id_ctx *ctx) return EOK; } - sige = tevent_add_signal(ctx->be->ev, ctx, SIGCHLD, SA_SIGINFO, - child_sig_handler, NULL); - if (sige == NULL) { - DEBUG(1, ("tevent_add_signal failed.\n")); - return ENOMEM; - } - if (debug_to_file != 0 && ldap_child_debug_fd == -1) { ret = open_debug_file_ex("ldap_child", &debug_filep); if (ret != EOK) { -- cgit