6:02 p.m.
On 04/23/2016 06:52 PM, Cole Robinson wrote:
From: Ben Gray <ben.r.gray(a)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(a)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(a)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(a)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