diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 1c03cfc173..fcf1704792 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -2415,29 +2415,27 @@ static int varlink_handle_upgrade_fds(sd_varlink *v, int *ret_input_fd, int *ret return varlink_log_errno(v, r, "Failed to set output fd to blocking mode: %m"); } - /* Hand out the fds to the caller. When the caller doesn't want one direction, shut it - * down: but avoid closing the underlying fd if the other direction still needs it - * (i.e. when input_fd == output_fd). */ - bool same_fd = v->input_fd == v->output_fd; + /* For bidirectional sockets (input_fd == output_fd), dup the fd so that callers + * always get two independent fds they can close separately. */ + if (v->input_fd == v->output_fd) { + v->output_fd = fcntl(v->input_fd, F_DUPFD_CLOEXEC, 3); + if (v->output_fd < 0) + return varlink_log_errno(v, errno, "Failed to dup upgraded connection fd: %m"); + } + /* Hand out requested fds, shut down unwanted directions. */ if (ret_input_fd) *ret_input_fd = TAKE_FD(v->input_fd); else { (void) shutdown(v->input_fd, SHUT_RD); - if (same_fd && ret_output_fd) - TAKE_FD(v->input_fd); /* don't close yet, output branch needs it */ - else - v->input_fd = safe_close(v->input_fd); + v->input_fd = safe_close(v->input_fd); } if (ret_output_fd) *ret_output_fd = TAKE_FD(v->output_fd); else { (void) shutdown(v->output_fd, SHUT_WR); - if (same_fd && ret_input_fd) - TAKE_FD(v->output_fd); - else - v->output_fd = safe_close(v->output_fd); + v->output_fd = safe_close(v->output_fd); } return 0; diff --git a/src/systemd/sd-varlink.h b/src/systemd/sd-varlink.h index 0b999b9154..527415ab80 100644 --- a/src/systemd/sd-varlink.h +++ b/src/systemd/sd-varlink.h @@ -137,9 +137,9 @@ int sd_varlink_callb(sd_varlink *v, const char *method, sd_json_variant **ret_pa sd_varlink_callb((v), (method), (ret_parameters), (ret_error_id), SD_JSON_BUILD_OBJECT(__VA_ARGS__)) /* Send method call with upgrade, wait for reply, then steal the connection fds for raw I/O. - * For bidirectional sockets ret_input_fd and ret_output_fd will be the same fd. Callers - * that need independent fds should dup() one of them. ret_parameters and ret_error_id are - * borrowed references valid only until v is closed or unreffed. + * For bidirectional sockets ret_input_fd and ret_output_fd will be separate (dupped) fds + * referring to the same underlying socket. ret_parameters and ret_error_id are borrowed + * references valid only until v is closed or unreffed. * Returns > 0 if the connection was upgraded, 0 if a Varlink error occurred (and ret_error_id was set), * or < 0 on local failure. */ int sd_varlink_call_and_upgrade(sd_varlink *v, const char *method, sd_json_variant *parameters, sd_json_variant **ret_parameters, const char **ret_error_id, int *ret_input_fd, int *ret_output_fd); @@ -171,9 +171,9 @@ int sd_varlink_replyb(sd_varlink *v, ...); /* Send a final reply to an upgrade request, then steal the connection fds for raw I/O. * The fds are returned in blocking mode. The varlink connection is disconnected afterwards. - * For bidirectional sockets ret_input_fd and ret_output_fd will be the same fd. Callers - * that need independent fds should dup() one of them. For pipe pairs (e.g. ssh-exec - * transport) they will differ. Either ret pointer may be NULL. + * For bidirectional sockets ret_input_fd and ret_output_fd will be separate (dupped) fds + * referring to the same underlying socket. For pipe pairs (e.g. ssh-exec transport) they + * will differ. Either ret pointer may be NULL. * * Note: this call synchronously blocks until the reply is flushed to the socket. This is * usually fine as flush is fast but a misbehaving/adversary client that stops reading diff --git a/src/test/test-varlink.c b/src/test/test-varlink.c index 36a4639376..219966c5d7 100644 --- a/src/test/test-varlink.c +++ b/src/test/test-varlink.c @@ -763,10 +763,6 @@ static int method_upgrade(sd_varlink *link, sd_json_variant *parameters, sd_varl if (r < 0) return r; - /* For a socketpair connection, both fds point to the same socket — avoid double-close */ - if (input_fd == output_fd) - output_fd = -EBADF; - /* After upgrade, do raw I/O: read until EOF, reverse, write back. * The client shuts down its write side after sending, so we get a clean EOF. */ char buf[64] = {}; @@ -777,8 +773,7 @@ static int method_upgrade(sd_varlink *link, sd_json_variant *parameters, sd_varl for (ssize_t i = 0; i < n / 2; i++) SWAP_TWO(buf[i], buf[n - 1 - i]); - int write_fd = output_fd >= 0 ? output_fd : input_fd; - ASSERT_OK(loop_write(write_fd, buf, n)); + ASSERT_OK(loop_write(output_fd, buf, n)); return 0; } @@ -807,16 +802,12 @@ static void *upgrade_thread(void *arg) { ASSERT_NULL(error_id); ASSERT_GE(input_fd, 0); ASSERT_GE(output_fd, 0); + ASSERT_NE(input_fd, output_fd); /* library dups for bidirectional sockets */ - /* For a socketpair connection, both fds point to the same socket — avoid double-close */ - if (input_fd == output_fd) - output_fd = -EBADF; - - /* Send a test string, expect reversed reply */ + /* Send a test string, shut down write side so server sees EOF, then read the reversed reply */ static const char msg[] = "Hello!"; - int write_fd = output_fd >= 0 ? output_fd : input_fd; - ASSERT_OK(loop_write(write_fd, msg, strlen(msg))); - ASSERT_OK_ERRNO(shutdown(write_fd, SHUT_WR)); + ASSERT_OK(loop_write(output_fd, msg, strlen(msg))); + ASSERT_OK_ERRNO(shutdown(output_fd, SHUT_WR)); char buf[64] = {}; ssize_t n = ASSERT_OK(loop_read(input_fd, buf, strlen(msg), /* do_poll= */ true)); diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index 1876b90bde..9a8e89b26c 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -672,15 +672,6 @@ static int varlink_call_and_upgrade(const char *url, const char *method, sd_json if (!isempty(error_id)) return log_error_errno(SYNTHETIC_ERRNO(EBADE), "Upgrade via %s() failed with error: %s", method, error_id); - /* For bidirectional sockets input_fd == output_fd. Dup immediately so that _cleanup_close_ - * on both variables can never double-close the same fd. Note that on fcntl() failure - * output_fd is overwritten with -1, so only input_fd holds the real fd at cleanup time. */ - if (input_fd == output_fd) { - output_fd = fcntl(input_fd, F_DUPFD_CLOEXEC, 3); - if (output_fd < 0) - 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. */ @@ -1213,8 +1204,8 @@ static int varlink_server_add_interface_from_method(sd_varlink_server *s, const static int method_serve_upgrade(sd_varlink *link, sd_json_variant *parameters, sd_varlink_method_flags_t flags, void *userdata) { char **exec_cmdline = ASSERT_PTR(userdata); - _cleanup_close_ int input_fd = -EBADF, _output_fd = -EBADF; - int output_fd, r; + _cleanup_close_ int input_fd = -EBADF, output_fd = -EBADF; + int r; if (!FLAGS_SET(flags, SD_VARLINK_METHOD_UPGRADE)) return sd_varlink_error(link, SD_VARLINK_ERROR_EXPECTED_UPGRADE, NULL); @@ -1223,9 +1214,6 @@ static int method_serve_upgrade(sd_varlink *link, sd_json_variant *parameters, s if (r < 0) return log_error_errno(r, "Failed to upgrade connection: %m"); - if (output_fd != input_fd) - _output_fd = output_fd; - /* Copy exec_cmdline before forking: pidref_safe_fork() calls rename_process() which * overwrites the argv area that exec_cmdline points into. */ _cleanup_strv_free_ char **cmdline_copy = strv_copy(exec_cmdline);