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@xxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxx> > 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@xxxxxxxxxx> > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list