[PATCH 0/2] Fix double free of stream error reply aborting libvirtd
libvirtd aborts with "free(): invalid pointer" whenever a stream write fails, and then crash-loops until systemd's start limit is reached, leaving the host with no management plane. Running guests are unaffected but become unmanageable. The cause is an ambiguous return contract. daemonStreamHandleWriteData() reports a failed stream write through virNetServerProgramSendReplyError(), which takes ownership of the message and queues it on client->tx, and then returns that function's result - 0 on success. Its caller reads 0 as "no reply was sent" and, for VIR_NET_CONTINUE, clears the message and queues it a second time. Since the message is by then the sole element of client->tx, virNetMessageQueuePush() walks to the tail - itself - and links it to itself. virNetMessageQueueServe() then hands out the same pointer twice and virNetServerClientDispatchWrite() frees it twice. Patch 1 is the fix: give the handlers a distinct return value meaning "already queued, do not touch msg again". daemonStreamHandleHole() had the same defect and is fixed alongside. Note the requeue test has to change from "ret > 0" to "ret == 1", otherwise the new value would requeue a message which is already on client->tx. Patch 2 is independent hardening: make virNetMessageQueuePush() refuse a push that would corrupt the list, so a caller bug of this shape surfaces as a log message rather than as heap corruption. It includes a regression test that reproduces the self-cycle deterministically. Note that virNetMessageClear() memsets the whole message including ->next, so a queued message can appear unlinked. That is why patch 2 walks the queue rather than testing msg->next, and why this class of bug is easy to miss by inspection. There is no ABI change; src/libvirt_remote.syms is untouched. Evidence -------- - valgrind memcheck: "Invalid free()" with the previous free at the *same* call site, plus 13 invalid reads and 6 invalid writes as the "while (client->tx)" loop re-reads the freed block. Exactly one invalid free per occurrence, as a self-cycle predicts. - A production core dump whose crash IP is the return address of the same call to virNetMessageFree(), resolving to virnetserverclient.c:1374. - Instrumented builds logging both push sites: virNetServerProgramSendError:168 followed by daemonStreamHandleWrite:797. Two cautions for anyone reproducing this: Under valgrind the daemon does *not* abort - memcheck replaces the allocator, so glibc's malloc_printerr never runs. It logs and continues, and the fault is easy to mistake for "works fine". Freed virNetMessage blocks are promptly reused by malloc, so the same address legitimately reappears as a new message moments after being freed. In RPC debug logs that closely resembles a use-after-free and is not one. Testing ------- Reproduced and fixed on three production hypervisors running 12.5.0 with these patches backported. One host had logged 5197 aborts beforehand; across all three there have been zero aborts and zero hardening warnings since. The hardening warning firing before the patch 1 fix and never firing after it is the direct evidence that the root cause, and not just the symptom, is addressed. Full test suite passes on master (306 ok, 1 expected fail, 0 failures). CI on a personal fork is green: 22 jobs passed, 0 failed, covering Fedora 43/44/rawhide, CentOS Stream 9/10, Ubuntu 24.04/26.04 (including clang), openSUSE Tumbleweed/Leap 16, Debian 13, armv7l and mingw32/64, plus check-dco and codestyle. Reported as https://gitlab.com/libvirt/libvirt/-/issues/902 Ross Golder (2): remote: don't queue the stream error reply twice rpc: refuse to queue a message that is already queued src/remote/remote_daemon_stream.c | 51 +++++++++++++----- src/rpc/virnetmessage.c | 43 +++++++++++++++ tests/virnetmessagetest.c | 87 +++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 12 deletions(-) -- 2.53.0
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=dumped, status=6/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 == 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 <ross@golder.org> --- 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_stream.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 *stream) /* * 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 *client, virErrorRestore(&err); - 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; } return 0; @@ -680,6 +687,13 @@ daemonStreamHandleAbort(virNetServerClient *client, } +/* + * 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); - 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; } return 0; @@ -772,7 +791,7 @@ daemonStreamHandleWrite(virNetServerClient *client, ret = -1; } - if (ret > 0) { + if (ret == 1) { /* still processing data from msg, put it back into queue */ msg->next = stream->rx; stream->rx = msg; @@ -785,6 +804,14 @@ daemonStreamHandleWrite(virNetServerClient *client, return -1; } + if (ret == 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 -- 2.53.0
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 <ross@golder.org> --- 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); virNetMessageClearPayload(msg); + + /* Make a stale reference from a queue detectable rather than dangling */ + msg->next = NULL; + g_free(msg); } +static bool +virNetMessageQueueContains(virNetMessage *queue, virNetMessage *msg) +{ + virNetMessage *tmp; + + for (tmp = queue; tmp; tmp = tmp->next) { + if (tmp == msg) + return true; + } + + return false; +} + + void virNetMessageQueuePush(virNetMessage **queue, virNetMessage *msg) { virNetMessage *tmp = *queue; VIR_DEBUG("queue=%p msg=%p", queue, msg); + /* 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 (queue=%p *queue=%p msg->next=%p)", + msg, queue, *queue, msg->next); + return; + } + if (tmp) { while (tmp->next) tmp = tmp->next; @@ -129,6 +162,16 @@ virNetMessage *virNetMessageQueueServe(virNetMessage **queue) if (tmp) { *queue = 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 == tmp) { + VIR_WARN("Detected self-referencing message %p on queue %p, breaking cycle", + tmp, queue); + *queue = NULL; + } } VIR_DEBUG("queue serve end queue=%p *queue=%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) } +static size_t +testMessageQueueLength(virNetMessage *queue) +{ + virNetMessage *tmp; + size_t len = 0; + + /* Bounded so a corrupted (cyclic) queue cannot hang the test */ + for (tmp = queue; tmp && len < 100; tmp = tmp->next) + len++; + + return len; +} + + +static int testMessageQueueDuplicatePush(const void *args G_GNUC_UNUSED) +{ + virNetMessage *queue = NULL; + virNetMessage *msgA = virNetMessageNew(false); + virNetMessage *msgB = virNetMessageNew(false); + int ret = -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 != msgA) { + VIR_TEST_DEBUG("Expected queue head %p, got %p", msgA, queue); + goto cleanup; + } + + if (msgA->next != NULL) { + VIR_TEST_DEBUG("Message linked to itself: msgA->next=%p", msgA->next); + goto cleanup; + } + + if (testMessageQueueLength(queue) != 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) != 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) != msgA) { + VIR_TEST_DEBUG("Expected msgA to be served first"); + goto cleanup; + } + + if (virNetMessageQueueServe(&queue) != msgB) { + VIR_TEST_DEBUG("Expected msgB to be served second"); + goto cleanup; + } + + if (queue != NULL || virNetMessageQueueServe(&queue) != NULL) { + VIR_TEST_DEBUG("Expected queue to be empty"); + goto cleanup; + } + + ret = 0; + cleanup: + virNetMessageFree(msgA); + virNetMessageFree(msgB); + return ret; +} + + static int mymain(void) { @@ -535,6 +619,9 @@ mymain(void) if (virTestRun("Message Payload Stream Encode", testMessagePayloadStreamEncode, NULL) < 0) ret = -1; + if (virTestRun("Message Queue Duplicate Push", testMessageQueueDuplicatePush, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.53.0
participants (1)
-
Ross Golder