[libvirt] [PATCH 6/9] daemonStreamHandleRead: Rework to follow our coding pattern

[John Ferlan]

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 at redhat.com>
> ---
>  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;
>  }
>