Merge remote-tracking branch 'origin/pr/151'

* origin/pr/151:
  Ensure that EOF is propagated to stdout
  Document the file descriptrs for struct process_io_request
  Prefer close() to shutdown()
  do_fork_exec(): Drop status pipe
  Use close_range() instead of close loop
  fix_fds(): check that input FDs are okay

Author: marmarek

agent/qrexec-client-vm.c

@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  *
  */
+#include <assert.h>
 #include <sys/socket.h>
 #include <sys/wait.h>
 #include <sys/un.h>
@@ -94,6 +95,7 @@ static void convert_target_name_keyword(char *target)
 enum {
     opt_no_filter_stdout = 't'+128,
     opt_no_filter_stderr = 'T'+128,
+    opt_use_stdin_socket = 'u'+128,
 };
 
 static struct option longopts[] = {
@@ -104,6 +106,7 @@ static struct option longopts[] = {
     { "no-filter-escape-chars-stderr", no_argument, 0, opt_no_filter_stderr},
     { "agent-socket", required_argument, 0, 'a'},
     { "prefix-data", required_argument, 0, 'p' },
+    { "use-stdin-socket", no_argument, 0, opt_use_stdin_socket },
     { "help", no_argument, 0, 'h' },
     { NULL, 0, 0, 0},
 };
@@ -141,6 +144,7 @@ int main(int argc, char **argv)
     int inpipe[2], outpipe[2];
     int buffer_size = 0;
     int opt;
+    int stdout_fd = 1;
     const char *agent_trigger_path = QREXEC_AGENT_TRIGGER_PATH, *prefix_data = NULL;
 
     setup_logging("qrexec-client-vm");
@@ -198,6 +202,23 @@ int main(int argc, char **argv)
                     exit(1);
                 }
                 break;
+            case opt_use_stdin_socket:
+                {
+                    int type;
+                    if (stdout_fd == 0)
+                        errx(2, "--use-stdin-socket passed twice");
+                    socklen_t len = sizeof(type);
+                    if (getsockopt(0, SOL_SOCKET, SO_TYPE, &type, &len)) {
+                        if (errno == ENOTSOCK)
+                            errx(2, "--use-stdin-socket passed, but stdin not a socket");
+                        err(2, "getsockopt(0, SOL_SOCKET, SO_TYPE)");
+                    }
+                    assert(len == sizeof(type));
+                    if (type != SOCK_STREAM)
+                        errx(2, "stdin was a socket of type %d, not SOCK_STREAM (%d)", type, SOCK_STREAM);
+                    stdout_fd = 0;
+                }
+                break;
             case '?':
                 usage(argv[0], 2);
         }
@@ -209,6 +230,10 @@ int main(int argc, char **argv)
     if (argc - optind > 2) {
         start_local_process = 1;
     }
+    if (start_local_process && stdout_fd != 1) {
+        fprintf(stderr, "cannot spawn a local process with --use-stdin-socket\n");
+        usage(argv[0], 2);
+    }
 
     if (!start_local_process) {
         if (replace_chars_stdout == -1 && isatty(1))
@@ -303,7 +328,7 @@ int main(int argc, char **argv)
     } else {
         ret = handle_data_client(MSG_SERVICE_CONNECT,
                 exec_params.connect_domain, exec_params.connect_port,
-                1, 0, -1, buffer_size, 0, prefix_data);
+                stdout_fd, 0, -1, buffer_size, 0, prefix_data);
     }
 
     close(trigger_fd);

daemon/qrexec-client.c

@@ -72,11 +72,16 @@ static _Noreturn void do_exec(const char *prog, const char *username __attribute
     PERROR("exec bash");
     exit(1);
 }
+enum {
+    opt_socket_dir = 'd'+128,
+    opt_use_stdin_socket = 'u'+128,
+};
 
 static struct option longopts[] = {
     { "help", no_argument, 0, 'h' },
-    { "socket-dir", required_argument, 0, 'd'+128 },
+    { "socket-dir", required_argument, 0, opt_socket_dir },
     { "no-exit-code", no_argument, 0, 'E' },
+    { "use-stdin-socket", no_argument, 0, opt_use_stdin_socket },
     { NULL, 0, 0, 0 },
 };
 
@@ -96,7 +101,8 @@ _Noreturn static void usage(const char *const name)
             "  -c - connect to existing process (response to trigger service call)\n"
             "  -w timeout - override default connection timeout of 5s (set 0 for no timeout)\n"
             "  -k - kill the domain right before exiting\n"
-            "  --socket-dir=PATH -  directory for qrexec socket, default: %s\n",
+            "  --socket-dir=PATH -  directory for qrexec socket, default: %s\n"
+            "  --use-stdin-socket - use fd 0 (which must be socket) for both stdin and stdout\n",
             name ? name : "qrexec-client", QREXEC_DAEMON_SOCKET_DIR);
     exit(1);
 }
@@ -207,9 +213,26 @@ int main(int argc, char **argv)
             case 'W':
                 wait_connection_end = true;
                 break;
-            case 'd' + 128:
+            case opt_socket_dir:
                 socket_dir = strdup(optarg);
                 break;
+            case opt_use_stdin_socket:
+                {
+                    int type;
+                    if (local_stdin_fd != 1)
+                        errx(2, "--use-stdin-socket passed twice");
+                    socklen_t len = sizeof(type);
+                    if (getsockopt(0, SOL_SOCKET, SO_TYPE, &type, &len)) {
+                        if (errno == ENOTSOCK)
+                            errx(2, "--use-stdin-socket passed, but stdin not a socket");
+                        err(2, "getsockopt(0, SOL_SOCKET, SO_TYPE)");
+                    }
+                    assert(len == sizeof(type) && "wrong socket option length?");
+                    if (type != SOCK_STREAM)
+                        errx(2, "stdin was a socket of type %d, not SOCK_STREAM (%d)", type, SOCK_STREAM);
+                    local_stdin_fd = 0;
+                }
+                break;
             case 'k':
                 kill = true;
                 break;
@@ -231,6 +254,11 @@ int main(int argc, char **argv)
         usage(argv[0]);
     }
 
+    if ((local_cmdline != NULL) && (local_stdin_fd != 1)) {
+        fprintf(stderr, "ERROR: at most one of -l and --use-stdin-socket can be specified\n");
+        usage(argv[0]);
+    }
+
     if (strcmp(domname, "dom0") == 0 || strcmp(domname, "@adminvm") == 0) {
         if (request_id == NULL) {
             fprintf(stderr, "ERROR: when target domain is 'dom0', -c must be specified\n");

daemon/qrexec-daemon-common.c

@@ -255,7 +255,8 @@ bool send_service_connect(int s, const char *conn_ident,
 
 #define QREXEC_DATA_MIN_VERSION QREXEC_PROTOCOL_V2
 
-static int local_stdin_fd = 1, local_stdout_fd = 0;
+static int local_stdout_fd = 0;
+int local_stdin_fd = 1;
 static pid_t local_pid = 0;
 
 static volatile sig_atomic_t sigchld = 0;

daemon/qrexec-daemon-common.h

@@ -42,3 +42,5 @@ __attribute__((warn_unused_result))
 bool qrexec_execute_vm(const char *target, bool autostart, int remote_domain_id,
                        const char *cmd, size_t service_length, const char *request_id,
                        bool just_exec, bool wait_connection_end);
+/* FD for stdout of remote process */
+extern int local_stdin_fd;

libqrexec/exec.c

@@ -30,6 +30,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
+#include <sys/syscall.h>
 #include <sys/un.h>
 #include <sys/wait.h>
 #include <netdb.h>
@@ -88,20 +89,27 @@ void exec_qubes_rpc_if_requested(const char *prog, char *const envp[]) {
 
 void fix_fds(int fdin, int fdout, int fderr)
 {
-    int i;
-    for (i = 3; i < 256; i++)
-        if (i != fdin && i != fdout && i != fderr)
-            close(i);
+    if (fdin < 0 || fdout < 1 || fderr < 2) {
+        LOG(ERROR, "fix_fds: bad FD numbers: fdin %d, fdout %d, fderr %d",
+            fdin, fdout, fderr);
+        _exit(125);
+    }
     if (dup2(fdin, 0) < 0 || dup2(fdout, 1) < 0 || dup2(fderr, 2) < 0) {
         PERROR("dup2");
         abort();
     }
-
-    if (close(fdin) || (fdin != fdout && close(fdout)) ||
-        (fdin != fderr && fdout != fderr && fderr != 2 && close(fderr))) {
-        PERROR("close");
+#ifdef SYS_close_range
+    int close_range_res = syscall(SYS_close_range, 3, ~0U, 0);
+    if (close_range_res == 0)
+        return;
+    assert(close_range_res == -1);
+    if (errno != ENOSYS) {
+        PERROR("close_range");
         abort();
     }
+#endif
+    for (int i = 3; i < 256; ++i)
+        close(i);
 }
 
 static int do_fork_exec(const char *user,
@@ -111,14 +119,13 @@ static int do_fork_exec(const char *user,
         int *stdout_fd,
         int *stderr_fd)
 {
-    int inpipe[2], outpipe[2], errpipe[2], statuspipe[2], retval;
+    int inpipe[2], outpipe[2], errpipe[2], retval;
 #ifndef SOCK_CLOEXEC
 #define SOCK_CLOEXEC 0
 #endif
-    if (socketpair(AF_UNIX, SOCK_STREAM, 0, inpipe) ||
-            socketpair(AF_UNIX, SOCK_STREAM, 0, outpipe) ||
-            (stderr_fd && socketpair(AF_UNIX, SOCK_STREAM, 0, errpipe)) ||
-            socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, statuspipe)) {
+    if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, inpipe) ||
+            socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, outpipe) ||
+            (stderr_fd && socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, errpipe))) {
         PERROR("socketpair");
         /* FD leaks do not matter, we exit soon anyway */
         return -2;
@@ -128,38 +135,19 @@ static int do_fork_exec(const char *user,
             PERROR("fork");
             /* ditto */
             return -2;
-        case 0: {
-            int status;
+        case 0:
             if (signal(SIGPIPE, SIG_DFL) == SIG_ERR)
                 abort();
             if (stderr_fd) {
                 fix_fds(inpipe[0], outpipe[1], errpipe[1]);
             } else
                 fix_fds(inpipe[0], outpipe[1], 2);
 
-            close(statuspipe[0]);
-            if (SOCK_CLOEXEC == (0)) {
-                status = fcntl(statuspipe[1], F_GETFD);
-                fcntl(statuspipe[1], F_SETFD, status | FD_CLOEXEC);
-            }
             if (exec_func != NULL)
                 exec_func(cmdline, user);
-            else
-                abort();
-            status = -1;
-            while (write(statuspipe[1], &status, sizeof status) <= 0) {}
-            _exit(-1);
-        }
-        default: {
-            close(statuspipe[1]);
-            if (read(statuspipe[0], &retval, sizeof retval) == sizeof retval) {
-                siginfo_t siginfo;
-                memset(&siginfo, 0, sizeof siginfo);
-                waitid(P_PID, *pid, &siginfo, WEXITED); // discard result
-            } else {
-                retval = 0;
-            }
-        }
+            abort();
+        default:
+            retval = 0;
     }
     close(inpipe[0]);
     close(outpipe[1]);

libqrexec/libqrexec-utils.h

@@ -267,6 +267,9 @@ struct process_io_request {
     libvchan_t *vchan;
     struct buffer *stdin_buf;
 
+    /* Note that stdin_fd, stdout_fd, and stderr_fd are named assuming a
+     * *local* process.  For a *remote* process, stdin_fd is the standard
+     * *output*, stdout_fd is the standard *input*, and stderr_fd must be -1. */
     // stderr_fd can be -1
     int stdin_fd, stdout_fd, stderr_fd;
     // 0 if no child process
@@ -275,12 +278,14 @@ struct process_io_request {
     /*
       is_service true (this is a service):
         - send local data as MSG_DATA_STDOUT
+        - send local stderr as MSG_DATA_STDERR, unless in dom0
         - send exit code
         - wait just for local end
         - return local exit code
 
       is_service false (this is a client):
         - send local data as MSG_DATA_STDIN
+        - stderr_fd is always -1
         - don't send exit code
         - wait for local and remote end
         - return remote exit code