On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote: > These functions are used by an upcoming commit. > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > --- >  src/libvirt_private.syms |  2 ++ >  src/util/vircommand.c    | 14 ++++++++++++++ >  src/util/vircommand.h    |  4 ++++ >  3 files changed, 20 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 9243c5591042..26c5ddb40505 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1506,6 +1506,8 @@ virCommandDaemonize; >  virCommandDoAsyncIO; >  virCommandExec; >  virCommandFree; > +virCommandGetGID; > +virCommandGetUID; >  virCommandHandshakeNotify; >  virCommandHandshakeWait; >  virCommandNew; > diff --git a/src/util/vircommand.c b/src/util/vircommand.c > index 60c1121dafea..fba73ca18eac 100644 > --- a/src/util/vircommand.c > +++ b/src/util/vircommand.c > @@ -1073,6 +1073,20 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) >  } >   >   > +gid_t > +virCommandGetGID(virCommandPtr cmd) > +{ > +    return cmd->gid; > +} > + > + > +uid_t > +virCommandGetUID(virCommandPtr cmd) > +{ > +    return cmd->uid; > +} > + > + >  void >  virCommandSetGID(virCommandPtr cmd, gid_t gid) >  { > diff --git a/src/util/vircommand.h b/src/util/vircommand.h > index e7c2e513bae1..b401d7b238d7 100644 > --- a/src/util/vircommand.h > +++ b/src/util/vircommand.h > @@ -68,6 +68,10 @@ int virCommandPassFDGetFDIndex(virCommandPtr cmd, >  void virCommandSetPidFile(virCommandPtr cmd, >                            const char *pidfile) ATTRIBUTE_NONNULL(2); >   > +gid_t virCommandGetGID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); > + > +uid_t virCommandGetUID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); > + >  void virCommandSetGID(virCommandPtr cmd, gid_t gid); >   >  void virCommandSetUID(virCommandPtr cmd, uid_t uid); ACK.

I guess the commit using those is still to come, right?

-- Cedric

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(a)linux.vnet.ibm.com&gt; Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec") Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; ---  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;  }