[libvirt] [PATCH] qemu: fix memory leak in qemuAgentGetFSInfo

[Daniel P. Berrange]

On Tue, Mar 10, 2015 at 11:38:52AM +0100, Pavel Hrdina wrote:
> On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
> > On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
> > > in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
> > > 
> > > ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
> > > ==10670==    at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> > > ==10670==    by 0x509DEBD: virAlloc (viralloc.c:144)
> > > ==10670==    by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
> > > ==10670==    by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
> > > 
> > > Signed-off-by: Chen Fan <chen.fan.fnst at cn.fujitsu.com>
> > > ---
> > >  src/libvirt-domain.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > 
> > This does fix the memory leak and makes the function behave like it's
> > documented in virDomainGetFSInfo and virDomainFSInfoFree:
> > http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
> > http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
> > 
> > But it changes the public API - if there are applications that already
> > work around this function by freeing the info, this change would
> > introduce a double free.
> 
> Well, this is a good point that we might break somebody's application, but the
> virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on
> passed argument.  In the documentation the @info is an array and we specifically
> says that calling virDomainFSInfoFree on each element and then just free the
> @info is sufficient, but it isn't and there will be a memory leak.  It changes
> behavior of public API to correctly free the allocated memory as documentation
> says.  Let's hear another opinion, but I'm for fixing it.

I agree, in the normal way of usage of this API, this will always be wrong.
virsh is leaking this, python bindings are leaking this, perl bindings are
leaking this. Only applications written in C have the opportunity to do a
workaround, and most of our apps are not written in C. I think we have to
fix this really.

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