From nobody Mon Sep 21 06:47:16 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 Reviewed-by: Daniel P. Berrang=C3=A9 --- 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