From 042f633b12c72e20f532647655bbe4afcafc2aa0 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 24 Mar 2026 09:48:39 +0100 Subject: [PATCH] varlinkctl: add support for `--exec` with `--upgrade` Having support for `--exec` when using `--upgrade` is nice so this commit adds it. It does it by extracting a shared helper called `exec_with_listen_fds()` and then use that in the `verb_call()` and `varlink_call_and_upgrade()` calls. --- man/varlinkctl.xml | 17 +- src/varlinkctl/varlinkctl.c | 171 +++++++++++++-------- test/units/TEST-74-AUX-UTILS.varlinkctl.sh | 24 ++- 3 files changed, 139 insertions(+), 73 deletions(-) diff --git a/man/varlinkctl.xml b/man/varlinkctl.xml index 2a771c1049..6aff5a05e1 100644 --- a/man/varlinkctl.xml +++ b/man/varlinkctl.xml @@ -275,13 +275,20 @@ When used with call: request a protocol upgrade. The method call is sent with the upgrade flag set. The service is expected to send a single reply confirming the upgrade. After the reply, the Varlink protocol is no longer in effect on - the connection. Instead, varlinkctl acts as a bidirectional proxy: data read - from standard input is forwarded to the upgraded connection, and data received from the connection - is written to standard output. + the connection. + + If is not specified, varlinkctl acts as a + bidirectional proxy: data read from standard input is forwarded to the upgraded connection, and data + received from the connection is written to standard output. + + If is specified, the upgraded connection socket is placed on both + standard input and standard output of the invoked process. This is similar to the regular + behavior (without ), which places the method call + reply on standard input. The invoked process can thus simply read from and write to + stdin/stdout to communicate over the upgraded protocol. This option may not be combined with , , - , , , or - . + , , or . diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index e73c72ab97..979797f8ec 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -650,7 +650,72 @@ static int upgrade_forward_done(SocketForward *sf, int error, void *userdata) { return sd_event_exit(event, error < 0 ? error : 0); } -static int varlink_call_and_upgrade(const char *url, const char *method, sd_json_variant *parameters) { +_noreturn_ static void exec_with_listen_fds(char **exec_cmdline, int *fds, size_t n_fds) { + int r; + + _cleanup_free_ char *j = strv_join(exec_cmdline, " "); + if (!j) { + log_oom(); + _exit(EXIT_FAILURE); + } + + log_close(); + log_set_open_when_needed(true); + + r = close_all_fds(fds, n_fds); + if (r < 0) { + log_error_errno(r, "Failed to close all remaining file descriptors: %m"); + _exit(EXIT_FAILURE); + } + + r = pack_fds(fds, n_fds); + if (r < 0) { + log_error_errno(r, "Failed to rearrange file descriptors: %m"); + _exit(EXIT_FAILURE); + } + + r = fd_cloexec_many(fds, n_fds, false); + if (r < 0) { + log_error_errno(r, "Failed to disable O_CLOEXEC for file descriptors: %m"); + _exit(EXIT_FAILURE); + } + + if (n_fds > 0) { + r = setenvf("LISTEN_FDS", /* overwrite= */ true, "%zu", n_fds); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_FDS environment variable: %m"); + _exit(EXIT_FAILURE); + } + + r = setenvf("LISTEN_PID", /* overwrite= */ true, PID_FMT, getpid_cached()); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_PID environment variable: %m"); + _exit(EXIT_FAILURE); + } + + uint64_t pidfdid; + if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { + r = setenvf("LISTEN_PIDFDID", /* overwrite= */ true, "%" PRIu64, pidfdid); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_PIDFDID environment variable: %m"); + _exit(EXIT_FAILURE); + } + } + } else { + (void) unsetenv("LISTEN_FDS"); + (void) unsetenv("LISTEN_PID"); + (void) unsetenv("LISTEN_PIDFDID"); + } + (void) unsetenv("LISTEN_FDNAMES"); + + log_debug("Executing: %s", j); + + execvp(exec_cmdline[0], exec_cmdline); + log_error_errno(errno, "Failed to execute '%s': %m", j); + _exit(EXIT_FAILURE); +} + +static int varlink_call_and_upgrade(const char *url, const char *method, sd_json_variant *parameters, char **exec_cmdline) { _cleanup_(sd_varlink_unrefp) sd_varlink *vl = NULL; _cleanup_(sd_event_unrefp) sd_event *event = NULL; _cleanup_(socket_forward_freep) SocketForward *sf = NULL; @@ -684,6 +749,40 @@ static int varlink_call_and_upgrade(const char *url, const char *method, sd_json return log_error_errno(errno, "Failed to dup upgraded connection fd: %m"); } + if (!strv_isempty(exec_cmdline)) { + /* --exec mode: place the upgraded connection on stdin/stdout so that the child + * process can just read/write naturally. */ + (void) sd_notify(/* unset_environment= */ false, "READY=1"); + + log_close(); + log_set_open_when_needed(true); + + int keep_fds[] = { TAKE_FD(input_fd), TAKE_FD(output_fd) }; + + r = close_all_fds(keep_fds, 2); + if (r < 0) { + log_error_errno(r, "Failed to close remaining file descriptors: %m"); + _exit(EXIT_FAILURE); + } + + r = move_fd(keep_fds[0], STDIN_FILENO, /* cloexec= */ false); + if (r < 0) { + log_error_errno(r, "Failed to move upgraded connection to stdin: %m"); + _exit(EXIT_FAILURE); + } + + r = move_fd(keep_fds[1], STDOUT_FILENO, /* cloexec= */ false); + if (r < 0) { + log_error_errno(r, "Failed to move upgraded connection to stdout: %m"); + _exit(EXIT_FAILURE); + } + + /* We exec and never return here */ + exec_with_listen_fds(exec_cmdline, /* fds= */ NULL, /* n_fds= */ 0); + } + + /* No --exec: bidirectional proxy between stdin/stdout and the upgraded socket */ + /* Use a clean event loop just for the forwarding to avoid any interference. */ r = sd_event_new(&event); if (r < 0) @@ -717,7 +816,7 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { _cleanup_(sd_json_variant_unrefp) sd_json_variant *jp = NULL; _cleanup_(sd_varlink_unrefp) sd_varlink *vl = NULL; const char *url, *method, *parameter, *source; - char **cmdline; + char **exec_cmdline; int r; assert(argc >= 3); @@ -731,9 +830,8 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--exec and --collect/--more/--oneway may not be combined."); if (arg_upgrade) { - /* TODO: support --exec with --upgrade, passing the upgraded socket fd to a child process */ - if (arg_exec || arg_collect || arg_method_flags != 0 || arg_push_fds.n_fds > 0 || !strv_isempty(arg_graceful)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--upgrade may not be combined with --exec/--collect/--more/--oneway/--push-fd=/--graceful."); + if (arg_collect || arg_method_flags != 0 || arg_push_fds.n_fds > 0 || !strv_isempty(arg_graceful)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "--upgrade may not be combined with --collect/--more/--oneway/--push-fd=/--graceful."); } (void) polkit_agent_open_if_enabled(BUS_TRANSPORT_LOCAL, arg_ask_password); @@ -741,7 +839,7 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { url = argv[1]; method = argv[2]; parameter = argc > 3 && !streq(argv[3], "-") ? argv[3] : NULL; - cmdline = strv_skip(argv, 4); + exec_cmdline = strv_skip(argv, 4); if (!varlink_idl_qualified_symbol_name_is_valid(method)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Not a valid qualified method name: '%s' (Expected valid Varlink interface name, followed by a dot, followed by a valid Varlink symbol name.)", method); @@ -754,7 +852,7 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { return log_error_errno(r, "Failed to parse parameters: %m"); } - return varlink_call_and_upgrade(url, method, jp); + return varlink_call_and_upgrade(url, method, jp, exec_cmdline); } /* No JSON mode explicitly configured? Then default to the same as -j (except if --exec is used, in @@ -938,10 +1036,6 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { if (mfd < 0) return log_error_errno(mfd, "Failed to allocate memfd for reply: %m"); - _cleanup_free_ char *j = strv_join(cmdline, " "); - if (!j) - return log_oom(); - int *fd_array = NULL, n = 0; size_t m = 0; CLEANUP_ARRAY(fd_array, m, close_many_and_free); @@ -968,66 +1062,13 @@ static int verb_call(int argc, char *argv[], uintptr_t _data, void *userdata) { * lives in our process their fds. Hence we will now no longer bubble up any * errors. */ - log_close(); - log_set_open_when_needed(true); - r = move_fd(mfd, STDIN_FILENO, /* cloexec= */ false); if (r < 0) { log_error_errno(r, "Failed to move reply to STDIN_FILENO: %m"); _exit(EXIT_FAILURE); } - r = close_all_fds(fd_array, m); - if (r < 0) { - log_error_errno(r, "Failed to close all remaining file descriptors: %m"); - _exit(EXIT_FAILURE); - } - - r = pack_fds(fd_array, m); - if (r < 0) { - log_error_errno(r, "Failed to rearrange file descriptors: %m"); - _exit(EXIT_FAILURE); - } - - r = fd_cloexec_many(fd_array, m, false); - if (r < 0) { - log_error_errno(r, "Failed to disable O_CLOEXEC for file descriptors: %m"); - _exit(EXIT_FAILURE); - } - - if (m > 0) { - r = setenvf("LISTEN_FDS", /* overwrite= */ true, "%zu", m); - if (r < 0) { - log_error_errno(r, "Failed to set $LISTEN_FDS environment variable: %m"); - _exit(EXIT_FAILURE); - } - - r = setenvf("LISTEN_PID", /* overwrite= */ true, PID_FMT, getpid_cached()); - if (r < 0) { - log_error_errno(r, "Failed to set $LISTEN_PID environment variable: %m"); - _exit(EXIT_FAILURE); - } - - uint64_t pidfdid; - if (pidfd_get_inode_id_self_cached(&pidfdid) >= 0) { - r = setenvf("LISTEN_PIDFDID", /* overwrite= */ true, "%" PRIu64, pidfdid); - if (r < 0) { - log_error_errno(r, "Failed to set $LISTEN_PIDFDID environment variable: %m"); - _exit(EXIT_FAILURE); - } - } - } else { - (void) unsetenv("LISTEN_FDS"); - (void) unsetenv("LISTEN_PID"); - (void) unsetenv("LISTEN_PIDFDID"); - } - (void) unsetenv("LISTEN_FDNAMES"); - - log_debug("Executing: %s", j); - - execvp(cmdline[0], cmdline); - log_error_errno(errno, "Failed to execute '%s': %m", j); - _exit(EXIT_FAILURE); + exec_with_listen_fds(exec_cmdline, fd_array, m); } if (arg_quiet) diff --git a/test/units/TEST-74-AUX-UTILS.varlinkctl.sh b/test/units/TEST-74-AUX-UTILS.varlinkctl.sh index 24b29e55c3..7f888756ae 100755 --- a/test/units/TEST-74-AUX-UTILS.varlinkctl.sh +++ b/test/units/TEST-74-AUX-UTILS.varlinkctl.sh @@ -297,9 +297,10 @@ out.flush() out.write(b"<<< UPGRADED >>>\n") out.write((json.dumps(received_parameters) + "\n").encode()) out.flush() -data = inp.read().decode().rstrip("\n") -out.write((data[::-1] + "\n").encode()) -out.flush() +for line in inp: + text = line.decode().rstrip("\n") + out.write((text[::-1] + "\n").encode()) + out.flush() if conn: conn.close() @@ -352,5 +353,22 @@ echo "$result" | grep "<<< UPGRADED >>>" >/dev/null echo "$result" | grep '"foo": "ssh"' >/dev/null echo "$result" | grep "tset epip hss" >/dev/null +# Start another server for --exec test +rm -f "$UPGRADE_SOCKET" +python3 "$UPGRADE_SERVER" "$UPGRADE_SOCKET" & +SERVER_PID=$! +timeout 5 bash -c "while [ ! -S '$UPGRADE_SOCKET' ]; do sleep 0.1; done" + +# Test --exec mode: the upgraded socket becomes stdin/stdout of the child. +# Since stdout goes to the socket (not the terminal), write results to a file for verification. +EXEC_RESULT="$(mktemp)" +varlinkctl call --upgrade --exec "unix:$UPGRADE_SOCKET" io.systemd.test.Reverse '{"foo":"bar"}' -- \ + bash -c "head -2 > '$EXEC_RESULT'; echo 'hello world'; head -1 >> '$EXEC_RESULT'" +grep "<<< UPGRADED >>>" "$EXEC_RESULT" >/dev/null +grep '"foo": "bar"' "$EXEC_RESULT" >/dev/null +grep "dlrow olleh" "$EXEC_RESULT" >/dev/null +rm -f "$EXEC_RESULT" + +wait "$SERVER_PID" || : rm -f "$UPGRADE_SOCKET" "$UPGRADE_SOCKET2" "$UPGRADE_SERVER" /tmp/test-upgrade-input rm -rf "$(dirname "$UPGRADE_SOCKET")" "$(dirname "$UPGRADE_SOCKET2")"