Re: [RFC PATCH 2/4] util: Fix deadlock across fork()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The commit looks good to me, but I'ld rather have Dan have a look at it.
I'm not expert enough to tell what's safe and what is not.

--
Cedric

On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
> This commit fixes the deadlock introduced by commit
> 0980764dee687e8da86dc410c351759867163389. The call getgrouplist() of
> the glibc library isn't safe to be called in between fork and
> exec (see commit 75c125641ac73473ba4b0542524d67a184769c8e).
> 
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx>
> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
> ---
>  src/lxc/lxc_container.c | 12 +++++++++++-
>  src/util/vircommand.c   | 25 ++++++++++++++-----------
>  src/util/vircommand.h   |  2 +-
>  tests/commandtest.c     | 15 ++++++++++-----
>  4 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index ec6d6a86b0b6..1f220c602b0a 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2182,6 +2182,8 @@ static int lxcContainerChild(void *data)
>      virDomainFSDefPtr root;
>      virCommandPtr cmd = NULL;
>      int hasReboot;
> +    gid_t *groups = NULL;
> +    int ngroups;
>  
>      if (NULL == vmDef) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2297,6 +2299,13 @@ static int lxcContainerChild(void *data)
>          goto cleanup;
>      }
>  
> +    /* TODO is it safe to call it here or should this call be moved in
> +     * front of the clone() as otherwise there might be a risk for a
> +     * deadlock */
> +    if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd),
> +                                   &groups)) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      VIR_FREE(ttyPath);
> @@ -2307,7 +2316,7 @@ static int lxcContainerChild(void *data)
>      if (ret == 0) {
>          VIR_DEBUG("Executing init binary");
>          /* this function will only return if an error occurred */
> -        ret = virCommandExec(cmd);
> +        ret = virCommandExec(cmd, groups, ngroups);
>      }
>  
>      if (ret != 0) {
> @@ -2317,6 +2326,7 @@ static int lxcContainerChild(void *data)
>                  virGetLastErrorMessage());
>      }
>  
> +    VIR_FREE(groups);
>      virCommandFree(cmd);
>      return ret;
>  }
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index fba73ca18eac..41a61da49f82 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -465,15 +465,10 @@ virCommandHandshakeChild(virCommandPtr cmd)
>  }
>  
>  static int
> -virExecCommon(virCommandPtr cmd)
> +virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups)
>  {
> -    gid_t *groups = NULL;
> -    int ngroups;
>      int ret = -1;
>  
> -    if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
> -        goto cleanup;
> -
>      if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
>          cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
>          VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> @@ -495,7 +490,6 @@ virExecCommon(virCommandPtr cmd)
>      ret = 0;
>  
>   cleanup:
> -    VIR_FREE(groups);
>      return ret;
>  }
>  
> @@ -519,6 +513,8 @@ virExec(virCommandPtr cmd)
>      const char *binary = NULL;
>      int ret;
>      struct sigaction waxon, waxoff;
> +    gid_t *groups = NULL;
> +    int ngroups;
>  
>      if (cmd->args[0][0] != '/') {
>          if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) {
> @@ -589,6 +585,9 @@ virExec(virCommandPtr cmd)
>          childerr = null;
>      }
>  
> +    if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
> +        goto cleanup;
> +
>      pid = virFork();
>  
>      if (pid < 0)
> @@ -756,7 +755,7 @@ virExec(virCommandPtr cmd)
>      }
>  # endif
>  
> -    if (virExecCommon(cmd) < 0)
> +    if (virExecCommon(cmd, groups, ngroups) < 0)
>          goto fork_error;
>  
>      if (virCommandHandshakeChild(cmd) < 0)
> @@ -799,6 +798,7 @@ virExec(virCommandPtr cmd)
>         should never jump here on error */
>  
>      VIR_FREE(binarystr);
> +    VIR_FREE(groups);
>  
>      /* NB we don't virReportError() on any failures here
>         because the code which jumped here already raised
> @@ -2167,6 +2167,8 @@ virCommandProcessIO(virCommandPtr cmd)
>  /**
>   * virCommandExec:
>   * @cmd: command to run
> + * @groups: array of supplementary group IDs used for the command
> + * @ngroups: number of group IDs in @groups
>   *
>   * Exec the command, replacing the current process. Meant to be called
>   * in the hook after already forking / cloning, so does not attempt to
> @@ -2176,7 +2178,7 @@ virCommandProcessIO(virCommandPtr cmd)
>   * Will not return on success.
>   */
>  #ifndef WIN32
> -int virCommandExec(virCommandPtr cmd)
> +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups)
>  {
>      if (!cmd ||cmd->has_error == ENOMEM) {
>          virReportOOMError();
> @@ -2188,7 +2190,7 @@ int virCommandExec(virCommandPtr cmd)
>          return -1;
>      }
>  
> -    if (virExecCommon(cmd) < 0)
> +    if (virExecCommon(cmd, groups, ngroups) < 0)
>          return -1;
>  
>      execve(cmd->args[0], cmd->args, cmd->env);
> @@ -2199,7 +2201,8 @@ int virCommandExec(virCommandPtr cmd)
>      return -1;
>  }
>  #else
> -int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED)
> +int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED, gid_t *groups ATTRIBUTE_UNUSED,
> +                   int ngroups ATTRIBUTE_UNUSED)
>  {
>      /* Mingw execve() has a broken signature. Disable this
>       * function until gnulib fixes the signature, since we
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index b401d7b238d7..d59278cf5f6c 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -173,7 +173,7 @@ void virCommandWriteArgLog(virCommandPtr cmd,
>  
>  char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK;
>  
> -int virCommandExec(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK;
> +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) ATTRIBUTE_RETURN_CHECK;
>  
>  int virCommandRun(virCommandPtr cmd,
>                    int *exitstatus) ATTRIBUTE_RETURN_CHECK;
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index 1f6f16bcde73..7d73f638a2e2 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
> @@ -1070,6 +1070,9 @@ static int test25(const void *unused ATTRIBUTE_UNUSED)
>      int rv = 0;
>      ssize_t tries = 100;
>      pid_t pid;
> +    gid_t *groups = NULL;
> +    int ngroups;
> +    virCommandPtr cmd = virCommandNew("some/nonexistent/binary");
>  
>      if (pipe(pipeFD) < 0) {
>          fprintf(stderr, "Unable to create pipe\n");
> @@ -1081,6 +1084,10 @@ static int test25(const void *unused ATTRIBUTE_UNUSED)
>          goto cleanup;
>      }
>  
> +    if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd),
> +                                   &groups)) < 0)
> +        goto cleanup;
> +
>      /* Now, fork and try to exec a nonexistent binary. */
>      pid = virFork();
>      if (pid < 0) {
> @@ -1090,11 +1097,7 @@ static int test25(const void *unused ATTRIBUTE_UNUSED)
>  
>      if (pid == 0) {
>          /* Child */
> -        virCommandPtr cmd = virCommandNew("some/nonexistent/binary");
> -
> -        rv = virCommandExec(cmd);
> -
> -        virCommandFree(cmd);
> +        rv = virCommandExec(cmd, groups, ngroups);
>  
>          if (safewrite(pipeFD[1], &rv, sizeof(rv)) < 0)
>              fprintf(stderr, "Unable to write to pipe\n");
> @@ -1129,6 +1132,8 @@ static int test25(const void *unused ATTRIBUTE_UNUSED)
>   cleanup:
>      VIR_FORCE_CLOSE(pipeFD[0]);
>      VIR_FORCE_CLOSE(pipeFD[1]);
> +    VIR_FREE(groups);
> +    virCommandFree(cmd);
>      return ret;
>  }
>  

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux