From 0bd40fdc0727e50098831abbdff028e5bf836586 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 13 Aug 2026 05:52:28 +0900 Subject: [PATCH] json-stream: do not take ownership of passed fds on failure This makes json_stream_attach_fds() and json_stream_connect_fd_pair() leave the passed file descriptors with the caller on failure. Callers should call TAKE_FD() only after a successful call. Also make qmp_client_connect_fd() always take ownership of the passed file descriptor, including on failure. Previously, ownership of the passed file descriptor on failure was unclear and depended on where the function failed. --- src/libsystemd/sd-json/json-stream.c | 39 +++++++++++++++----------- src/libsystemd/sd-varlink/sd-varlink.c | 15 ++++++---- src/shared/qmp-client.c | 8 ++++-- src/test/test-qmp-client-qemu.c | 9 ++---- src/vmspawn/vmspawn-qmp.c | 6 ++-- src/vmspawn/vmspawn.c | 4 +-- 6 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/libsystemd/sd-json/json-stream.c b/src/libsystemd/sd-json/json-stream.c index 66d9fa9f61..defda4a414 100644 --- a/src/libsystemd/sd-json/json-stream.c +++ b/src/libsystemd/sd-json/json-stream.c @@ -199,7 +199,6 @@ int json_stream_connect_address(JsonStream *s, const char *address) { r = connect_unix_path(sock_fd, AT_FDCWD, address); } else r = RET_NERRNO(connect(sock_fd, &sockaddr.sa, r)); - if (r < 0) { if (!IN_SET(r, -EAGAIN, -EINPROGRESS)) return json_stream_log_errno(s, r, "Failed to connect to %s: %m", address); @@ -212,8 +211,12 @@ int json_stream_connect_address(JsonStream *s, const char *address) { s->flags |= JSON_STREAM_CONNECTING; } - int fd = TAKE_FD(sock_fd); - return json_stream_attach_fds(s, fd, fd); + r = json_stream_attach_fds(s, sock_fd, sock_fd); + if (r < 0) + return r; + + TAKE_FD(sock_fd); + return 0; } int json_stream_attach_fds(JsonStream *s, int input_fd, int output_fd) { @@ -227,17 +230,9 @@ int json_stream_attach_fds(JsonStream *s, int input_fd, int output_fd) { assert(output_fd != s->input_fd); assert(output_fd != s->output_fd); - /* NB: input_fd and output_fd are donated to the JsonStream instance! */ + /* NB: input_fd and output_fd are donated to the JsonStream instance on success. */ - if (s->input_fd != s->output_fd) { - safe_close(s->input_fd); - safe_close(s->output_fd); - } else - safe_close(s->input_fd); - - s->input_fd = input_fd; - s->output_fd = output_fd; - s->flags &= ~(JSON_STREAM_PREFER_READ|JSON_STREAM_PREFER_WRITE); + JsonStreamFlags flags = s->flags & ~(JSON_STREAM_PREFER_READ|JSON_STREAM_PREFER_WRITE); /* Detect non-socket fds up front so the read/write paths use read()/write() for * non-socket fds and send()/recv() for sockets (mostly for MSG_NOSIGNAL). */ @@ -245,17 +240,24 @@ int json_stream_attach_fds(JsonStream *s, int input_fd, int output_fd) { if (fstat(input_fd, &st) < 0) return -errno; if (!S_ISSOCK(st.st_mode)) - s->flags |= JSON_STREAM_PREFER_READ; + flags |= JSON_STREAM_PREFER_READ; } if (output_fd >= 0 && output_fd != input_fd) { if (fstat(output_fd, &st) < 0) return -errno; if (!S_ISSOCK(st.st_mode)) - s->flags |= JSON_STREAM_PREFER_WRITE; - } else if (FLAGS_SET(s->flags, JSON_STREAM_PREFER_READ)) - s->flags |= JSON_STREAM_PREFER_WRITE; + flags |= JSON_STREAM_PREFER_WRITE; + } else if (FLAGS_SET(flags, JSON_STREAM_PREFER_READ)) + flags |= JSON_STREAM_PREFER_WRITE; + if (s->input_fd != s->output_fd) + safe_close(s->output_fd); + safe_close(s->input_fd); + + s->input_fd = input_fd; + s->output_fd = output_fd; + s->flags = flags; return 0; } @@ -266,6 +268,9 @@ int json_stream_connect_fd_pair(JsonStream *s, int input_fd, int output_fd) { assert(input_fd >= 0); assert(output_fd >= 0); + /* NB: input_fd and output_fd are donated to the JsonStream instance on success. + * The fds may be changed to non-blocking mode even on failure. */ + r = fd_nonblock(input_fd, true); if (r < 0) return json_stream_log_errno(s, r, "Failed to make input fd %d nonblocking: %m", input_fd); diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index 5d27723834..cb8864635e 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -307,11 +307,12 @@ _public_ int sd_varlink_connect_exec(sd_varlink **ret, const char *_command, cha if (r < 0) return log_debug_errno(r, "Failed to create varlink object: %m"); - int conn_fd = TAKE_FD(client_fd); - r = json_stream_attach_fds(&v->stream, conn_fd, conn_fd); + r = json_stream_attach_fds(&v->stream, client_fd, client_fd); if (r < 0) return r; + TAKE_FD(client_fd); + v->exec_pidref = TAKE_PIDREF(pidref); varlink_set_state(v, VARLINK_IDLE_CLIENT); @@ -395,11 +396,12 @@ static int varlink_connect_ssh_unix(sd_varlink **ret, const char *where) { if (r < 0) return log_debug_errno(r, "Failed to create varlink object: %m"); - int conn_fd = TAKE_FD(client_fd); - r = json_stream_attach_fds(&v->stream, conn_fd, conn_fd); + r = json_stream_attach_fds(&v->stream, client_fd, client_fd); if (r < 0) return r; + TAKE_FD(client_fd); + v->exec_pidref = TAKE_PIDREF(pidref); varlink_set_state(v, VARLINK_IDLE_CLIENT); @@ -490,10 +492,13 @@ static int varlink_connect_ssh_exec(sd_varlink **ret, const char *where) { if (r < 0) return log_debug_errno(r, "Failed to create varlink object: %m"); - r = json_stream_attach_fds(&v->stream, TAKE_FD(output_pipe[0]), TAKE_FD(input_pipe[1])); + r = json_stream_attach_fds(&v->stream, output_pipe[0], input_pipe[1]); if (r < 0) return r; + TAKE_FD(output_pipe[0]); + TAKE_FD(input_pipe[1]); + v->exec_pidref = TAKE_PIDREF(pidref); varlink_set_state(v, VARLINK_IDLE_CLIENT); diff --git a/src/shared/qmp-client.c b/src/shared/qmp-client.c index 8de903de1a..89b8551126 100644 --- a/src/shared/qmp-client.c +++ b/src/shared/qmp-client.c @@ -655,10 +655,12 @@ static int qmp_client_capabilities_reply( int qmp_client_connect_fd(QmpClient **ret, int fd) { _cleanup_(qmp_client_unrefp) QmpClient *c = NULL; + _cleanup_close_ int fd_close = ASSERT_FD(TAKE_FD(fd)); int r; assert(ret); - assert(fd >= 0); + + /* This function always takes ownership of the passed fd. */ c = new(QmpClient, 1); if (!c) @@ -681,10 +683,12 @@ int qmp_client_connect_fd(QmpClient **ret, int fd) { if (r < 0) return r; - r = json_stream_connect_fd_pair(&c->stream, fd, fd); + r = json_stream_connect_fd_pair(&c->stream, fd_close, fd_close); if (r < 0) return r; + TAKE_FD(fd_close); + /* Eagerly queue qmp_capabilities. QEMU accepts commands as soon as the socket opens * — its greeting is informational and doesn't gate writes on our side. FIFO ordering * of the output queue guarantees cap precedes any user command a later invoke() diff --git a/src/test/test-qmp-client-qemu.c b/src/test/test-qmp-client-qemu.c index 813b9c5687..82d7129af9 100644 --- a/src/test/test-qmp-client-qemu.c +++ b/src/test/test-qmp-client-qemu.c @@ -123,12 +123,11 @@ TEST(qmp_client_qemu_handshake_and_schema) { ASSERT_OK(start_qemu(qemu, qmp_fds[1], &pidref)); qmp_fds[1] = safe_close(qmp_fds[1]); - r = qmp_client_connect_fd(&client, qmp_fds[0]); + r = qmp_client_connect_fd(&client, TAKE_FD(qmp_fds[0])); if (r < 0) { log_tests_skipped_errno(r, "QMP connect failed (QEMU may not support -machine none)"); return; } - TAKE_FD(qmp_fds[0]); ASSERT_OK(qmp_client_attach_event(client, event, SD_EVENT_PRIORITY_NORMAL)); @@ -187,12 +186,11 @@ TEST(qmp_client_qemu_query_status) { ASSERT_OK(start_qemu(qemu, qmp_fds[1], &pidref)); qmp_fds[1] = safe_close(qmp_fds[1]); - r = qmp_client_connect_fd(&client, qmp_fds[0]); + r = qmp_client_connect_fd(&client, TAKE_FD(qmp_fds[0])); if (r < 0) { log_tests_skipped_errno(r, "QMP connect failed"); return; } - TAKE_FD(qmp_fds[0]); ASSERT_OK(qmp_client_attach_event(client, event, SD_EVENT_PRIORITY_NORMAL)); @@ -277,12 +275,11 @@ TEST(qmp_client_qemu_add_fd) { ASSERT_OK(start_qemu(qemu, qmp_fds[1], &pidref)); qmp_fds[1] = safe_close(qmp_fds[1]); - r = qmp_client_connect_fd(&client, qmp_fds[0]); + r = qmp_client_connect_fd(&client, TAKE_FD(qmp_fds[0])); if (r < 0) { log_tests_skipped_errno(r, "QMP connect failed"); return; } - TAKE_FD(qmp_fds[0]); ASSERT_OK(qmp_client_attach_event(client, event, SD_EVENT_PRIORITY_NORMAL)); diff --git a/src/vmspawn/vmspawn-qmp.c b/src/vmspawn/vmspawn-qmp.c index 092f8dae44..dccf7309cc 100644 --- a/src/vmspawn/vmspawn-qmp.c +++ b/src/vmspawn/vmspawn-qmp.c @@ -1863,19 +1863,21 @@ static int probe_schema(QmpClient *c, VmspawnQmpBridge *bridge) { int vmspawn_qmp_init(VmspawnQmpBridge **ret, int fd, sd_event *event) { _cleanup_(vmspawn_qmp_bridge_freep) VmspawnQmpBridge *bridge = NULL; + _cleanup_close_ int fd_close = ASSERT_FD(TAKE_FD(fd)); int r; assert(ret); - assert(fd >= 0); assert(event); + /* This function always takes ownership of the passed fd. */ + bridge = new0(VmspawnQmpBridge, 1); if (!bridge) return log_oom(); bridge->scsi_controller_port_idx = -1; - r = qmp_client_connect_fd(&bridge->qmp, fd); + r = qmp_client_connect_fd(&bridge->qmp, TAKE_FD(fd_close)); if (r < 0) return log_error_errno(r, "Failed to create QMP client: %m"); diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index f3501c3b66..725e90dbec 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -3865,12 +3865,10 @@ static int run_virtual_machine(int kvm_device_fd, int vhost_device_fd) { /* Connect to VMM backend */ _cleanup_(vmspawn_qmp_bridge_freep) VmspawnQmpBridge *bridge = NULL; - r = vmspawn_qmp_init(&bridge, bridge_fds[0], event); + r = vmspawn_qmp_init(&bridge, TAKE_FD(bridge_fds[0]), event); if (r < 0) return r; - TAKE_FD(bridge_fds[0]); - /* Probe QEMU feature availability synchronously before device setup consumes the flags. */ r = vmspawn_qmp_probe_features(bridge); if (r < 0)