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

[Daniel P. Berrange]

On Sat, Apr 23, 2016 at 07:02:40PM -0400, Cole Robinson wrote:
> 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.

Yep, it is.

> 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.

I think its valid to claim that either virNetClientCallDispatchReply or
virNetClientIOWriteMessage are appropriate places to free the messsage
contents. The problem is really that we're not doing a thorough job
at clearing it out in virNetClientIOWriteMessage.

The reason we don't use virNetMessageClear at that point is that we do
not want to blow away the msg->header field, as that contains info we
need in order to validate the reply.

The fix above is correcet wrt to the FD leak. We should also be
setting msg->nfds = 0; when we free msg->fds, to protect against
the crash you describe.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|