[PATCH 1/2] util: share code between virExec and virCommandExec

[Cédric Bosdonnat <cbosdonnat@xxxxxxxx>]

virCommand is a version of virExec that doesn't fork, however it is
just calling execve and doesn't honors setting uid/gid and pwd.

This commit moves those pieces from virExec to virCommandExec and
makes virExec use virCommandExec to avoid code duplication.
---
 src/util/vircommand.c | 92 ++++++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 49 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index e1bbc0526..aa97a5a10 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -481,21 +481,18 @@ virExec(virCommandPtr cmd)
     int childerr = -1;
     int tmpfd;
     char *binarystr = NULL;
-    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]))) {
+        if (!(binarystr = virFindFileInPath(cmd->args[0]))) {
             virReportSystemError(ENOENT,
                                  _("Cannot find '%s' in path"),
                                  cmd->args[0]);
             return -1;
         }
-    } else {
-        binary = cmd->args[0];
+        VIR_FREE(cmd->args[0]);
+        cmd->args[0] = binarystr;
     }
 
     if (childin < 0) {
@@ -556,9 +553,6 @@ virExec(virCommandPtr cmd)
         childerr = null;
     }
 
-    if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
-        goto cleanup;
-
     pid = virFork();
 
     if (pid < 0)
@@ -577,9 +571,6 @@ virExec(virCommandPtr cmd)
 
         cmd->pid = pid;
 
-        VIR_FREE(binarystr);
-        VIR_FREE(groups);
-
         return 0;
     }
 
@@ -727,29 +718,6 @@ virExec(virCommandPtr cmd)
     }
 # endif
 
-    /* The steps above may need to do something privileged, so we delay
-     * setuid and clearing capabilities until the last minute.
-     */
-    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",
-                  (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
-        if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
-                                 cmd->capabilities,
-                                 !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
-            goto fork_error;
-        }
-    }
-
-    if (cmd->pwd) {
-        VIR_DEBUG("Running child in %s", cmd->pwd);
-        if (chdir(cmd->pwd) < 0) {
-            virReportSystemError(errno,
-                                 _("Unable to change to %s"), cmd->pwd);
-            goto fork_error;
-        }
-    }
-
     if (virCommandHandshakeChild(cmd) < 0)
        goto fork_error;
 
@@ -771,15 +739,10 @@ virExec(virCommandPtr cmd)
     /* Close logging again to ensure no FDs leak to child */
     virLogReset();
 
-    if (cmd->env)
-        execve(binary, cmd->args, cmd->env);
-    else
-        execv(binary, cmd->args);
+    if (virCommandExec(cmd) == -2)
+        goto fork_error;
 
     ret = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
-    virReportSystemError(errno,
-                         _("cannot execute binary %s"),
-                         cmd->args[0]);
 
  fork_error:
     virDispatchError(NULL);
@@ -789,9 +752,6 @@ virExec(virCommandPtr cmd)
     /* This is cleanup of parent process only - child
        should never jump here on error */
 
-    VIR_FREE(groups);
-    VIR_FREE(binarystr);
-
     /* NB we don't virReportError() on any failures here
        because the code which jumped here already raised
        an error condition which we must not overwrite */
@@ -2150,23 +2110,57 @@ virCommandProcessIO(virCommandPtr cmd)
  * in the hook after already forking / cloning, so does not attempt to
  * daemonize or preserve any FDs.
  *
- * Returns -1 on any error executing the command.
+ * Returns -1 on any error executing the command, -2 if the error happen
+ * before running the command.
+ *
  * Will not return on success.
  */
 #ifndef WIN32
 int virCommandExec(virCommandPtr cmd)
 {
+    gid_t *groups = NULL;
+    int ngroups;
+
     if (!cmd ||cmd->has_error == ENOMEM) {
         virReportOOMError();
-        return -1;
+        return -2;
     }
     if (cmd->has_error) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("invalid use of command API"));
-        return -1;
+        return -2;
     }
 
-    execve(cmd->args[0], cmd->args, cmd->env);
+    if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) {
+        VIR_FREE(groups);
+        return -2;
+    }
+
+    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",
+                  (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
+        if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
+                                 cmd->capabilities,
+                                 !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
+            return -2;
+        }
+    }
+    VIR_FREE(groups);
+
+    if (cmd->pwd) {
+        VIR_DEBUG("Running child in %s", cmd->pwd);
+        if (chdir(cmd->pwd) < 0) {
+            virReportSystemError(errno,
+                                 _("Unable to change to %s"), cmd->pwd);
+            return -2;
+        }
+    }
+
+    if (cmd->env)
+        execve(cmd->args[0], cmd->args, cmd->env);
+    else
+        execv(cmd->args[0], cmd->args);
 
     virReportSystemError(errno,
                          _("cannot execute binary %s"),
-- 
2.12.2

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list