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@linux.vnet.ibm.com>
Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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
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 <mhartmay@linux.vnet.ibm.com>
> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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
On Mon, Oct 09, 2017 at 09:14:56PM +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 <mhartmay@linux.vnet.ibm.com>
> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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 */
Yes, clone() is equiv to fork() so it needs to be before clone()
> + if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd),
> + &groups)) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> VIR_FREE(ttyPath);
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.