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.

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) {