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 <[email protected]>
---
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
