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

On 04/23/2016 06:52 PM, Cole Robinson wrote: > From: Ben Gray <ben.r.gray(a)gmail.com&gt; > > 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(a)redhat.com&gt; > --- > 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(a)gmail.com&gt; 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(a)redhat.com&gt; 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.