From nobody Tue Aug 4 22:19:11 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 38.145.34.151 as permitted sender) client-ip=38.145.34.151; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 38.145.34.151 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [38.145.34.151]) by mx.zohomail.com with SMTPS id 1785746231167929.1491777437922; Mon, 3 Aug 2026 01:37:11 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 993) id 92908417CA; Mon, 3 Aug 2026 04:37:09 -0400 (EDT) Received: from [172.19.199.10] (unknown [10.16.107.18]) by lists.libvirt.org (Postfix) with ESMTP id E8D0F41B63; Mon, 3 Aug 2026 04:33:19 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 993) id DB3A53FB96; Sun, 2 Aug 2026 09:47:18 -0400 (EDT) Received: from v5244.v57ae4e16.euw1.send.eu.mailgun.net (v5244.v57ae4e16.euw1.send.eu.mailgun.net [161.38.204.244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with UTF8SMTPS id B475B3FAAD for ; Sun, 2 Aug 2026 09:47:16 -0400 (EDT) Received: from x870e.golder.lan (node-u79.pool-125-27.dynamic.nt-isp.net [125.27.152.229]) by 144294aa76f8373def79bdd025f3f4c8c9008d3202958d6ffc3f44d277f01fa0 with SMTP id 6a6f4935cd45e8032b6498c2 (version=TLS1.3, cipher=TLS_AES_128_GCM_SHA256); Sun, 02 Aug 2026 13:42:13 GMT X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=4.0.1 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=golder.org; q=dns/txt; s=s1; t=1785678435; x=1785685635; h=Content-Transfer-Encoding: Content-Type: MIME-Version: References: In-Reply-To: Message-ID: Date: Subject: Subject: Cc: To: To: From: From: Sender: Sender; bh=+OIwZ7TJz32FSyUXubSLcUo+7rcI5fLzxalOCuUYZzA=; b=nZBCwuKWat74ezJhVHqu0sIijVM7iJkXYlvfOcJbJDOlqKm63mA3RupuE9fVucLCOl4Ob0yzwhXd/Dn8v/aMmDrCFrw3+ogj++gN6vgB8N1ElBFaZ1AWCFZDMiyw6W3jVsa6eh4sC6qgryxLUjuuyIfoZYF22j7kxVX175wT5xPrrK/q6O7b+pVhP41mfCrKmYO5ogeBL2v26h2S6UwfFCIb7mGD6etO3qHg4R97GL6b8eNQXY4mgMGMsErzghGWVz7CxJ9xb69zLk13C5PHf5JUR1S6PzctYEfYwZAqs5paOh8bl4AvccjyCOcLaz/F4HrzQx3armB+P91i/t9HSw== X-Mailgun-Sid: WyI2MTcxOCIsImRldmVsQGxpc3RzLmxpYnZpcnQub3JnIiwiNjI1ZDciXQ== X-Mailgun-Sending-Ip: 161.38.204.244 Sender: ross@golder.org From: Ross Golder To: devel@lists.libvirt.org Subject: [PATCH 1/2] remote: don't queue the stream error reply twice Date: Sun, 2 Aug 2026 20:42:05 +0700 Message-ID: <218f78fd8d61db4005bd7c4d429ead72f44acae8.1785660713.git.ross@golder.org> X-Mailer: git-send-email 2.53.0 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-MailFrom: bounce+bcaaa1.625d7-devel=lists.libvirt.org@golder.org X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.lists.libvirt.org-0; emergency; member-moderation Message-ID-Hash: SAVPW4LL7LOSKI3KOLSQ7C6VXQUWBL52 X-Message-ID-Hash: SAVPW4LL7LOSKI3KOLSQ7C6VXQUWBL52 X-Mailman-Approved-At: Mon, 03 Aug 2026 08:33:00 +0000 CC: Ross Golder X-Mailman-Version: 3.3.10 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Invalid signature record: Signature is expired since 16h.) X-ZM-MESSAGEID: 1785746232195158500 Content-Type: text/plain; charset="utf-8" When a stream write fails, daemonStreamHandleWriteData() reports the error to the client via virNetServerProgramSendReplyError(), which takes ownership of 'msg' and queues it on client->tx. It then returns that function's return value, which is 0 on success. Its caller daemonStreamHandleWrite() treats 0 as "the handler did not send anything", so for VIR_NET_CONTINUE it clears the message and queues it a second time to release the client's request slot. As the message is by then the sole element of client->tx, virNetMessageQueuePush() walks to the tail - which is the message itself - and links it to itself. The resulting cycle makes virNetMessageQueueServe() hand out the same pointer twice, and virNetServerClientDispatchWrite() frees it twice: libvirtd[109078]: free(): invalid pointer systemd[1]: libvirtd.service: Main process exited, code=3Ddumped, status=3D6/ABRT The daemon then crash-loops until systemd's start limit is reached. Note that virNetMessageClear() memsets the whole message, including ->next, so the doubly-queued message looks unlinked and the condition is not detectable by inspecting msg->next alone. Give the handlers a distinct return value 2, meaning "fully processed and already queued, the caller must not touch msg again", and honour it in daemonStreamHandleWrite(). The requeue test changes from "ret > 0" to "ret =3D=3D 1" - otherwise the new value would requeue a message which is already on client->tx. daemonStreamHandleHole() had the identical defect and is fixed the same way. daemonStreamHandleFinish() and daemonStreamHandleAbort() also consume the message, but are only reached for VIR_NET_OK and VIR_NET_ERROR respectively, so the VIR_NET_CONTINUE re-send never applies to them. Closes: https://gitlab.com/libvirt/libvirt/-/issues/902 Signed-off-by: Ross Golder --- src/remote/remote_daemon_stream.c | 51 +++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_s= tream.c index 3777c8e684..f2514f9d5f 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -537,8 +537,10 @@ daemonRemoveAllClientStreams(daemonClientStream *strea= m) /* * Returns: * -1 if fatal error occurred - * 0 if message was fully processed + * 0 if message was fully processed and the caller still owns 'msg' * 1 if message is still being processed + * 2 if message was fully processed and has already been queued for + * sending, so the caller must not touch 'msg' again */ static int daemonStreamHandleWriteData(virNetServerClient *client, @@ -577,11 +579,16 @@ daemonStreamHandleWriteData(virNetServerClient *clien= t, =20 virErrorRestore(&err); =20 - return virNetServerProgramSendReplyError(stream->prog, - client, - msg, - &rerr, - &msg->header); + /* SendReplyError() takes ownership of 'msg' and queues it on the + * client, so tell the caller not to send it a second time */ + if (virNetServerProgramSendReplyError(stream->prog, + client, + msg, + &rerr, + &msg->header) < 0) + return -1; + + return 2; } =20 return 0; @@ -680,6 +687,13 @@ daemonStreamHandleAbort(virNetServerClient *client, } =20 =20 +/* + * Returns: + * -1 if fatal error occurred + * 0 if message was fully processed and the caller still owns 'msg' + * 2 if message was fully processed and has already been queued for + * sending, so the caller must not touch 'msg' again + */ static int daemonStreamHandleHole(virNetServerClient *client, daemonClientStream *stream, @@ -714,11 +728,16 @@ daemonStreamHandleHole(virNetServerClient *client, virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); =20 - return virNetServerProgramSendReplyError(stream->prog, - client, - msg, - &rerr, - &msg->header); + /* SendReplyError() takes ownership of 'msg' and queues it on the + * client, so tell the caller not to send it a second time */ + if (virNetServerProgramSendReplyError(stream->prog, + client, + msg, + &rerr, + &msg->header) < 0) + return -1; + + return 2; } =20 return 0; @@ -772,7 +791,7 @@ daemonStreamHandleWrite(virNetServerClient *client, ret =3D -1; } =20 - if (ret > 0) { + if (ret =3D=3D 1) { /* still processing data from msg, put it back into queue */ msg->next =3D stream->rx; stream->rx =3D msg; @@ -785,6 +804,14 @@ daemonStreamHandleWrite(virNetServerClient *client, return -1; } =20 + if (ret =3D=3D 2) { + /* The handler hit an error and has already queued 'msg' on the + * client as the error reply. Sending it again below would push + * a message which is still on client->tx back onto that same + * queue, linking it to itself and freeing it twice. */ + continue; + } + /* 'CONTINUE' messages don't send a reply (unless error * occurred), so to release the 'msg' object we need to * send a fake zero-length reply. Nothing actually gets --=20 2.53.0 From nobody Tue Aug 4 22:19:11 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 38.145.34.151 as permitted sender) client-ip=38.145.34.151; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 38.145.34.151 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [38.145.34.151]) by mx.zohomail.com with SMTPS id 1785746332764735.0266452738199; Mon, 3 Aug 2026 01:38:52 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 993) id A28613FAD0; Mon, 3 Aug 2026 04:38:50 -0400 (EDT) Received: from [172.19.199.10] (unknown [10.16.107.18]) by lists.libvirt.org (Postfix) with ESMTP id 6424A41B70; Mon, 3 Aug 2026 04:33:25 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 993) id 1C01D3FA64; Sun, 2 Aug 2026 09:47:19 -0400 (EDT) Received: from v5244.v57ae4e16.euw1.send.eu.mailgun.net (v5244.v57ae4e16.euw1.send.eu.mailgun.net [161.38.204.244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with UTF8SMTPS id 375553FCA7 for ; Sun, 2 Aug 2026 09:47:16 -0400 (EDT) Received: from x870e.golder.lan (node-u79.pool-125-27.dynamic.nt-isp.net [125.27.152.229]) by 144294aa76f8373def79bdd025f3f4c8c9008d3202958d6ffc3f44d277f01fa0 with SMTP id 6a6f49378927fc5a1c5f6abe (version=TLS1.3, cipher=TLS_AES_128_GCM_SHA256); Sun, 02 Aug 2026 13:42:15 GMT X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=4.0.1 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=golder.org; q=dns/txt; s=s1; t=1785678436; x=1785685636; h=Content-Transfer-Encoding: Content-Type: MIME-Version: References: In-Reply-To: Message-ID: Date: Subject: Subject: Cc: To: To: From: From: Sender: Sender; bh=hnsxkMFbAp8D4ErbHZ8R574eZfqblvgI4DUBnvSmBD4=; b=YFD/l199UN65gHZwoyRdtOeqQ/N0ifQzbWetQtYbVNMnXDk1iZtuNouySRFiI/LPNn1pMlNtjHUBBjmo5JfOsBh/yaeRA5VA8ulo2M1C1vU07+kzJzis9ohyqA6bch+FPg6tRDcT3Dnhs+KDDk/lfRaBYraVFNrfIZ3/MlD4WTH2acEksbHQ1IAB8zyCtt9sdUFdm+ONXxM5KELIcjKmVS2tIQPAYwqe23gGBT5dRZSVyU5KzLnKVAIgcfFuB0PuvYn9NxmaX0+SfPSGJSa5F/R8UKCQEmWMVP3ACYYgEQXLQOA892CVNPrYLUA/Xz0PJhF1PSakR/OIZGRn9lbfcw== X-Mailgun-Sid: WyI2MTcxOCIsImRldmVsQGxpc3RzLmxpYnZpcnQub3JnIiwiNjI1ZDciXQ== X-Mailgun-Sending-Ip: 161.38.204.244 Sender: ross@golder.org From: Ross Golder To: devel@lists.libvirt.org Subject: [PATCH 2/2] rpc: refuse to queue a message that is already queued Date: Sun, 2 Aug 2026 20:42:06 +0700 Message-ID: X-Mailer: git-send-email 2.53.0 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-MailFrom: bounce+bcaaa1.625d7-devel=lists.libvirt.org@golder.org X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.lists.libvirt.org-0; emergency; member-moderation Message-ID-Hash: 74JGA7WOA7NISP5JOEOIFRZVCMLPLVYQ X-Message-ID-Hash: 74JGA7WOA7NISP5JOEOIFRZVCMLPLVYQ X-Mailman-Approved-At: Mon, 03 Aug 2026 08:33:00 +0000 CC: Ross Golder X-Mailman-Version: 3.3.10 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Invalid signature record: Signature is expired since 16h.) X-ZM-MESSAGEID: 1785746334741158500 Content-Type: text/plain; charset="utf-8" virNetMessageQueuePush() appends by walking to the tail of the queue. If it is handed a message which is already in that queue, and that message happens to be the tail, it links the message to itself. The cycle then makes virNetMessageQueueServe() return the same pointer on consecutive calls, and callers which free what they are served - such as virNetServerClientDispatchWrite() - free it twice, aborting the process with "free(): invalid pointer". Refuse such a push and warn instead, so that a caller bug shows up as a diagnosable log message rather than as heap corruption some time later. The check has to walk the queue rather than just test msg->next, because virNetMessageClear() memsets the whole message: a queued message which has been cleared appears unlinked while still being referenced. Also break the cycle in virNetMessageQueueServe() if one is somehow already present, rather than handing out the same message indefinitely, and clear msg->next in virNetMessageFree() so a stale reference held by a queue is detectable instead of dangling. The accompanying test reproduces the self-cycle deterministically: with the check removed it fails with "Message linked to itself". Signed-off-by: Ross Golder --- src/rpc/virnetmessage.c | 43 +++++++++++++++++++ tests/virnetmessagetest.c | 87 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index e66df5c9e2..eda8430f70 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -102,15 +102,48 @@ void virNetMessageFree(virNetMessage *msg) msg->cb(msg, msg->opaque); =20 virNetMessageClearPayload(msg); + + /* Make a stale reference from a queue detectable rather than dangling= */ + msg->next =3D NULL; + g_free(msg); } =20 +static bool +virNetMessageQueueContains(virNetMessage *queue, virNetMessage *msg) +{ + virNetMessage *tmp; + + for (tmp =3D queue; tmp; tmp =3D tmp->next) { + if (tmp =3D=3D msg) + return true; + } + + return false; +} + + void virNetMessageQueuePush(virNetMessage **queue, virNetMessage *msg) { virNetMessage *tmp =3D *queue; =20 VIR_DEBUG("queue=3D%p msg=3D%p", queue, msg); =20 + /* A message which is already linked into a queue must never be pushed + * again. If it happens to be the tail of this very queue, the loop + * below would link it to itself, and virNetMessageQueueServe() would + * then hand out the same message repeatedly - which the callers go on + * to free more than once. + * + * Note that virNetMessageClear() memsets ->next, so a message can be + * queued and yet appear unlinked; the queue has to be walked. + */ + if (msg->next || virNetMessageQueueContains(*queue, msg)) { + VIR_WARN("Refusing to queue message %p which is already queued (qu= eue=3D%p *queue=3D%p msg->next=3D%p)", + msg, queue, *queue, msg->next); + return; + } + if (tmp) { while (tmp->next) tmp =3D tmp->next; @@ -129,6 +162,16 @@ virNetMessage *virNetMessageQueueServe(virNetMessage *= *queue) =20 if (tmp) { *queue =3D g_steal_pointer(&tmp->next); + + /* A message linked to itself means the queue was corrupted by a + * duplicate push; serving it would hand out the same pointer + * indefinitely. Break the cycle rather than looping on it. + */ + if (*queue =3D=3D tmp) { + VIR_WARN("Detected self-referencing message %p on queue %p, br= eaking cycle", + tmp, queue); + *queue =3D NULL; + } } =20 VIR_DEBUG("queue serve end queue=3D%p *queue=3D%p", queue, *queue); diff --git a/tests/virnetmessagetest.c b/tests/virnetmessagetest.c index e426bc7791..72ec4c0ce7 100644 --- a/tests/virnetmessagetest.c +++ b/tests/virnetmessagetest.c @@ -511,6 +511,90 @@ static int testMessagePayloadStreamEncode(const void *= args G_GNUC_UNUSED) } =20 =20 +static size_t +testMessageQueueLength(virNetMessage *queue) +{ + virNetMessage *tmp; + size_t len =3D 0; + + /* Bounded so a corrupted (cyclic) queue cannot hang the test */ + for (tmp =3D queue; tmp && len < 100; tmp =3D tmp->next) + len++; + + return len; +} + + +static int testMessageQueueDuplicatePush(const void *args G_GNUC_UNUSED) +{ + virNetMessage *queue =3D NULL; + virNetMessage *msgA =3D virNetMessageNew(false); + virNetMessage *msgB =3D virNetMessageNew(false); + int ret =3D -1; + + if (!msgA || !msgB) + goto cleanup; + + /* Pushing the same message twice must not corrupt the queue. Without + * the check in virNetMessageQueuePush() this links msgA to itself, + * and serving the queue then returns it forever - the callers going + * on to free it more than once. + */ + virNetMessageQueuePush(&queue, msgA); + virNetMessageQueuePush(&queue, msgA); + + if (queue !=3D msgA) { + VIR_TEST_DEBUG("Expected queue head %p, got %p", msgA, queue); + goto cleanup; + } + + if (msgA->next !=3D NULL) { + VIR_TEST_DEBUG("Message linked to itself: msgA->next=3D%p", msgA->= next); + goto cleanup; + } + + if (testMessageQueueLength(queue) !=3D 1) { + VIR_TEST_DEBUG("Expected queue length 1, got %zu", + testMessageQueueLength(queue)); + goto cleanup; + } + + /* A distinct message must still append normally, and re-pushing an + * already queued non-tail message must also be refused. + */ + virNetMessageQueuePush(&queue, msgB); + virNetMessageQueuePush(&queue, msgA); + + if (testMessageQueueLength(queue) !=3D 2) { + VIR_TEST_DEBUG("Expected queue length 2, got %zu", + testMessageQueueLength(queue)); + goto cleanup; + } + + /* Serving must hand out each message exactly once, then empty */ + if (virNetMessageQueueServe(&queue) !=3D msgA) { + VIR_TEST_DEBUG("Expected msgA to be served first"); + goto cleanup; + } + + if (virNetMessageQueueServe(&queue) !=3D msgB) { + VIR_TEST_DEBUG("Expected msgB to be served second"); + goto cleanup; + } + + if (queue !=3D NULL || virNetMessageQueueServe(&queue) !=3D NULL) { + VIR_TEST_DEBUG("Expected queue to be empty"); + goto cleanup; + } + + ret =3D 0; + cleanup: + virNetMessageFree(msgA); + virNetMessageFree(msgB); + return ret; +} + + static int mymain(void) { @@ -535,6 +619,9 @@ mymain(void) if (virTestRun("Message Payload Stream Encode", testMessagePayloadStre= amEncode, NULL) < 0) ret =3D -1; =20 + if (virTestRun("Message Queue Duplicate Push", testMessageQueueDuplica= tePush, NULL) < 0) + ret =3D -1; + return ret =3D=3D 0 ? EXIT_SUCCESS : EXIT_FAILURE; } =20 --=20 2.53.0