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