[libvirt] [PATCH] rpc: Don't leak fd via CreateXMLWithFiles

[Cole Robinson]

On 04/23/2016 06:52 PM, Cole Robinson wrote:
> From: Ben Gray <ben.r.gray at gmail.com>
> 
> FD passing APIs like CreateXMLWithFiles or OpenGraphicsFD will leak
> file descriptors. The user passes in an fd, which is dup()'d in
> virNetClientProgramCall. The new fd is what is transfered to the
> server virNetClientIOWriteMessage.
> 
> Once all the fds have been written though, the parent msg->fds list
> is immediately free'd, so the individual fds are never closed.
> 
> This closes each FD as its send to the server, so all fds have been
> closed by the time msg->fds is free'd.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1159766
> Signed-off-by: Cole Robinson <crobinso at redhat.com>
> ---
> Ben's patch, my comments
> 
>  src/rpc/virnetclient.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 781e74c..d8ed15b 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -1177,20 +1177,21 @@ virNetClientIOWriteMessage(virNetClientPtr client,
>  
>      if (thecall->msg->bufferOffset == thecall->msg->bufferLength) {
>          size_t i;
>          for (i = thecall->msg->donefds; i < thecall->msg->nfds; i++) {
>              int rv;
>              if ((rv = virNetSocketSendFD(client->sock, thecall->msg->fds[i])) < 0)
>                  return -1;
>              if (rv == 0) /* Blocking */
>                  return 0;
>              thecall->msg->donefds++;
> +            VIR_FORCE_CLOSE(thecall->msg->fds[i]);
>          }
>          thecall->msg->donefds = 0;
>          thecall->msg->bufferOffset = thecall->msg->bufferLength = 0;
>          VIR_FREE(thecall->msg->fds);
>          VIR_FREE(thecall->msg->buffer);
>          if (thecall->expectReply)
>              thecall->mode = VIR_NET_CLIENT_MODE_WAIT_RX;
>          else
>              thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE;
>      }
> 

I wanted to give a bit more comments.

I think this patch is correct as it can be with the current code. However the
pattern of freeing msg resources at this part of the code is concerning. Not
so sure about msg->buffer but definitely msg->fds

It seems like the proper place that the freeing should be taking place is in
virNetClientCallDispatchReply , which takes the server's response message and
stuffs the content in the original client message structure, which is returned
up to the API user. This function has code like:

    if (VIR_REALLOC_N(thecall->msg->buffer, client->msg.bufferLength) < 0)
        return -1;

    memcpy(thecall->msg->buffer, client->msg.buffer, client->msg.bufferLength);
    memcpy(&thecall->msg->header, &client->msg.header,
sizeof(client->msg.header));
    thecall->msg->bufferLength = client->msg.bufferLength;
    thecall->msg->bufferOffset = client->msg.bufferOffset;

    thecall->msg->nfds = client->msg.nfds;
    thecall->msg->fds = client->msg.fds;
    client->msg.nfds = 0;
    client->msg.fds = NULL;

In this case, thecall->msg is the same message that was passed to
virNetClientIOWriteMessage. Notice above there's a complete lack of VIR_FREE
cleanup of the original client message contents.

I think virNetClientCallDispatchReply is expecting the message to have been
virNetMessageClear'd first? I didn't tease the code flow out all the way to
figure it out.

Back to the VIR_FREE of msg->fds in virNetClientIOWriteMessage. This seems
problematic, because until virNetClientCallDispatchReply re-fills in the fds
structure from the server's response, the client message is in an inconsistent
state such that if an error triggers virNetMessageFree, the client will crash
trying to iterate over the free'd fds list, closing each fd.

That VIR_FREE(thecall->msg->fds); line was added by:


commit 1d8193ee8a7c9b6355468bd58e483d84fe1ed40b
Author: Sergey Fionov <fionov at gmail.com>
Date:   Sun Feb 17 18:20:59 2013 +0400

    Fix memory leak in virNetClientIOWriteMessage


Which was a response to:


commit 18937c3ae0990b4417a43aa07a2c35aaf8cb6ec2
Author: Daniel P. Berrange <berrange at redhat.com>
Date:   Fri Dec 21 16:49:12 2012 +0000

    Fix receiving of file descriptors from server

And I think the root problem is the lacking of free'ing in
virNetClientCallDispatchReply. So maybe the proper thing to do here is to fix
CallDispatchReply... but I'm not confident that's the only case we need to
care about, and it's confusing that the email thread of the memory leak commit
above says the leak was reproducible with non-fd using RPC calls, which kinda
confuses me, so I think I might be missing something.

Another bit that maybe we add to make the existing code more safe is
thecall->msg->fds = 0; before the problematic VIR_FREE call, which will likely
fix the potential virNetMessageFree crash, but maybe have other side effects.

Anyways, that's my brain dump, I don't really plan on digging into this any
deeper. I can dump it in a bug report too if people think it's useful

Thanks,
Cole