varlinkctl: reopen tty stdin/stdout non-blocking for call --upgrade

When using `varlinkctl call --upgrade` interactively from a terminal
the connection hangs as soon as the first line is typed. The socket
forwarder splice()s with SPLICE_F_NONBLOCK but that only covers the
pipe side, so with a blocking tty as stdin the second splice() blocks
and the single event loop is stuck. This went unnoticed so far because
the existing users (like `sftp -D`) pass pipes.

So this commit reopens stdin/stdout with O_NONBLOCK when they are a
tty, the same way ptyfwd does it. Reopening gives us our own open file
description so the terminal of the invoking shell is not switched to
non-blocking. Everything else keeps the plain dup as pipes are already
covered by SPLICE_F_NONBLOCK and regular files never block. This also
documents the requirement in socket-forward.h.

src/shared/socket-forward.h

@@ -9,6 +9,10 @@
  * avoiding userspace copies. Internally uses two independent half-duplex forwarders,
  * one per direction. All four fds must be distinct - use dup()/fcntl(fd, F_DUPFD_CLOEXEC, 3) for bidirectional sockets.
  *
+ * SPLICE_F_NONBLOCK only applies to the internal pipe, not to the fds being spliced from/to. Since all
+ * forwarding runs in one event loop, sockets and ttys must be O_NONBLOCK or a single blocking splice()
+ * stalls every direction. Pipes and regular files do not need it.
+ *
  * When forwarding completes (both directions reach EOF or error), the completion callback is invoked.
  *
  * The SocketForward takes ownership of all fds - they are closed when the SocketForward is freed

src/varlinkctl/varlinkctl.c

@@ -642,6 +642,36 @@ static int upgrade_forward_done(SocketForward *sf, int error, void *userdata) {
         return sd_event_exit(event, error < 0 ? error : 0);
 }
 
+static int dup_stdio_fd(int fd, int flags, int *ret_nonblock_reset) {
+        int r;
+
+        assert(fd >= 0);
+        assert(ret_nonblock_reset);
+
+        /* SPLICE_F_NONBLOCK only covers the forwarder's internal pipe, hence stdin/stdout must be
+         * O_NONBLOCK themselves. Reopen (as ptyfwd does) so that the invoking shell's open file description
+         * is not switched to non-blocking too. Sockets cannot be reopened, hence the fallback. */
+        r = fd_reopen_propagate_append_and_position(fd, flags|O_CLOEXEC|O_NOCTTY|O_NONBLOCK);
+        if (r >= 0) {
+                *ret_nonblock_reset = -EBADF;
+                return r;
+        }
+
+        log_debug_errno(r, "Failed to reopen fd %i, using a dup instead: %m", fd);
+
+        _cleanup_close_ int dup_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+        if (dup_fd < 0)
+                return -errno;
+
+        /* The dup shares the open file description, whoever uses it after us expects it blocking again. */
+        r = fd_nonblock(dup_fd, true);
+        if (r < 0)
+                return r;
+        *ret_nonblock_reset = r > 0 ? fd : -EBADF;
+
+        return TAKE_FD(dup_fd);
+}
+
 /* This will only return if something goes wrong, otherwise the exec_cmdline
  * is run and replaces our code. */
 static int exec_with_listen_fds(char **exec_cmdline, int *fds, size_t n_fds) {
@@ -835,18 +865,29 @@ static int varlink_call_and_upgrade(const char *url, const char *method, sd_json
 
         /* No --exec: bidirectional proxy between stdin/stdout and the upgraded socket */
 
+        /* splice() into a socket honours the socket's O_NONBLOCK, not SPLICE_F_NONBLOCK. */
+        r = fd_nonblock(input_fd, true);
+        if (r < 0)
+                return log_error_errno(r, "Failed to set upgraded input fd to non-blocking mode: %m");
+
+        r = fd_nonblock(output_fd, true);
+        if (r < 0)
+                return log_error_errno(r, "Failed to set upgraded output fd to non-blocking mode: %m");
+
         /* Use a clean event loop just for the forwarding to avoid any interference. */
         r = sd_event_new(&event);
         if (r < 0)
                 return log_error_errno(r, "Failed to allocate event loop: %m");
 
-        _cleanup_close_ int stdin_fd = fcntl(STDIN_FILENO, F_DUPFD_CLOEXEC, 3);
-        if (stdin_fd < 0)
-                return log_error_errno(errno, "Failed to dup stdin: %m");
+        _cleanup_(nonblock_resetp) int stdin_nonblock_reset = -EBADF, stdout_nonblock_reset = -EBADF;
 
-        _cleanup_close_ int stdout_fd = fcntl(STDOUT_FILENO, F_DUPFD_CLOEXEC, 3);
+        _cleanup_close_ int stdin_fd = dup_stdio_fd(STDIN_FILENO, O_RDONLY, &stdin_nonblock_reset);
+        if (stdin_fd < 0)
+                return log_error_errno(stdin_fd, "Failed to dup stdin: %m");
+
+        _cleanup_close_ int stdout_fd = dup_stdio_fd(STDOUT_FILENO, O_WRONLY, &stdout_nonblock_reset);
         if (stdout_fd < 0)
-                return log_error_errno(errno, "Failed to dup stdout: %m");
+                return log_error_errno(stdout_fd, "Failed to dup stdout: %m");
 
         r = socket_forward_new_pair(
                         event,

test/units/TEST-74-AUX-UTILS.varlinkctl.sh

@@ -379,6 +379,28 @@ result="$(varlinkctl call --upgrade "unix:$SERVE_SOCKET" io.systemd.Compress.Dec
 echo "$result" | grep "untrusted data decompressed safely" >/dev/null
 kill "$SERVE_PID" 2>/dev/null || true
 wait "$SERVE_PID" 2>/dev/null || true
+rm -f "$SERVE_SOCKET"
+
+# Test 3: interactive use with a (blocking) tty as stdin. The reply has to arrive while stdin is still
+# open, i.e. forwarding must not get stuck reading from the tty after the first line. Only then is stdin
+# closed, which ends the session.
+SERVE_PID=$(systemd-notify --fork -- \
+                           systemd-socket-activate -l "$SERVE_SOCKET" -- \
+                                   varlinkctl serve io.systemd.test.Reverse -- sh -c 'head -n1 | rev')
+TTY_INPUT="$(dirname "$SERVE_SOCKET")/tty-input"
+TTY_OUTPUT="$(dirname "$SERVE_SOCKET")/tty-output"
+mkfifo "$TTY_INPUT"
+timeout 30 script -qec "varlinkctl call --upgrade unix:$SERVE_SOCKET io.systemd.test.Reverse {}" /dev/null \
+        <"$TTY_INPUT" >"$TTY_OUTPUT" &
+CALL_PID=$!
+exec 3>"$TTY_INPUT"
+echo "via tty" >&3
+timeout 30 bash -c "until grep 'ytt aiv' '$TTY_OUTPUT' >/dev/null; do sleep .1; done"
+exec 3>&-
+wait "$CALL_PID"
+kill "$SERVE_PID" 2>/dev/null || true
+wait "$SERVE_PID" 2>/dev/null || true
+rm -f "$SERVE_SOCKET" "$TTY_INPUT" "$TTY_OUTPUT"
 
 # Test 3: --polkit-action=: root passes without a polkit round trip, an unprivileged user is denied
 # because no policy grants the (nonexistent) action.