On Sat, Sep 26, 2009 at 02:24:33AM +0200, Matthias Bolte wrote:

I tracked down a memleak in libvirtd's message processing. The leak was introduced in commit 47cab734995fa9521b1df05d37e9978eedd8d3a2 "Split out code for handling incoming method call messages". This commit changed the way qemud_client_message objects were reused.

Before this commit:

- qemudWorker() removes a message from dispatch queue and passes it to remoteDispatchClientRequest() - remoteDispatchClientRequest() handles the message and reuses the same message for the response. If an error occurs the same message is used to report it (rpc_error label). If another error occurs while trying to report the first error remoteDispatchClientRequest() would return -1 and the message will be freed in qemudWorker()

After this commit:

- qemudWorker() removes a message from dispatch queue and passes it to remoteDispatchClientRequest() - remoteDispatchClientRequest() calls remoteDispatchClientCall() if the message it is an remote call or otherwise respond with a new error message by calling remoteSerializeReplyError() and the original message leaks - remoteDispatchClientCall() handles the message and reuses the same message for the response. If an error occurs it calls remoteSerializeReplyError() and the original message leaks. If a fatal error occurs remoteDispatchClientCall() returns -1 and the original message will be freed in qemudWorker()

To fix this leak the original message has to be freed if remoteSerializeReplyError() succeeds. If remoteSerializeReplyError() fails the original message will be freed in qemudWorker().

In addition I came across another leak in remoteSerializeError(). If an error occurs while serializing the initial error the message leaks. To fix this leak the message has to be freed in case of an XDR error.

ACK to your patch - this is all my fault caught up in the many times I re-wrote the streams support. I really hate the contract of the remoteDispatchClientRequest() method, it should always be responsible for free'ing the 'msg' regardless of return status. I had actually refactored things todo that, but then I rewrote it again and forget that I had changed the error reporting functions thus leading to this leak. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|