Re: [libvirt] Fix memleaks in libvirtd's message processing

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