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; } -- 2.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list