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