[SSSD] [PATCH] Properly set up SIGCHLD handlers
Sumit Bose
sbose at redhat.com
Mon May 3 20:57:41 UTC 2010
On Mon, May 03, 2010 at 09:39:44AM -0400, Stephen Gallagher wrote:
> On 05/03/2010 07:36 AM, Stephen Gallagher wrote:
> >On 05/02/2010 05:19 PM, Stephen Gallagher wrote:
> >>On 05/02/2010 03:37 PM, Stephen Gallagher wrote:
> >>>On 05/02/2010 08:10 AM, Stephen Gallagher wrote:
> >>>>Previously, we could set up more than one handler for the same
> >>>>backend, because the LDAP and Kerberos providers each set one up.
> >>>>
> >>>>See https://fedorahosted.org/sssd/ticket/460 for more detail on the
> >>>>issue this patch addresses.
> >>>>
> >>>>Please review this patch promptly, as it is a dependency for my dyndns
> >>>>patch that I am working on.
> >>>
> >>>Withdrawing this patch. I have a better way (patch coming later today).
> >>>
> >>
> >>Ok, new version of this patch (much more involved).
> >>
> >>Now, instead of setting up a global handler for all children, we will
> >>set up individual handlers for the PIDs we care about.
> >>
> >>This patch also adds functionality to allow setting a callback on the
> >>signal handler, to get the results back.
> >>
> >
> >Sumit spotted a mistake in the patch during review. There were too many
> >arguments being passed to sdap_fork_child().
> >
> >New patch attached.
> >
>
> I found a logic bug. It was only calling the callback when the child
> succeeded. It will now return the errors as well.
>
Nack, with a wrong password the krb5_child is not handled properly
because the SIG_CHLD handler is never called and krb5_child remains in
zombie state.
bye,
Sumit
> --
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> From d1b7c3ab4350c219bf04824a18680a4049090704 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Sun, 2 May 2010 08:07:50 -0400
> Subject: [PATCH] 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 | 123 ++++++++++++++++++++++++++-----
> src/providers/child_common.h | 14 ++++
> src/providers/ipa/ipa_init.c | 15 ----
> src/providers/krb5/krb5_auth.c | 7 ++
> src/providers/krb5/krb5_init.c | 9 --
> src/providers/ldap/sdap_child_helpers.c | 19 ++---
> 6 files changed, 134 insertions(+), 53 deletions(-)
>
> diff --git a/src/providers/child_common.c b/src/providers/child_common.c
> index b9802557709227a0acad8cfdccbca7898a862a27..af1bb004d8c2d2f23526c5c94289c41f0bb0e310 100644
> --- a/src/providers/child_common.c
> +++ b/src/providers/child_common.c
> @@ -33,6 +33,56 @@
> #include "db/sysdb.h"
> #include "providers/child_common.h"
>
> +struct tevent_signal *child_sig = NULL;
> +
> +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(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev, int pid,
> + sss_child_callback_t cb, void *pvt,
> + struct tevent_signal **signal_handler)
> +{
> + struct sss_child_ctx *child_ctx;
> +
> + DEBUG(8, ("Setting up signal handler up for pid [%d]\n", pid));
> +
> + child_ctx = talloc_zero(mem_ctx, struct sss_child_ctx);
> + if (child_ctx == NULL) {
> + talloc_free(child_sig);
> + return ENOMEM;
> + }
> +
> + child_sig = tevent_add_signal(ev, mem_ctx, SIGCHLD, SA_SIGINFO,
> + child_sig_handler, child_ctx);
> + if(!child_sig) {
> + /* Error setting up signal handler */
> + return ENOMEM;
> + }
> +
> + /* Steal the memory for the child context onto the
> + * event handler.
> + */
> + talloc_steal(child_sig, child_ctx);
> +
> + child_ctx->pid = pid;
> + child_ctx->cb = cb;
> + child_ctx->pvt = pvt;
> + child_ctx->sige = child_sig;
> +
> + if (signal_handler) {
> + *signal_handler = child_sig;
> + }
> +
> + 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 +306,72 @@ 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;
>
> - DEBUG(7, ("Waiting for [%d] childeren.\n", count));
> - do {
> - errno = 0;
> - ret = waitpid(-1, &child_status, WNOHANG);
> + if (count <= 0) {
> + DEBUG(0, ("SIGCHLD handler called with invalid child count\n"));
> + return;
> + }
>
> - 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));
> - }
> + child_ctx = talloc_get_type(pvt, struct sss_child_ctx);
> + DEBUG(7, ("Waiting for child [%d].\n", child_ctx->pid));
> +
> + 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 (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));
> + }
> +
> + /* 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;
> }
>
> - --count;
> - } while (count < 0);
> + 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);
> + }
> + else {
> + /* Just destroy the tevent signal */
> + talloc_free(child_ctx->sige);
> + }
> +}
> +
> 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 0b2081d2d2b47284fd1032a17f715fb4c5d51ecc..43cb97b9911918f4a83400158f49bb91864baf0b 100644
> --- a/src/providers/child_common.h
> +++ b/src/providers/child_common.h
> @@ -45,6 +45,20 @@ struct io_buffer {
> size_t size;
> };
>
> +/* Callback to be invoked when a sigchld handler is called.
> + * It is safe (and recommended) to free the tevent_signal *
> + * associated with the handler from this function.
> + */
> +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(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev, int pid,
> + sss_child_callback_t cb, void *pvt,
> + struct tevent_signal **signal_handler);
> +
> /* 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 6e0688f0516e824556cd8e5381a0ee3d8d978b42..bd5a9e03c8bf3af6ba2c8864c9291d92a5efa1d8 100644
> --- a/src/providers/ipa/ipa_init.c
> +++ b/src/providers/ipa/ipa_init.c
> @@ -165,7 +165,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;
> @@ -239,20 +238,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 0c08fe16eb2c2f174715802346fa905d7ceb8ccf..27fe9ced19ff76750fc6555ecf52ff60eb644be6 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -533,6 +533,13 @@ 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(kr, ev, pid,
> + NULL, 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 83129d9c5f294e18522d4ee6bb63ba2f1b453249..9cf5bfac3a898cb8c59570a8bf1c223ca7008b07 100644
> --- a/src/providers/krb5/krb5_init.c
> +++ b/src/providers/krb5/krb5_init.c
> @@ -48,7 +48,6 @@ int sssm_krb5_auth_init(struct be_ctx *bectx,
> {
> struct krb5_ctx *ctx = NULL;
> int ret;
> - struct tevent_signal *sige;
> unsigned v;
> FILE *debug_filep;
> const char *krb5_servers;
> @@ -126,14 +125,6 @@ int sssm_krb5_auth_init(struct be_ctx *bectx,
> goto fail;
> }
>
> - sige = tevent_add_signal(bectx->ev, ctx, SIGCHLD, SA_SIGINFO,
> - child_sig_handler, NULL);
> - if (sige == NULL) {
> - DEBUG(1, ("tevent_add_signal failed.\n"));
> - ret = ENOMEM;
> - goto fail;
> - }
> -
> 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 273fc67873ed6029a4913b96df602dff4a45dd7b..2f525279dabe5a1b243c15abaeb7d19881010866 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,12 @@ 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, ev, pid,
> + NULL, NULL, NULL);
> + if (ret != EOK) {
> + return ret;
> + }
> +
> } else { /* error */
> err = errno;
> DEBUG(1, ("fork failed [%d][%s].\n", err, strerror(err)));
> @@ -275,7 +282,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 +429,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 +438,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) {
> --
> 1.7.0.1
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list