[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