util: Fix deadlock across fork()

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>

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index ec6d6a86b0b6b124736e97957c3f357d9d0ddd35..1f220c602b0ab4276d274e8a2393a03332f54082 100644 (file)
--- 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 fba73ca18eac2b6da41069d492d64fc54ca636de..41a61da49f82247c5c37782f837ea20b31681dce 100644 (file)
--- 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 b401d7b238d777e525fe1ea78a780a8542c1a818..d59278cf5f6cd7b829edc1841dc6d976a5146c67 100644 (file)
--- 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 1f6f16bcde73936ede89bcbc42ad7e3d0971d0b2..7d73f638a2e2ae3d4c3aa2d032c7b7b72a956001 100644 (file)
--- 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;
 }