util: Fix deadlock across fork()

This commit fixes the deadlock introduced by commit
0980764d. The call getgrouplist() of
the glibc library isn't safe to be called in between fork and
exec (see commit 75c12564).
Signed-off-by: NMarc Hartmayer <mhartmay@linux.vnet.ibm.com>
Fixes: 0980764d ("util: share code between virExec and virCommandExec")
Reviewed-by: NBjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: NBoris Fiuczynski <fiuczy@linux.vnet.ibm.com>

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;
 }

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

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;

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;
 }