[PATCH] Terminate forked process if adcli is not installed

[Jakub Hrozek]

Hi,

the attached patches should fix:
    https://fedorahosted.org/sssd/ticket/3016

To reproduce, just rpm -e adcli and then start the ad provider. The bug
reproduces better with a shorter machine renewal timeout.

[Lukas Slebodnik]

On (17/05/16 12:53), Jakub Hrozek wrote:
> Hi,
>
> the attached patches should fix:
>     https://fedorahosted.org/sssd/ticket/3016
>
> To reproduce, just rpm -e adcli and then start the ad provider. The bug
> reproduces better with a shorter machine renewal timeout. 
> From 92e7d4236c0aec2ad323933e5ffc79437b28906e Mon Sep 17 00:00:00
> 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> Date: Tue, 17 May 2016 11:52:00 +0200
> Subject: [PATCH 1/2] UTIL: exit() the forked process if exec()-ing a child
>  process fails
>
> When exec() fails, we should not attempt to continue, but just kill the
> forked process. The patch adds this logic to the exec_child() and
> exec_child_ex() functions to avoid code duplication
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/3016
> ---
>  src/providers/ad/ad_gpo.c                | 14 ++++-----
>  src/providers/ad/ad_machine_pw_renewal.c | 16 +++++-----
>  src/providers/ipa/ipa_selinux.c          | 12 ++++----
>  src/providers/krb5/krb5_child_handler.c  | 16 +++++-----
>  src/providers/ldap/sdap_child_helpers.c  | 12 ++++----
>  src/responder/pam/pamsrv_p11.c           | 14 ++++-----
>  src/tests/cmocka/test_child_common.c     | 51 ++++++++++++++------------------
>  src/util/child_common.c                  | 30 +++++++++----------
>  src/util/child_common.h                  | 16 +++++-----
>  9 files changed, 85 insertions(+), 96 deletions(-)
>
> diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> index 22ac803378d61b8001ea7a842a0c54964614b238..ec2ae28833315fdd439cdd548fa1a90b4ea18993
> 100644
> --- a/src/providers/ad/ad_gpo.c
> +++ b/src/providers/ad/ad_gpo.c
> @@ -4189,13 +4189,13 @@ gpo_fork_child(struct tevent_req *req)
>      pid = fork();
>  
>      if (pid == 0) { /* child */
> -        err = exec_child_ex(state,
> -                            pipefd_to_child, pipefd_from_child,
> -                            GPO_CHILD, gpo_child_debug_fd, NULL, false,
> -                            STDIN_FILENO, AD_GPO_CHILD_OUT_FILENO);
> -        DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
> -              err, strerror(err));
> -        return err;
> +        exec_child_ex(state,
> +                      pipefd_to_child, pipefd_from_child,
> +                      GPO_CHILD, gpo_child_debug_fd, NULL, false,
> +                      STDIN_FILENO, AD_GPO_CHILD_OUT_FILENO);
> +
> +        /* We should never get here */
> +        DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec gpo_child:\n");
>      } else if (pid > 0) { /* parent */
>          state->child_pid = pid;
>          state->io->read_from_child_fd = pipefd_from_child[0];
> diff --git a/src/providers/ad/ad_machine_pw_renewal.c
> b/src/providers/ad/ad_machine_pw_renewal.c
> index 7997fbb0cdaa9490cd4e5c794c9d98e3b892673e..3d79aa0a600233c7269917b0088bdf07204680d8
> 100644
> --- a/src/providers/ad/ad_machine_pw_renewal.c
> +++ b/src/providers/ad/ad_machine_pw_renewal.c
> @@ -174,15 +174,13 @@ ad_machine_account_password_renewal_send(TALLOC_CTX *mem_ctx,
>  
>      child_pid = fork();
>      if (child_pid == 0) { /* child */
> -        ret = exec_child_ex(state, pipefd_to_child, pipefd_from_child,
> -                            renewal_data->prog_path, -1,
> -                            extra_args, true,
> -                            STDIN_FILENO, STDERR_FILENO);
> -        if (ret != EOK) {
> -            DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec renewal child:
> [%d][%s].\n",
> -                                       ret, strerror(ret));
> -            goto done;
> -        }
> +        exec_child_ex(state, pipefd_to_child, pipefd_from_child,
> +                      renewal_data->prog_path, -1,
> +                      extra_args, true,
> +                      STDIN_FILENO, STDERR_FILENO);
> +
> +        /* We should never get here */
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec renewal child\n");
>      } else if (child_pid > 0) { /* parent */
>  
>          state->read_from_child_fd = pipefd_from_child[0];
> diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
> index 3e9efee325f518846dff8b87d4f513b5e5a5ff89..c546d3a99c2fb6f8b2313a87fc82e4d9e0250441
> 100644
> --- a/src/providers/ipa/ipa_selinux.c
> +++ b/src/providers/ipa/ipa_selinux.c
> @@ -1047,12 +1047,12 @@ static errno_t selinux_fork_child(struct selinux_child_state
> *state)
>      pid = fork();
>  
>      if (pid == 0) { /* child */
> -        ret = exec_child(state,
> -                         pipefd_to_child, pipefd_from_child,
> -                         SELINUX_CHILD, selinux_child_debug_fd);
> -        DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec selinux_child:
> [%d][%s].\n",
> -              ret, sss_strerror(ret));
> -        return ret;
> +        exec_child(state,
> +                   pipefd_to_child, pipefd_from_child,
> +                   SELINUX_CHILD, selinux_child_debug_fd);
> +
> +        /* We should never get here */
> +        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]);
> diff --git a/src/providers/krb5/krb5_child_handler.c
> b/src/providers/krb5/krb5_child_handler.c
> index 167a2b2ad09b67908cdce8051d8a37e557c91545..1ca74fcd71e75a49c6afb634c1414f0a71764317
> 100644
> --- a/src/providers/krb5/krb5_child_handler.c
> +++ b/src/providers/krb5/krb5_child_handler.c
> @@ -309,15 +309,13 @@ static errno_t fork_child(struct tevent_req *req)
>      pid = fork();
>  
>      if (pid == 0) { /* child */
> -        err = exec_child_ex(state,
> -                            pipefd_to_child, pipefd_from_child,
> -                            KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd,
> -                            k5c_extra_args, false, STDIN_FILENO, STDOUT_FILENO);
> -        if (err != EOK) {
> -            DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec KRB5 child:
> [%d][%s].\n",
> -                      err, strerror(err));
> -            return err;
> -        }
> +        exec_child_ex(state,
> +                      pipefd_to_child, pipefd_from_child,
> +                      KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd,
> +                      k5c_extra_args, false, STDIN_FILENO, STDOUT_FILENO);
> +
> +        /* We should never get here */
> +        DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec KRB5 child\n");
>      } else if (pid > 0) { /* parent */
>          state->child_pid = pid;
>          state->io->read_from_child_fd = pipefd_from_child[0];
> diff --git a/src/providers/ldap/sdap_child_helpers.c
> b/src/providers/ldap/sdap_child_helpers.c
> index 90330f13ff5d04ad0a779bdef8aad4d856f9afd0..69b470dbf0010cb66275c98894219481dccde80b
> 100644
> --- a/src/providers/ldap/sdap_child_helpers.c
> +++ b/src/providers/ldap/sdap_child_helpers.c
> @@ -96,12 +96,12 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
>      pid = fork();
>  
>      if (pid == 0) { /* child */
> -        err = exec_child(child,
> -                         pipefd_to_child, pipefd_from_child,
> -                         LDAP_CHILD, ldap_child_debug_fd);
> -        DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec LDAP child: [%d][%s].\n",
> -                                    err, strerror(err));
> -        return err;
> +        exec_child(child,
> +                   pipefd_to_child, pipefd_from_child,
> +                   LDAP_CHILD, ldap_child_debug_fd);
> +
> +        /* We should never get here */
> +        DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec LDAP child\n");
>      } else if (pid > 0) { /* parent */
>          child->pid = pid;
>          child->io->read_from_child_fd = pipefd_from_child[0];
> diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
> index 7a8002c2828c14e55ef2d827e37398035a0c6726..d290283de42765565a0b73aee11d6311f2c2095a
> 100644
> --- a/src/responder/pam/pamsrv_p11.c
> +++ b/src/responder/pam/pamsrv_p11.c
> @@ -321,14 +321,12 @@ struct tevent_req *pam_check_cert_send(TALLOC_CTX *mem_ctx,
>  
>      child_pid = fork();
>      if (child_pid == 0) { /* child */
> -        ret = exec_child_ex(state, pipefd_to_child, pipefd_from_child,
> -                            P11_CHILD_PATH, child_debug_fd, extra_args, false,
> -                            STDIN_FILENO, STDOUT_FILENO);
> -        if (ret != EOK) {
> -            DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec p11 child:
> [%d][%s].\n",
> -                                       ret, strerror(ret));
> -            goto done;
> -        }
> +        exec_child_ex(state, pipefd_to_child, pipefd_from_child,
> +                      P11_CHILD_PATH, child_debug_fd, extra_args, false,
> +                      STDIN_FILENO, STDOUT_FILENO);
> +
> +        /* We should never get here */
> +        DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec p11 child\n");
>      } else if (child_pid > 0) { /* parent */
>  
>          state->read_from_child_fd = pipefd_from_child[0];
> diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
> index be842c4f502f134ac54d720abf19669d4b7f8c46..ae696e771159126ed4a4bfe790980ffca181f79f
> 100644
> --- a/src/tests/cmocka/test_child_common.c
> +++ b/src/tests/cmocka/test_child_common.c
> @@ -94,11 +94,10 @@ void test_exec_child(void **state)
>      child_pid = fork();
>      assert_int_not_equal(child_pid, -1);
>      if (child_pid == 0) {
> -        ret = exec_child(child_tctx,
> -                         child_tctx->pipefd_to_child,
> -                         child_tctx->pipefd_from_child,
> -                         CHILD_DIR"/"TEST_BIN, 2);
> -        assert_int_equal(ret, EOK);
> +        exec_child(child_tctx,
> +                   child_tctx->pipefd_to_child,
> +                   child_tctx->pipefd_from_child,
> +                   CHILD_DIR"/"TEST_BIN, 2);
>      } else {
>              do {
>                  errno = 0;
> @@ -166,13 +165,12 @@ static void extra_args_test(struct child_test_ctx *child_tctx,
>      if (child_pid == 0) {
>          debug_timestamps = 1;
>  
> -        ret = exec_child_ex(child_tctx,
> -                            child_tctx->pipefd_to_child,
> -                            child_tctx->pipefd_from_child,
> -                            CHILD_DIR"/"TEST_BIN, 2, extra_args,
> -                            extra_args_only,
> -                            STDIN_FILENO, STDOUT_FILENO);
> -        assert_int_equal(ret, EOK);
> +        exec_child_ex(child_tctx,
> +                      child_tctx->pipefd_to_child,
> +                      child_tctx->pipefd_from_child,
> +                      CHILD_DIR"/"TEST_BIN, 2, extra_args,
> +                      extra_args_only,
> +                      STDIN_FILENO, STDOUT_FILENO);
>      } else {
>              do {
>                  errno = 0;
> @@ -290,11 +288,10 @@ void test_exec_child_handler(void **state)
>      child_pid = fork();
>      assert_int_not_equal(child_pid, -1);
>      if (child_pid == 0) {
> -        ret = exec_child(child_tctx,
> -                         child_tctx->pipefd_to_child,
> -                         child_tctx->pipefd_from_child,
> -                         CHILD_DIR"/"TEST_BIN, 2);
> -        assert_int_equal(ret, EOK);
> +        exec_child(child_tctx,
> +                   child_tctx->pipefd_to_child,
> +                   child_tctx->pipefd_from_child,
> +                   CHILD_DIR"/"TEST_BIN, 2);
>      }
>  
>      ret = child_handler_setup(child_tctx->test_ctx->ev, child_pid,
> @@ -341,12 +338,11 @@ void test_exec_child_echo(void **state)
>      child_pid = fork();
>      assert_int_not_equal(child_pid, -1);
>      if (child_pid == 0) {
> -        ret = exec_child_ex(child_tctx,
> -                            child_tctx->pipefd_to_child,
> -                            child_tctx->pipefd_from_child,
> -                            CHILD_DIR"/"TEST_BIN, 2, NULL, false,
> -                            STDIN_FILENO, 3);
> -        assert_int_equal(ret, EOK);
> +        exec_child_ex(child_tctx,
> +                      child_tctx->pipefd_to_child,
> +                      child_tctx->pipefd_from_child,
> +                      CHILD_DIR"/"TEST_BIN, 2, NULL, false,
> +                      STDIN_FILENO, 3);
>      }
>  
>      DEBUG(SSSDBG_FUNC_DATA, "Forked into %d\n", child_pid);
> @@ -475,11 +471,10 @@ void test_sss_child(void **state)
>      child_pid = fork();
>      assert_int_not_equal(child_pid, -1);
>      if (child_pid == 0) {
> -        ret = exec_child(child_tctx,
> -                         child_tctx->pipefd_to_child,
> -                         child_tctx->pipefd_from_child,
> -                         CHILD_DIR"/"TEST_BIN, 2);
> -        assert_int_equal(ret, EOK);
> +        exec_child(child_tctx,
> +                   child_tctx->pipefd_to_child,
> +                   child_tctx->pipefd_from_child,
> +                   CHILD_DIR"/"TEST_BIN, 2);
>      }
>  
>      ret = sss_child_register(child_tctx, sc_ctx,
> diff --git a/src/util/child_common.c b/src/util/child_common.c
> index 60466c146b5bd9147e9425736072f1ea6ed73663..52b78f01b74d1c7a306e98abf9bcd8a87c247241
> 100644
> --- a/src/util/child_common.c
> +++ b/src/util/child_common.c
> @@ -726,11 +726,11 @@ fail:
>      return ret;
>  }
>  
> -errno_t exec_child_ex(TALLOC_CTX *mem_ctx,
> -                      int *pipefd_to_child, int *pipefd_from_child,
> -                      const char *binary, int debug_fd,
> -                      const char *extra_argv[], bool extra_args_only,
> -                      int child_in_fd, int child_out_fd)
> +void exec_child_ex(TALLOC_CTX *mem_ctx,
> +                   int *pipefd_to_child, int *pipefd_from_child,
> +                   const char *binary, int debug_fd,
> +                   const char *extra_argv[], bool extra_args_only,
> +                   int child_in_fd, int child_out_fd)
>  {
>      int ret;
>      errno_t err;
> @@ -742,7 +742,7 @@ errno_t exec_child_ex(TALLOC_CTX *mem_ctx,
>          err = errno;
>          DEBUG(SSSDBG_CRIT_FAILURE,
>                "dup2 failed [%d][%s].\n", err, strerror(err));
> -        return err;
> +        exit(1); 
Could we use EXIT_FAILURE?

Otherwise nice work.

LS

[Jakub Hrozek]

On Thu, May 19, 2016 at 08:16:36AM +0200, Lukas Slebodnik wrote:
>  Could we use EXIT_FAILURE?
>  
>  Otherwise nice work. 
Sure, new patches are attached.

[Lukas Slebodnik]

On (19/05/16 09:28), Jakub Hrozek wrote:
> On Thu, May 19, 2016 at 08:16:36AM +0200, Lukas Slebodnik wrote:
> > Could we use EXIT_FAILURE?
> > 
> > Otherwise nice work.
>
> Sure, new patches are attached. 
> From 6941f025e6a93c3f4bc13ee5fa24f4724ab3039f Mon Sep 17 00:00:00
> 2001
> From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> Date: Tue, 17 May 2016 11:52:00 +0200
> Subject: [PATCH 1/2] UTIL: exit() the forked process if exec()-ing a child
>  process fails
>  
ACK

CI is still down due to lab maintenance but it passed locally.

LS

[Lukas Slebodnik]

On (19/05/16 13:56), Lukas Slebodnik wrote:
> On (19/05/16 09:28), Jakub Hrozek wrote:
> >On Thu, May 19, 2016 at 08:16:36AM +0200, Lukas Slebodnik wrote:
> >> Could we use EXIT_FAILURE?
> >> 
> >> Otherwise nice work.
> >
> >Sure, new patches are attached.
>
> >From 6941f025e6a93c3f4bc13ee5fa24f4724ab3039f Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> >Date: Tue, 17 May 2016 11:52:00 +0200
> >Subject: [PATCH 1/2] UTIL: exit() the forked process if exec()-ing a child
> > process fails
> >
> ACK
>
> CI is still down due to lab maintenance but it passed locally.
>  
master:
* 030b821b3704771b15f58293e2b1259a2c0fc32f
* de8815aba87d08b6b7ac5d502dcb1755787e0857

LS

[Lukas Slebodnik]

On (19/05/16 14:08), Lukas Slebodnik wrote:
> On (19/05/16 13:56), Lukas Slebodnik wrote:
> >On (19/05/16 09:28), Jakub Hrozek wrote:
> >>On Thu, May 19, 2016 at 08:16:36AM +0200, Lukas Slebodnik wrote:
> >>> Could we use EXIT_FAILURE?
> >>> 
> >>> Otherwise nice work.
> >>
> >>Sure, new patches are attached.
> >
> >>From 6941f025e6a93c3f4bc13ee5fa24f4724ab3039f Mon Sep 17 00:00:00 2001
> >>From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> >>Date: Tue, 17 May 2016 11:52:00 +0200
> >>Subject: [PATCH 1/2] UTIL: exit() the forked process if exec()-ing a child
> >> process fails
> >>
> >ACK
> >
> >CI is still down due to lab maintenance but it passed locally.
> >
>
> master:
> * 030b821b3704771b15f58293e2b1259a2c0fc32f
> * de8815aba87d08b6b7ac5d502dcb1755787e0857
>  
sssd-1-13:
* 1eae9836ab344f6109bfcefdcf5e6b5611616e0b
* 763af7adc2ce2d8fbbb9cb8e6faff9c346ef23d0
* 166c06a0cd867ca516dcaece6ba0e9af486452f1

LS