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