Do not leak fds in case of failures setting up a child process

Resolves:
    https://fedorahosted.org/sssd/ticket/3006

The handling of open pipes in failure cases was suboptimal. Moreover,
the faulty logic was copied all over the place. This patch introduces
helper macros to:
    - initialize the pipe endpoints to -1
    - close an open pipe fd and set it to -1 afterwards
    - close both ends unless already closed
These macros are used in the child handling code.

The patch also uses child_io_destructor in the p11_child code for safer
fd handling.

Reviewed-by: Petr Cech <pcech@redhat.com>

6 files changed, 93 insertions, 74 deletions

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index ec2ae2883..c22d32c5e 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -4088,8 +4088,7 @@ static void gpo_cse_step(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->write_to_child_fd);
-    state->io->write_to_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->write_to_child_fd);
 
     subreq = read_pipe_send(state, state->ev, state->io->read_from_child_fd);
 
@@ -4119,8 +4118,7 @@ static void gpo_cse_done(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->read_from_child_fd);
-    state->io->read_from_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->read_from_child_fd);
 
     ret = ad_gpo_parse_gpo_child_response(state->buf, state->len,
                                           &sysvol_gpt_version, &child_result);
@@ -4162,28 +4160,27 @@ int ad_gpo_process_cse_recv(struct tevent_req *req)
 static errno_t
 gpo_fork_child(struct tevent_req *req)
 {
-    int pipefd_to_child[2];
-    int pipefd_from_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
+    int pipefd_from_child[2] = PIPE_INIT;
     pid_t pid;
-    int ret;
-    errno_t err;
+    errno_t ret;
     struct ad_gpo_process_cse_state *state;
 
     state = tevent_req_data(req, struct ad_gpo_process_cse_state);
 
     ret = pipe(pipefd_from_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+        goto fail;
     }
     ret = pipe(pipefd_to_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+        goto fail;
     }
 
     pid = fork();
@@ -4199,9 +4196,9 @@ gpo_fork_child(struct tevent_req *req)
     } else if (pid > 0) { /* parent */
         state->child_pid = pid;
         state->io->read_from_child_fd = pipefd_from_child[0];
-        close(pipefd_from_child[1]);
+        PIPE_FD_CLOSE(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
         sss_fd_nonblocking(state->io->read_from_child_fd);
         sss_fd_nonblocking(state->io->write_to_child_fd);
 
@@ -4209,16 +4206,21 @@ gpo_fork_child(struct tevent_req *req)
         if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "Could not set up child signal handler\n");
-            return ret;
+            goto fail;
         }
     } else { /* error */
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "fork failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+        goto fail;
     }
 
     return EOK;
+
+fail:
+    PIPE_CLOSE(pipefd_from_child);
+    PIPE_CLOSE(pipefd_to_child);
+    return ret;
 }
 
 struct ad_gpo_get_sd_referral_state {
diff --git a/src/providers/ad/ad_machine_pw_renewal.c b/src/providers/ad/ad_machine_pw_renewal.c
index 205e623e0..6b8fe9c8a 100644
--- a/src/providers/ad/ad_machine_pw_renewal.c
+++ b/src/providers/ad/ad_machine_pw_renewal.c
@@ -123,8 +123,8 @@ ad_machine_account_password_renewal_send(TALLOC_CTX *mem_ctx,
     struct tevent_req *subreq;
     pid_t child_pid;
     struct timeval tv;
-    int pipefd_to_child[2];
-    int pipefd_from_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
+    int pipefd_from_child[2] = PIPE_INIT;
     int ret;
     const char **extra_args;
     const char *server_name;
@@ -190,11 +190,11 @@ ad_machine_account_password_renewal_send(TALLOC_CTX *mem_ctx,
     } else if (child_pid > 0) { /* parent */
 
         state->io->read_from_child_fd = pipefd_from_child[0];
-        close(pipefd_from_child[1]);
+        PIPE_FD_CLOSE(pipefd_from_child[1]);
         sss_fd_nonblocking(state->io->read_from_child_fd);
 
         state->io->write_to_child_fd = pipefd_to_child[1];
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
         sss_fd_nonblocking(state->io->write_to_child_fd);
 
         /* Set up SIGCHLD handler */
@@ -239,6 +239,8 @@ ad_machine_account_password_renewal_send(TALLOC_CTX *mem_ctx,
 
 done:
     if (ret != EOK) {
+        PIPE_CLOSE(pipefd_from_child);
+        PIPE_CLOSE(pipefd_to_child);
         tevent_req_error(req, ret);
         tevent_req_post(req, ev);
     }
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index f717bf6cd..b12cd514c 100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -781,6 +781,7 @@ nsupdate_child_send(TALLOC_CTX *mem_ctx,
 
     req = tevent_req_create(mem_ctx, &state, struct nsupdate_child_state);
     if (req == NULL) {
+        close(pipefd_to_child);
         return NULL;
     }
     state->pipefd_to_child = pipefd_to_child;
@@ -864,8 +865,7 @@ nsupdate_child_stdin_done(struct tevent_req *subreq)
         return;
     }
 
-    close(state->pipefd_to_child);
-    state->pipefd_to_child = -1;
+    PIPE_FD_CLOSE(state->pipefd_to_child);
 
     /* Now either wait for the timeout to fire or the child
      * to finish
@@ -909,6 +909,8 @@ nsupdate_child_recv(struct tevent_req *req, int *child_status)
 
     *child_status = state->child_status;
 
+    PIPE_FD_CLOSE(state->pipefd_to_child);
+
     TEVENT_REQ_RETURN_ON_ERROR(req);
 
     return ERR_OK;
@@ -932,7 +934,7 @@ struct tevent_req *be_nsupdate_send(TALLOC_CTX *mem_ctx,
                                     char *nsupdate_msg,
                                     bool force_tcp)
 {
-    int pipefd_to_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
     pid_t child_pid;
     errno_t ret;
     struct tevent_req *req = NULL;
@@ -958,7 +960,7 @@ struct tevent_req *be_nsupdate_send(TALLOC_CTX *mem_ctx,
     child_pid = fork();
 
     if (child_pid == 0) { /* child */
-        close(pipefd_to_child[1]);
+        PIPE_FD_CLOSE(pipefd_to_child[1]);
         ret = dup2(pipefd_to_child[0], STDIN_FILENO);
         if (ret == -1) {
             ret = errno;
@@ -991,8 +993,11 @@ struct tevent_req *be_nsupdate_send(TALLOC_CTX *mem_ctx,
         DEBUG(SSSDBG_CRIT_FAILURE, "execv failed [%d][%s].\n", ret, strerror(ret));
         goto done;
     } else if (child_pid > 0) { /* parent */
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
 
+        /* the nsupdate_child request now owns the pipefd and is responsible
+         * for closing it
+         */
         subreq = nsupdate_child_send(state, ev, pipefd_to_child[1],
                                      child_pid, nsupdate_msg);
         if (subreq == NULL) {
@@ -1010,6 +1015,7 @@ struct tevent_req *be_nsupdate_send(TALLOC_CTX *mem_ctx,
     ret = EOK;
 done:
     if (ret != EOK) {
+        PIPE_CLOSE(pipefd_to_child);
         tevent_req_error(req, ret);
         tevent_req_post(req, ev);
     }
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index c546d3a99..13eabc00b 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -1023,8 +1023,8 @@ static errno_t selinux_child_create_buffer(struct selinux_child_state *state)
 
 static errno_t selinux_fork_child(struct selinux_child_state *state)
 {
-    int pipefd_to_child[2];
-    int pipefd_from_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
+    int pipefd_from_child[2] = PIPE_INIT;
     pid_t pid;
     errno_t ret;
 
@@ -1033,7 +1033,7 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
         ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, sss_strerror(errno));
-        return ret;
+        goto fail;
     }
 
     ret = pipe(pipefd_to_child);
@@ -1041,7 +1041,7 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
         ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, sss_strerror(errno));
-        return ret;
+        goto fail;
     }
 
     pid = fork();
@@ -1055,9 +1055,9 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
         DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec selinux_child\n");
     } else if (pid > 0) { /* parent */
         state->io->read_from_child_fd = pipefd_from_child[0];
-        close(pipefd_from_child[1]);
+        PIPE_FD_CLOSE(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
         sss_fd_nonblocking(state->io->read_from_child_fd);
         sss_fd_nonblocking(state->io->write_to_child_fd);
 
@@ -1065,16 +1065,21 @@ static errno_t selinux_fork_child(struct selinux_child_state *state)
         if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "Could not set up child signal handler\n");
-            return ret;
+            goto fail;
         }
     } else { /* error */
         ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "fork failed [%d][%s].\n", errno, sss_strerror(errno));
-        return ret;
+        goto fail;
     }
 
     return EOK;
+
+fail:
+    PIPE_CLOSE(pipefd_from_child);
+    PIPE_CLOSE(pipefd_to_child);
+    return ret;
 }
 
 static void selinux_child_step(struct tevent_req *subreq)
@@ -1094,8 +1099,7 @@ static void selinux_child_step(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->write_to_child_fd);
-    state->io->write_to_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->write_to_child_fd);
 
     subreq = read_pipe_send(state, state->ev, state->io->read_from_child_fd);
     if (subreq == NULL) {
@@ -1124,8 +1128,7 @@ static void selinux_child_done(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->read_from_child_fd);
-    state->io->read_from_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->read_from_child_fd);
 
     ret = selinux_child_parse_response(buf, len, &child_result);
     if (ret != EOK) {
diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c
index 1ca74fcd7..09a1e5f59 100644
--- a/src/providers/krb5/krb5_child_handler.c
+++ b/src/providers/krb5/krb5_child_handler.c
@@ -275,11 +275,10 @@ static errno_t activate_child_timeout_handler(struct tevent_req *req,
 
 static errno_t fork_child(struct tevent_req *req)
 {
-    int pipefd_to_child[2];
-    int pipefd_from_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
+    int pipefd_from_child[2] = PIPE_INIT;
     pid_t pid;
-    int ret;
-    errno_t err;
+    errno_t ret;
     struct handle_child_state *state = tevent_req_data(req,
                                                      struct handle_child_state);
     const char *k5c_extra_args[3];
@@ -293,17 +292,17 @@ static errno_t fork_child(struct tevent_req *req)
 
     ret = pipe(pipefd_from_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+        goto fail;
     }
     ret = pipe(pipefd_to_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
               "pipe failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+        goto fail;
     }
 
     pid = fork();
@@ -319,9 +318,9 @@ static errno_t fork_child(struct tevent_req *req)
     } else if (pid > 0) { /* parent */
         state->child_pid = pid;
         state->io->read_from_child_fd = pipefd_from_child[0];
-        close(pipefd_from_child[1]);
+        PIPE_FD_CLOSE(pipefd_from_child[1]);
         state->io->write_to_child_fd = pipefd_to_child[1];
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
         sss_fd_nonblocking(state->io->read_from_child_fd);
         sss_fd_nonblocking(state->io->write_to_child_fd);
 
@@ -329,24 +328,29 @@ static errno_t fork_child(struct tevent_req *req)
         if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "Could not set up child signal handler\n");
-            return ret;
+            goto fail;
         }
 
-        err = activate_child_timeout_handler(req, state->ev,
+        ret = activate_child_timeout_handler(req, state->ev,
                   dp_opt_get_int(state->kr->krb5_ctx->opts, KRB5_AUTH_TIMEOUT));
-        if (err != EOK) {
+        if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "activate_child_timeout_handler failed.\n");
         }
 
     } else { /* error */
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
-              "fork failed [%d][%s].\n", errno, strerror(errno));
-        return err;
+              "fork failed [%d][%s].\n", errno, strerror(ret));
+        goto fail;
     }
 
     return EOK;
+
+fail:
+    PIPE_CLOSE(pipefd_from_child);
+    PIPE_CLOSE(pipefd_to_child);
+    return ret;
 }
 
 static void handle_child_step(struct tevent_req *subreq);
@@ -426,8 +430,7 @@ static void handle_child_step(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->write_to_child_fd);
-    state->io->write_to_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->write_to_child_fd);
 
     subreq = read_pipe_send(state, state->ev, state->io->read_from_child_fd);
     if (!subreq) {
@@ -454,8 +457,7 @@ static void handle_child_done(struct tevent_req *subreq)
         return;
     }
 
-    close(state->io->read_from_child_fd);
-    state->io->read_from_child_fd = -1;
+    PIPE_FD_CLOSE(state->io->read_from_child_fd);
 
     tevent_req_done(req);
     return;
diff --git a/src/providers/ldap/sdap_child_helpers.c b/src/providers/ldap/sdap_child_helpers.c
index 69b470dbf..92642e8e4 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -72,25 +72,24 @@ static void sdap_close_fd(int *fd)
 static errno_t sdap_fork_child(struct tevent_context *ev,
                                struct sdap_child *child)
 {
-    int pipefd_to_child[2];
-    int pipefd_from_child[2];
+    int pipefd_to_child[2] = PIPE_INIT;
+    int pipefd_from_child[2] = PIPE_INIT;
     pid_t pid;
-    int ret;
-    errno_t err;
+    errno_t ret;
 
     ret = pipe(pipefd_from_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
-              "pipe failed [%d][%s].\n", err, strerror(err));
-        return err;
+              "pipe failed [%d][%s].\n", ret, strerror(ret));
+        goto fail;
     }
     ret = pipe(pipefd_to_child);
     if (ret == -1) {
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
-              "pipe failed [%d][%s].\n", err, strerror(err));
-        return err;
+              "pipe failed [%d][%s].\n", ret, strerror(ret));
+        goto fail;
     }
 
     pid = fork();
@@ -105,25 +104,30 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
     } else if (pid > 0) { /* parent */
         child->pid = pid;
         child->io->read_from_child_fd = pipefd_from_child[0];
-        close(pipefd_from_child[1]);
+        PIPE_FD_CLOSE(pipefd_from_child[1]);
         child->io->write_to_child_fd = pipefd_to_child[1];
-        close(pipefd_to_child[0]);
+        PIPE_FD_CLOSE(pipefd_to_child[0]);
         sss_fd_nonblocking(child->io->read_from_child_fd);
         sss_fd_nonblocking(child->io->write_to_child_fd);
 
         ret = child_handler_setup(ev, pid, NULL, NULL, NULL);
         if (ret != EOK) {
-            return ret;
+            goto fail;
         }
 
     } else { /* error */
-        err = errno;
+        ret = errno;
         DEBUG(SSSDBG_CRIT_FAILURE,
-              "fork failed [%d][%s].\n", err, strerror(err));
-        return err;
+              "fork failed [%d][%s].\n", ret, strerror(ret));
+        goto fail;
     }
 
     return EOK;
+
+fail:
+    PIPE_CLOSE(pipefd_from_child);
+    PIPE_CLOSE(pipefd_to_child);
+    return ret;
 }
 
 static errno_t create_tgt_req_send_buffer(TALLOC_CTX *mem_ctx,