On 04/15/2016 09:51 AM, Michal Privoznik wrote:
> Usually, we have this 'if() goto cleanup;' pattern in our new
> code. It is going to be useful here too. Thing is, there was a
> memleak. If there has been an error in
> virNetServerProgramSendStreamError() or
> virNetServerProgramSendStreamData() created message was never
> freed.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> daemon/stream.c | 68 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/daemon/stream.c b/daemon/stream.c
> index a2a370c..cf42a10 100644
> --- a/daemon/stream.c
> +++ b/daemon/stream.c
> @@ -709,9 +709,12 @@ static int
> daemonStreamHandleRead(virNetServerClientPtr client,
> daemonClientStream *stream)
> {
> + virNetMessagePtr msg = NULL;
> + virNetMessageError rerr;
> char *buffer;
> size_t bufferLen = VIR_NET_MESSAGE_LEGACY_PAYLOAD_MAX;
> - int ret;
> + int ret = -1;
> + int rv;
>
> VIR_DEBUG("client=%p, stream=%p tx=%d closed=%d",
> client, stream, stream->tx, stream->closed);
> @@ -728,50 +731,47 @@ daemonStreamHandleRead(virNetServerClientPtr client,
> if (!stream->tx)
> return 0;
>
> + memset(&rerr, 0, sizeof(rerr));
> +
> if (VIR_ALLOC_N(buffer, bufferLen) < 0)
> return -1;
>
> - ret = virStreamRecv(stream->st, buffer, bufferLen);
> - if (ret == -2) {
> + if (!(msg = virNetMessageNew(false)))
> + goto cleanup;
> +
> + rv = virStreamRecv(stream->st, buffer, bufferLen);
> + if (rv == -2) {
> /* Should never get this, since we're only called when we know
> * we're readable, but hey things change... */
If for some reason rv == -2, then you later set "msg = NULL" which leaks
it (Coverity found)
I assume 'msg' gets 'eaten' by virNetServerProgramSendStreamError and
virNetServerProgramSendStreamData, so then after successful return from
either that's when the "msg = NULL;" should be done.
John
> - ret = 0;
> - } else if (ret < 0) {
> - virNetMessagePtr msg;
> - virNetMessageError rerr;
> -
> - memset(&rerr, 0, sizeof(rerr));
> -
> - if (!(msg = virNetMessageNew(false)))
> - ret = -1;
> - else
> - ret = virNetServerProgramSendStreamError(remoteProgram,
> - client,
> - msg,
> - &rerr,
> - stream->procedure,
> - stream->serial);
> + } else if (rv < 0) {
> + if (virNetServerProgramSendStreamError(remoteProgram,
> + client,
> + msg,
> + &rerr,
> + stream->procedure,
> + stream->serial) < 0)
> + goto cleanup;
> } else {
> - virNetMessagePtr msg;
> stream->tx = false;
> - if (ret == 0)
> + if (rv == 0)
> stream->recvEOF = true;
> - if (!(msg = virNetMessageNew(false)))
> - ret = -1;
>
> - if (msg) {
> - msg->cb = daemonStreamMessageFinished;
> - msg->opaque = stream;
> - stream->refs++;
> - ret = virNetServerProgramSendStreamData(remoteProgram,
> - client,
> - msg,
> - stream->procedure,
> - stream->serial,
> - buffer, ret);
> - }
> + msg->cb = daemonStreamMessageFinished;
> + msg->opaque = stream;
> + stream->refs++;
> + if (virNetServerProgramSendStreamData(remoteProgram,
> + client,
> + msg,
> + stream->procedure,
> + stream->serial,
> + buffer, rv) < 0)
> + goto cleanup;
> }
>
> + msg = NULL;
^^^^
If (rv == -2) this is leaked.
> + ret = 0;
> + cleanup:
> VIR_FREE(buffer);
> + virNetMessageFree(msg);
> return ret;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list