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 <mhart...@linux.vnet.ibm.com> > Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec") > Reviewed-by: Bjoern Walk <bw...@linux.vnet.ibm.com> > Reviewed-by: Boris Fiuczynski <fiu...@linux.vnet.ibm.com> > --- > 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list