[libvirt] [RFC PATCH 2/4] util: Fix deadlock across fork()
Marc Hartmayer
mhartmay at linux.vnet.ibm.com
Mon Oct 9 19:14:56 UTC 2017
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 at linux.vnet.ibm.com>
Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy at 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
More information about the libvir-list
mailing list