Fix SIGUSR1 after stdin_fd closed

Just shut it down for writing, and save it so that it can later be used
for reading.

This also ensures deterministic behavior when --use-stdin-socket is
used.  Previously, the socket would be shut down for reading or writing
but not both, depending on whether reading or writing stopped first.
This could be quite confusing for callers.  Instead, always shut down
the socket for both reading and writing, guaranteeing deterministic
behavior.

This partially reverts a3bbcb5 ("Prefer close() to shutdown()"):
the change to use close() is reverted, but the substantial
simplification of the code is not.

Fixes: a3bbcb5 ("Prefer close() to shutdown()")
Fixes: QubesOS/qubes-issues#9183

Author: DemiMarie

libqrexec/process_io.c

@@ -42,24 +42,31 @@ static _Noreturn void handle_vchan_error(const char *op)
 /*
  * Closing the file descriptors:
  *
- * - If this is a single socket used for both stdin and stdout, call shutdown()
- *   to indicate that no more data will be sent or received.
- * - Otherwise, restore the blocking status of the FD.
+ * - If this file descriptor was inherited _and_ is not used for both stdin
+ *   and stdout, restore the blocking status of the FD and then close it.
+ * - Otherwise, call shutdown() to indicate that no more data will be sent or
+ *   received in the given direction.
  */
-static void close_stdio(int fd, int other_fd, int direction) {
+static bool close_stdio(int fd, int direction, bool single_socket) {
     if (fd == -1)
-        return;
-    if (fd == other_fd) {
-        /* Do not close the FD, as it is still in use. */
-        /* ENOTCONN can happen with TCP and is harmless */
-        if (shutdown(fd, direction) == -1 && errno != ENOTSOCK && errno != ENOTCONN)
-            PERROR("shutdown");
-        return;
-    }
-
-    if (fd < 3)
+        return false;
+    if (single_socket || fd > 2) {
+        if (shutdown(fd, direction) == -1) {
+            if (errno == ENOTSOCK && !single_socket) {
+                close(fd);
+                return true;
+            } else if (errno != ENOTCONN) {
+                // ENOTCONN can happen with TCP and is harmless
+                PERROR("shutdown");
+            }
+        }
+        return false;
+    } else {
+        /* Close the file descriptor */
         set_block(fd);
-    close(fd);
+        close(fd);
+        return true;
+    }
 }
 
 static void close_stderr(int fd) {
@@ -96,6 +103,10 @@ int process_io(const struct process_io_request *req) {
 
     pid_t local_status = -1;
     pid_t remote_status = -1;
+    /* Saved version of stdin_fd.  If we get SIGHUP,
+     * this replaces stdout_fd.  Set to -1 only if stdin_fd
+     * is closed, not merely shut down. */
+    int saved_stdin_fd = stdin_fd;
     int stdout_msg_type = is_service ? MSG_DATA_STDOUT : MSG_DATA_STDIN;
     int ret;
     struct pollfd fds[FD_NUM];
@@ -116,22 +127,34 @@ int process_io(const struct process_io_request *req) {
     sigprocmask(SIG_BLOCK, &pollmask, NULL);
     sigemptyset(&pollmask);
 
+    /* Invariants:
+     *
+     * - if stdin_fd == stdout_fd, then use_stdio_socket is true.
+     * - if use_stdio_socket is true, then either:
+     *   - stdin_fd == stdout_fd right now.
+     *   - stdin_fd == stdout_fd at some point in the past,
+     *     and one of them is currently -1.
+     */
+    bool use_stdio_socket = stdin_fd == stdout_fd;
     set_nonblock(stdin_fd);
-    if (stdout_fd != stdin_fd)
+    if (!use_stdio_socket)
         set_nonblock(stdout_fd);
     if (stderr_fd >= 0) {
         assert(is_service); // if this is a client, stderr_fd is *always* -1
         set_nonblock(stderr_fd);
     }
 
     /* Convenience macros that eliminate a ton of error-prone boilerplate */
-#define close_stdin() do {                      \
-    close_stdio(stdin_fd, stdout_fd, SHUT_WR);  \
-    stdin_fd = -1;                              \
+#define close_stdin() do {                                  \
+    /* if stdin_fd was actually closed, set saved_stdin_fd  \
+     * to -1 to avoid use-after-close */                    \
+    if (close_stdio(stdin_fd, SHUT_WR, use_stdio_socket))   \
+        saved_stdin_fd = -1;                                \
+    stdin_fd = -1;                                          \
 } while (0)
-#define close_stdout() do {                     \
-    close_stdio(stdout_fd, stdin_fd, SHUT_RD);  \
-    stdout_fd = -1;                             \
+#define close_stdout() do {                             \
+    close_stdio(stdout_fd, SHUT_RD, use_stdio_socket);  \
+    stdout_fd = -1;                                     \
 } while (0)
 #pragma GCC poison close_stdio
 
@@ -187,9 +210,10 @@ int process_io(const struct process_io_request *req) {
         }
 
         /* child signaled desire to use single socket for both stdin and stdout */
-        if (sigusr1 && *sigusr1 && stdin_fd != stdout_fd) {
+        if (sigusr1 && *sigusr1 && !use_stdio_socket) {
             close_stdout();
-            stdout_fd = stdin_fd;
+            stdout_fd = saved_stdin_fd;
+            use_stdio_socket = true;
             *sigusr1 = 0;
         }
 

qrexec/tests/socket/agent.py

@@ -1341,6 +1341,36 @@ def test_run_client_eof(self):
         self.client.wait()
         self.assertEqual(self.client.returncode, 42)
 
+    def test_run_client_bidirectional_shutdown(self):
+        try:
+            remote, local = socket.socketpair()
+            target_client = self.run_service(stdio=remote)
+            initial_data = b"stdout data\n"
+            target_client.send_message(qrexec.MSG_DATA_STDOUT, initial_data)
+            # FIXME: data can be received in multiple messages
+            self.assertEqual(local.recv(len(initial_data)), initial_data)
+            initial_data = b"stdin data\n"
+            local.sendall(initial_data)
+            local.shutdown(socket.SHUT_WR)
+            self.assertStdoutMessages(target_client, initial_data, qrexec.MSG_DATA_STDIN)
+            # Check that EOF received
+            self.assertEqual(target_client.recv_message(), (qrexec.MSG_DATA_STDIN, b""))
+            target_client.send_message(qrexec.MSG_DATA_STDOUT, b"")
+            # Check that EOF got propagated on this side too, even though
+            # we still have a reference to the socket.  This indicates that
+            # qrexec-client-vm shut down the socket for writing.
+            self.assertEqual(local.recv(1), b"")
+            with self.assertRaises(BrokenPipeError):
+                remote.send(b"a")
+            target_client.send_message(
+                qrexec.MSG_DATA_EXIT_CODE, struct.pack("<L", 42)
+            )
+            self.client.wait()
+            self.assertEqual(self.client.returncode, 42)
+        finally:
+            remote.close()
+            local.close()
+
     def test_run_client_replace_chars(self):
         target_client = self.run_service(options=["-t"])
         target_client.send_message(
@@ -1424,6 +1454,41 @@ def test_stdio_socket(self):
         self.client.wait()
         self.assertEqual(self.client.returncode, 42)
 
+    def test_stdio_socket_closed_stdin(self):
+        fifo = os.path.join(self.tempdir, "new_file")
+        os.mkfifo(fifo, mode=0o600)
+        target = self.run_service(
+            local_program=[
+                "/bin/sh",
+                "-euc",
+                """\
+echo hello world
+# Wait for stdin to be closed for writing
+read -r x
+cat
+# Request that stdin be used to send stdout data
+kill -USR1 "$QREXEC_AGENT_PID"
+echo "received: $x" >&0
+# Avoid a race condition by ensuring the program does not exit too soon
+read < "$1"
+""",
+                "sh",
+                fifo,
+            ]
+        )
+        self.assertStdoutMessages(target, b"hello world\n", qrexec.MSG_DATA_STDIN)
+
+        target.send_message(qrexec.MSG_DATA_STDOUT, b"stdin\n")
+        target.send_message(qrexec.MSG_DATA_STDOUT, b"")
+
+        self.assertStdoutMessages(target, b"received: stdin\n", qrexec.MSG_DATA_STDIN)
+        with open(fifo, "wb"): pass
+        self.assertEqual(target.recv_message(), (qrexec.MSG_DATA_STDIN, b""))
+
+        target.send_message(qrexec.MSG_DATA_EXIT_CODE, struct.pack("<L", 42))
+        self.client.wait()
+        self.assertEqual(self.client.returncode, 42)
+
     def test_run_client_with_local_proc_service_failed(self):
         target_client = self.run_service(local_program=["/bin/cat"])
         target_client.send_message(qrexec.MSG_DATA_STDOUT, b"")