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@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@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@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@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 :|